[ovs-dev] fatal-signal: Skip acquiring log fd lock.
diff mbox series

Message ID 1584998537-115892-1-git-send-email-u9012063@gmail.com
State New
Headers show
Series
  • [ovs-dev] fatal-signal: Skip acquiring log fd lock.
Related show

Commit Message

William Tu March 23, 2020, 9:22 p.m. UTC
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@gmail.com>
---
 lib/fatal-signal.c | 55 +++++++++++++++++++++++++++++++++---------------------
 lib/vlog.c         |  1 +
 2 files changed, 35 insertions(+), 21 deletions(-)

Comments

Ben Pfaff March 23, 2020, 9:34 p.m. UTC | #1
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@gmail.com>

You can't just mark the existing send_backtrace_to_monitor()
OVS_NO_THREAD_SAFETY_ANALYSIS?  It seems like the easiest change.

Also, the OVS_REQUIRES should also go in vlog.h.

Thanks,

Ben.
William Tu March 23, 2020, 9:58 p.m. UTC | #2
On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <blp@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@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.
OK
Thanks,
William
William Tu March 23, 2020, 10:34 p.m. UTC | #3
On Mon, Mar 23, 2020 at 2:58 PM William Tu <u9012063@gmail.com> wrote:
>
> On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <blp@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@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?

Thanks,
William
Ilya Maximets March 23, 2020, 11:13 p.m. UTC | #4
On 3/23/20 11:34 PM, William Tu wrote:
> On Mon, Mar 23, 2020 at 2:58 PM William Tu <u9012063@gmail.com> wrote:
>>
>> On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <blp@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@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)));
    }
}

William, Ben, WDYT?

Best regards, Ilya Maximets.
William Tu March 23, 2020, 11:47 p.m. UTC | #5
On Mon, Mar 23, 2020 at 4:13 PM Ilya Maximets <i.maximets@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@gmail.com> wrote:
> >>
> >> On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <blp@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@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)));
>     }
> }
>
> William, Ben, WDYT?
>
Hi Ilya,
I think this is a good idea, let me work on it.
Thanks
William
Ben Pfaff March 23, 2020, 11:49 p.m. UTC | #6
On Tue, Mar 24, 2020 at 12:13:12AM +0100, Ilya Maximets wrote:
> On 3/23/20 11:34 PM, William Tu wrote:
> > On Mon, Mar 23, 2020 at 2:58 PM William Tu <u9012063@gmail.com> wrote:
> >>
> >> On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <blp@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@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)));
>     }
> }
> 
> William, Ben, WDYT?

I like that solution.  Thanks.
William Tu March 24, 2020, 4:26 a.m. UTC | #7
On Mon, Mar 23, 2020 at 4:13 PM Ilya Maximets <i.maximets@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@gmail.com> wrote:
> >>
> >> On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <blp@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@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'
William Tu March 24, 2020, 2:30 p.m. UTC | #8
On Mon, Mar 23, 2020 at 9:26 PM William Tu <u9012063@gmail.com> wrote:
>
> On Mon, Mar 23, 2020 at 4:13 PM Ilya Maximets <i.maximets@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@gmail.com> wrote:
> > >>
> > >> On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <blp@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@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
Ilya Maximets March 24, 2020, 2:33 p.m. UTC | #9
On 3/24/20 3:30 PM, William Tu wrote:
> On Mon, Mar 23, 2020 at 9:26 PM William Tu <u9012063@gmail.com> wrote:
>>
>> On Mon, Mar 23, 2020 at 4:13 PM Ilya Maximets <i.maximets@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@gmail.com> wrote:
>>>>>
>>>>> On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <blp@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@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.

Patch
diff mbox series

diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
index 4965c1ae82c0..44c53e5465a0 100644
--- a/lib/fatal-signal.c
+++ b/lib/fatal-signal.c
@@ -158,13 +158,43 @@  fatal_signal_add_hook(void (*hook_cb)(void *aux), void (*cancel_cb)(void *aux),
 }
 
 #ifdef HAVE_UNWIND
-/* Send the backtrace buffer to monitor thread.
+/* Write the backtrace to log file.
+ *
+ * Use OVS_NO_THREAD_SAFETY_ANALYSIS to skip acquiring the lock
+ * of 'vlog_get_fd()'.
+ */
+static inline void
+send_backtrace_to_logfd(struct unw_backtrace *unw_bt, const int dep)
+    OVS_NO_THREAD_SAFETY_ANALYSIS
+{
+    char str[] = "SIGSEGV detected, backtrace:\n";
+
+    if (vlog_get_fd() < 0) {
+        return;
+    }
+
+    ignore(write(vlog_get_fd(), str, strlen(str)));
+
+    for (int i = 0; i < dep; i++) {
+        char line[64];
+
+        /* snprintf() is not async-signal-safe. */
+        snprintf(line, 64, "0x%016"PRIxPTR" <%s+0x%"PRIxPTR">\n",
+                 unw_bt[i].ip,
+                 unw_bt[i].func,
+                 unw_bt[i].offset);
+        ignore(write(vlog_get_fd(), line, strlen(line)));
+    }
+}
+
+/* Send the backtrace buffer to main or monitor thread.
  *
  * Note that this runs in the signal handling context, any system
  * library functions used here must be async-signal-safe.
  */
 static inline void
-send_backtrace_to_monitor(void) {
+send_backtrace_to_monitor(void)
+{
     /* volatile added to prevent a "clobbered" error on ppc64le with gcc */
     volatile int dep;
     struct unw_backtrace unw_bt[UNW_MAX_DEPTH];
@@ -192,26 +222,9 @@  send_backtrace_to_monitor(void) {
                      dep * sizeof(struct unw_backtrace)));
     } else {
         /* Since there is no monitor daemon running, write backtrace
-         * in current process.  This is not asyn-signal-safe due to
-         * use of snprintf().
+         * in current process.  This is not asyn-signal-safe.
          */
-        char str[] = "SIGSEGV detected, backtrace:\n";
-
-        if (vlog_get_fd() < 0) {
-            return;
-        }
-
-        ignore(write(vlog_get_fd(), str, strlen(str)));
-
-        for (int i = 0; i < dep; i++) {
-            char line[64];
-
-            snprintf(line, 64, "0x%016"PRIxPTR" <%s+0x%"PRIxPTR">\n",
-                     unw_bt[i].ip,
-                     unw_bt[i].func,
-                     unw_bt[i].offset);
-            ignore(write(vlog_get_fd(), line, strlen(line)));
-        }
+        send_backtrace_to_logfd(unw_bt, dep);
     }
 }
 #else
diff --git a/lib/vlog.c b/lib/vlog.c
index 502b33fc831e..0ea57abadd9e 100644
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -614,6 +614,7 @@  vlog_set_syslog_target(const char *target)
 
 int
 vlog_get_fd(void)
+    OVS_REQUIRES(log_file_mutex)
 {
     return log_fd;
 }