Re: [PATCH] memcg: Only free spare array when readers are done

[Martijn Coenen <maco@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

On Thu, Jan 14, 2016 at 3:38 PM, Michal Hocko <mhocko@kernel.org> wrote:

> On Thu 14-01-16 14:33:52, Martijn Coenen wrote:
> > A spare array holding mem cgroup threshold events is kept around
> > to make sure we can always safely deregister an event and have an
> > array to store the new set of events in.
> >
> > In the scenario where we're going from 1 to 0 registered events, the
> > pointer to the primary array containing 1 event is copied to the spare
> > slot, and then the spare slot is freed because no events are left.
> > However, it is freed before calling synchronize_rcu(), which means
> > readers may still be accessing threshold->primary after it is freed.
>
> Have you seen this triggering in the real life?
>

It was pretty easy to reproduce in a stress test setup, where we spawn a
process, put it in a mem cgroup and setup the threshold, have it allocate a
lot of memory quickly (crossing the threshold), unregister the event, kill
and repeat. Usually within 30 mins.

>
> >
> > Fixed by only freeing after synchronize_rcu().
> >
>
> Fixes: 8c7577637ca3 ("memcg: free spare array to avoid memory leak")
> > Signed-off-by: Martijn Coenen <maco@google.com>
> Cc: stable
>
> Acked-by: Michal Hocko <mhocko@suse.com>
>
> Thanks!
>
> > ---
> >  mm/memcontrol.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 14cb1db..73228b6 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -3522,16 +3522,17 @@ static void
> > __mem_cgroup_usage_unregister_event(struct mem_cgroup *memcg,
> >  swap_buffers:
> >       /* Swap primary and spare array */
> >       thresholds->spare = thresholds->primary;
> > -     /* If all events are unregistered, free the spare array */
> > -     if (!new) {
> > -             kfree(thresholds->spare);
> > -             thresholds->spare = NULL;
> > -     }
> >
> >       rcu_assign_pointer(thresholds->primary, new);
> >
> >       /* To be sure that nobody uses thresholds */
> >       synchronize_rcu();
> > +
> > +     /* If all events are unregistered, free the spare array */
> > +     if (!new) {
> > +             kfree(thresholds->spare);
> > +             thresholds->spare = NULL;
> > +     }
> >  unlock:
> >       mutex_unlock(&memcg->thresholds_lock);
> >  }
> > --
> > 2.6.0.rc2.230.g3dd15c0
>
> --
> Michal Hocko
> SUSE Labs
>

[-- Attachment #2: Type: text/html, Size: 3501 bytes --]