[lttng-dev] Bulletproof RCU arena resize bug

[Mathieu Desnoyers]

FYI, this fix uncovered other issues, fixed by the following master branch commits:

c1be8fb Fix: urcu-bp segfault in glibc pthread_kill()
95b9424 Fix urcu-bp: don't move registry
5c02e37 Fix: compat futex duplicated lock and completion
976720e Fix: i386 compat code duplicated mutex instances
9d8612b Fix: urcu-bp: Bulletproof RCU arena resize bug
f61dfd9 Fix: test_mutex.c uninitialized mutex

Comments are welcome,

Thanks,

Mathieu

----- Original Message -----
> From: "Mathieu Desnoyers" <mathieu.desnoyers at efficios.com>
> To: "Milosz Tanski" <milosz at adfin.com>
> Cc: lttng-dev at lists.lttng.org
> Sent: Monday, September 30, 2013 12:09:59 PM
> Subject: Re: [lttng-dev] Bulletproof RCU arena resize bug
> 
> 
> 
> ----- Original Message -----
> > From: "Mathieu Desnoyers" <mathieu.desnoyers at efficios.com>
> > To: "Milosz Tanski" <milosz at adfin.com>
> > Cc: lttng-dev at lists.lttng.org
> > Sent: Monday, September 30, 2013 11:49:11 AM
> > Subject: Re: [lttng-dev] Bulletproof RCU arena resize bug
> > 
> > ----- Original Message -----
> > > From: "Milosz Tanski" <milosz at adfin.com>
> > > To: lttng-dev at lists.lttng.org
> > > Sent: Monday, September 30, 2013 11:16:37 AM
> > > Subject: [lttng-dev] Bulletproof RCU arena resize bug
> > > 
> > > Hi,
> > > 
> > > While trying to use the BP flavor of RCU I ran into random crashes. I
> > > tracked it down to issues with resizing of the BP RCU memory pool.
> > > 
> > > The problem is in the urcu-bp.c file in the resize_arena() function.
> > > On successful allocation / remapping the len member of the
> > > registry_arena struct is never set anywhere function. On the second
> > > resize of the arena the code in resize_arena() still thinks the
> > > previous size is equal to the original mapping size. I've fixed this
> > > issue locally by just adding the following code at the bottom of
> > > resize_arena().
> > > 
> > > I hope this helps,
> > 
> > Good catch !!
> > 
> > However, I think your fix misses one case: if we happen to re-use the same
> > region, we want to update the length right ? I propose:
> 
> Sorry, my previous patch was wrong: I updated arena->len before it was used
> (expecting old value).
> 
> Here is an updated fix. It includes a bit of cleanup so we can make some
> sense of the variables there.
> 
> diff --git a/urcu-bp.c b/urcu-bp.c
> index 34bd14e..93d781f 100644
> --- a/urcu-bp.c
> +++ b/urcu-bp.c
> @@ -305,27 +305,37 @@ int rcu_read_ongoing(void)
>   */
>  static void resize_arena(struct registry_arena *arena, size_t len)
>  {
> -       void *new_arena;
> +       void *new_p;
> +       size_t old_len;
> +
> +       old_len = arena->len;
> 
>         if (!arena->p)
> -               new_arena = mmap(arena->p, len,
> -                                PROT_READ | PROT_WRITE,
> -                                MAP_ANONYMOUS | MAP_PRIVATE,
> -                                -1, 0);
> +               new_p = mmap(arena->p, len,
> +                       PROT_READ | PROT_WRITE,
> +                       MAP_ANONYMOUS | MAP_PRIVATE,
> +                       -1, 0);
>         else
> -               new_arena = mremap_wrapper(arena->p, arena->len,
> -                                       len, MREMAP_MAYMOVE);
> -       assert(new_arena != MAP_FAILED);
> -       arena->len = len;
> +               new_p = mremap_wrapper(arena->p, old_len,
> +                       len, MREMAP_MAYMOVE);
> +       assert(new_p != MAP_FAILED);
> +
> +       /*
> +        * Zero the newly allocated memory. Since mmap() does not
> +        * clearly specify if memory is zeroed or not (although it is
> +        * very likely that it is), be extra careful by not expecting
> +        * the new range to be zeroed by mremap.
> +        */
> +       bzero(new_p + old_len, len - old_len);
> 
>         /*
> -        * re-used the same region ?
> +        * If we did not re-use the same region, we need to update the
> +        * arena pointer.
>          */
> -       if (new_arena == arena->p)
> -               return;
> +       if (new_p != arena->p)
> +               arena->p = new_p;
> 
> -       bzero(new_arena + arena->len, len - arena->len);
> -       arena->p = new_arena;
> +       arena->len = len;
>  }
> 
>  /* Called with signals off and mutex locked */
> 
> Thoughts ?
> 
> Thanks,
> 
> Mathieu
> 
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com