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@xxxxxxxxx>
> 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@xxxxxxxxx>
>
> 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
