Re: Possible race in obj_stock_flush_required() vs drain_obj_stock()

[Michal Hocko <mhocko@suse.com>]

On Mon 03-10-22 15:47:10, Alexander Fedorov wrote:
> On 02.10.2022 19:16, Roman Gushchin wrote:
> > On Sat, Oct 01, 2022 at 03:38:43PM +0300, Alexander Fedorov wrote:
> >> Tested READ_ONCE() patch and it works.
> > 
> > Thank you!
> > 
> >> But are rcu primitives an overkill?
> >> For me they are documenting how actually complex is synchronization here.
> > 
> > I agree, however rcu primitives will add unnecessary barriers on hot paths.
> > In this particular case most accesses to stock->cached_objcg are done from
> > a local cpu, so no rcu primitives are needed. So in my opinion using a
> > READ_ONCE() is preferred.
> 
> Understood, then here is patch that besides READ_ONCE() also fixes mentioned
> use-after-free that exists in 5.10.  In mainline the drain_obj_stock() part
> of the patch should be skipped.
> 
> Should probably be also Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> but I am not sure if I have rights to add that :)
> 
> 
>     mm/memcg: fix race in obj_stock_flush_required() vs drain_obj_stock()
>     
>     When obj_stock_flush_required() is called from drain_all_stock() it
>     reads the `memcg_stock->cached_objcg` field twice for another CPU's
>     per-cpu variable, leading to TOCTTOU race: another CPU can
>     simultaneously enter drain_obj_stock() and clear its own instance of
>     `memcg_stock->cached_objcg`.
>     
>     Another problem is in drain_obj_stock() which sets `cached_objcg` to
>     NULL after freeing which might lead to use-after-free.
>     
>     To fix it use READ_ONCE() for TOCTTOU problem and also clear the
>     `cached_objcg` pointer earlier in drain_obj_stock() for use-after-free
>     problem.
> 
> Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API")
> Signed-off-by: Alexander Fedorov <halcien@gmail.com>
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c1152f8747..56bd5ea6d3 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3197,17 +3197,30 @@ static void drain_obj_stock(struct memcg_stock_pcp *stock)
>  		stock->nr_bytes = 0;
>  	}
>  
> -	obj_cgroup_put(old);
> +	/*
> +	 * Clear pointer before freeing memory so that
> +	 * drain_all_stock() -> obj_stock_flush_required()
> +	 * does not see a freed pointer.
> +	 */
>  	stock->cached_objcg = NULL;
> +	obj_cgroup_put(old);

Do we need barrier() or something else to ensure there is no reordering?
I am not reallyu sure what kind of barriers are implied by the pcp ref
counting.

-- 
Michal Hocko
SUSE Labs