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

[Michal Hocko]

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?

> 
> Fixed by only freeing after synchronize_rcu().
> 

Fixes: 8c7577637ca3 ("memcg: free spare array to avoid memory leak")
> Signed-off-by: Martijn Coenen <maco@xxxxxxxxxx>
Cc: stable

Acked-by: Michal Hocko <mhocko@xxxxxxxx>

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