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

William Tu u9012063 at gmail.com
Tue Mar 24 14:30:40 UTC 2020


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.

Thanks
William


More information about the dev mailing list