| Message ID | 1584998537-115892-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] fatal-signal: Skip acquiring log fd lock. | expand |
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.
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
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
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.
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
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.
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'
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
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.
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; }
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(-)