[ovs-dev] [patch v3 1/4] conntrack: Stop exporting internal datastructures.

Ben Pfaff blp at ovn.org
Wed Dec 12 02:55:00 UTC 2018


On Tue, Dec 11, 2018 at 06:05:11PM -0800, Darrell Ball wrote:
> On Tue, Dec 11, 2018 at 8:51 AM Ben Pfaff <blp at ovn.org> wrote:
> 
> > On Mon, Dec 10, 2018 at 07:18:43PM -0800, Darrell Ball wrote:
> > > On Mon, Dec 10, 2018 at 5:22 PM Ben Pfaff <blp at ovn.org> wrote:
> > >
> > > > On Mon, Dec 10, 2018 at 03:47:17PM -0800, Ben Pfaff wrote:
> > > > > On Sun, Dec 02, 2018 at 09:17:16PM -0800, Darrell Ball wrote:
> > > > > > Remove the exporting of the main internal conntrack datastructure.
> > > > > > These are made static.  Also stop passing around a pointer
> > parameter
> > > > > > to all the internal datastructures; only one or two is used
> > > > > > for a given code path and these can be referenced directly and
> > passed
> > > > > > specifically where appropriate.
> > > > > >
> > > > > > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
> > > > >
> > > > > Seems fine, I applied this to master.  Thank you!
> > > >
> > > > Actually I had to un-apply this because:
> > > >
> > > > 1099: dpctl - add-dp del-dp                           FAILED (
> > > > ovs-macros.at:193)
> > > > 1100: dpctl - add-if set-if del-if                    FAILED (
> > > > ovs-macros.at:193)
> > > >
> > > > due to the following Address Sanitizer reports.  The following is for
> > > > 1099 but the one for 1100 is almost identical:
> > > >
> > > > =================================================================
> > > > ==17824==ERROR: AddressSanitizer: SEGV on unknown address 0xeafffba8
> > (pc
> > > > 0xf657d67d bp 0x00000000 sp 0xffc65150 T0)
> > > > ==17824==The signal is caused by a READ memory access.
> > > >     #0 0xf657d67c in __GI___pthread_timedjoin_ex
> > > > (/lib/i386-linux-gnu/libpthread.so.0+0x767c)
> > > >     #1 0xf657d5c3 in pthread_join
> > > > (/lib/i386-linux-gnu/libpthread.so.0+0x75c3)
> > > >     #2 0xf7522c24 in conntrack_destroy ../lib/conntrack.c:378
> > > >
> > >
> > >
> > > I don't see this using the address sanitizer; I am using
> > >
> > > *../configure** CFLAGS="-g -O2 **-march=native**  -fsanitize=address
> > > -fno-omit-frame-pointer -fno-common" **--with-linux=/lib/modules/`uname
> > > -r`/build CC=gcc **--enable-Werror*
> > >
> > > Furthermore, I don't see a functional change here, but I'll double check.
> >
> > When I apply the following patch:
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index a69026d6f32f..f9bdfb6c2aa3 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -338,6 +338,7 @@ ct_print_conn_info(const struct conn *c, const char
> > *log_msg,
> >  void
> >  conntrack_init(void)
> >  {
> > +    VLOG_WARN("%s:%d", __FILE__, __LINE__);
> >      long long now = time_msec();
> >
> >      ct_rwlock_init(&resources_lock);
> > @@ -374,6 +375,7 @@ conntrack_init(void)
> >  void
> >  conntrack_destroy(void)
> >  {
> > +    VLOG_WARN("%s:%d", __FILE__, __LINE__);
> >      latch_set(&clean_thread_exit);
> >      pthread_join(clean_thread, NULL);
> >      latch_destroy(&clean_thread_exit);
> >
> > The test shows the following logging:
> >
> > --- /dev/null   2018-11-19 11:45:05.360009223 -0800
> > +++ /home/blp/nicira/ovs/_build/tests/testsuite.dir/at-groups/1099/stdout
> >      2018-12-11 08:50:04.623158278 -0800
> > @@ -0,0 +1,3 @@
> > +2018-12-11T16:50:04.582Z|00008|conntrack|WARN|../lib/conntrack.c:341
> > +2018-12-11T16:50:04.596Z|00031|conntrack|WARN|../lib/conntrack.c:341
> > +2018-12-11T16:50:04.613Z|00038|conntrack|WARN|../lib/conntrack.c:378
> >
> > which indicates that conntrack_init() is being called more than once.
> >
> 
> 
> Thanks; I am glad your running of the address sanitizer flagged this at
> least.
> All the built-in testing, including Valgrind and manual testing even with 2
> bridges did not, providing a false sense of security.
> Missed the forest for PPS tress.

You're welcome!  (Really I got lucky ;-)


More information about the dev mailing list