[ovs-dev] [PATCH] fatal-signal: Skip acquiring log fd lock.

[Ilya Maximets]

On 3/24/20 3:30 PM, William Tu wrote:
> On Mon, Mar 23, 2020 at 9:26 PM William Tu <u9012063 at gmail.com> wrote:
>>
>> On Mon, Mar 23, 2020 at 4:13 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>>>
>>> On 3/23/20 11:34 PM, William Tu wrote:
>>>> On Mon, Mar 23, 2020 at 2:58 PM William Tu <u9012063 at gmail.com> wrote:
>>>>>
>>>>> On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <blp at ovn.org> wrote:
>>>>>>
>>>>>> On Mon, Mar 23, 2020 at 02:22:17PM -0700, William Tu wrote:
>>>>>>> Due to not acquiring lock, clang reports:
>>>>>>>   lib/vlog.c:618:12: error: reading variable 'log_fd' requires holding mutex
>>>>>>>   'log_file_mutex' [-Werror,-Wthread-safety-analysis]
>>>>>>>   return log_fd;
>>>>>>> The patch fixes by skipping it using OVS_NO_THREAD_SAFETY_ANALYSIS.
>>>>>>>
>>>>>>> Tested-at: https://travis-ci.org/github/williamtu/ovs-travis/builds/666051210
>>>>>>> Fixex: ecd4a8fcdff2 ("fatal-signal: Log backtrace when no monitor daemon.")
>>>>>>> Signed-off-by: William Tu <u9012063 at gmail.com>
>>>>>>
>>>>>> You can't just mark the existing send_backtrace_to_monitor()
>>>>>> OVS_NO_THREAD_SAFETY_ANALYSIS?  It seems like the easiest change.
>>>>> OK
>>>>> that will also work.
>>>>>>
>>>>>> Also, the OVS_REQUIRES should also go in vlog.h.
>>>>
>>>> Adding 'OVS_REQUIRES(log_file_mutext)' to vlog.h has error like this:
>>>> ./include/openvswitch/vlog.h:147:36: error: use of undeclared
>>>> identifier 'log_file_mutex'
>>>> int vlog_get_fd(void) OVS_REQUIRES(log_file_mutex);
>>>>
>>>> I will have to move log_file_mutex and pattern_rwlock to vlog.h.
>>>> Do we want this?
>>>> Does it make difference if OVS_REQUIRES is only used in C file
>>>> but not in header file?
>>>
>>> To avoid exposure of the vlog internals, maybe we could create a new function like:
>>>
>>> /*
>>>  * This function writes directly to log file without using async writer or
>>>  * taking a lock.  Caller must hold 'log_file_mutex' or be sure that it's
>>>  * not necessary.  Could be used in exceptional cases like dumping of backtrace
>>>  * on fatal signals.
>>>  */
>>> void
>>> vlog_direct_write_to_log_file_unsafe(const char *s)
>>>     OVS_NO_THREAD_SAFETY_ANALYSIS
>>> {
>>>     if (log_fd >= 0) {
>>>         ignore(write(log_fd, s, strlen(s)));
>>>     }
>>> }
>>>
>>
>> Hi Ilya,
>>
>> Do you know any restrictions when calling functions in signal handler?
>> When use this approach, the backtrace does not print anything.
>> I'm still debugging it with the below patch:
>>
>> diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
>> index 476bf3d6d5b4..886fce5e0fd5 100644
>> --- a/include/openvswitch/vlog.h
>> +++ b/include/openvswitch/vlog.h
>> @@ -143,8 +143,8 @@ void vlog_set_syslog_method(const char *method);
>>  /* Configure syslog target. */
>>  void vlog_set_syslog_target(const char *target);
>>
>> -/* Return the log_fd. */
>> -int vlog_get_fd(void);
>> +/* Write directly to log file. */
>> +void vlog_direct_write_to_log_file_unsafe(const char *s);
>>
>>  /* Initialization. */
>>  void vlog_init(void);
>> diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
>> index 4965c1ae82c0..51cf628d994e 100644
>> --- a/lib/fatal-signal.c
>> +++ b/lib/fatal-signal.c
>> @@ -197,11 +197,7 @@ send_backtrace_to_monitor(void) {
>>           */
>>          char str[] = "SIGSEGV detected, backtrace:\n";
>>
>> -        if (vlog_get_fd() < 0) {
>> -            return;
>> -        }
>> -
>> -        ignore(write(vlog_get_fd(), str, strlen(str)));
>> +        vlog_direct_write_to_log_file_unsafe(str);
>>
>>          for (int i = 0; i < dep; i++) {
>>              char line[64];
>> @@ -210,7 +206,7 @@ send_backtrace_to_monitor(void) {
>>                       unw_bt[i].ip,
>>                       unw_bt[i].func,
>>                       unw_bt[i].offset);
>> -            ignore(write(vlog_get_fd(), line, strlen(line)));
>> +            vlog_direct_write_to_log_file_unsafe(line);
>>          }
>>      }
>>  }
>> diff --git a/lib/vlog.c b/lib/vlog.c
>> index 502b33fc831e..6d17d4837e9c 100644
>> --- a/lib/vlog.c
>> +++ b/lib/vlog.c
>> @@ -612,10 +612,19 @@ vlog_set_syslog_target(const char *target)
>>      ovs_rwlock_unlock(&pattern_rwlock);
>>  }
>>
>> -int
>> -vlog_get_fd(void)
>> +/*
>> + * This function writes directly to log file without using async writer or
>> + * taking a lock.  Caller must hold 'log_file_mutex' or be sure that it's
>> + * not necessary.  Could be used in exceptional cases like dumping of backtrace
>> + * on fatal signals.
>> + */
>> +void
>> +vlog_direct_write_to_log_file_unsafe(const char *s)
>> +    OVS_NO_THREAD_SAFETY_ANALYSIS
>>  {
>> -    return log_fd;
>> +    if (log_fd >= 0) {
>> +        ignore(write(log_fd, s, strlen(s)));
>> +    }
>>  }
>>
>>  /* Returns 'false' if 'facility' is not a valid string. If 'facility'
> 
> I have the issue fixed and sent v2 patch
> https://mail.openvswitch.org/pipermail/ovs-dev/2020-March/369070.html
> 
> The issue was that I compile the code with address sanitizer below:
> ./configure CC=clang --enable-Werror CFLAGS="-g -O2 -fsanitize=address"
> Then no backtrace is shown.
> Remove '-fsanitize=address' or use GCC works OK.

OK.  Makes sense.