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

[Roman Gushchin <roman.gushchin@linux.dev>]

On Fri, Sep 30, 2022 at 02:06:48PM +0000, Alexander Fedorov wrote:
> Hi,
> 
> reposting this to the mainline list as requested and with updated patch.
> 
> I've encountered a race on kernel version 5.10.131-rt72 when running
> LTP cgroup_fj_stress_memory* tests and need help with understanding
> synchronization in mm/memcontrol.c, it seems really not-trivial...
> Have also checked patches in the latest mainline kernel but do not see
> anything similar to the problem.  Realtime patch also does not seem to
> be the culprit: it just changed preemption to migration disabling and
> irq_disable to local_lock.
> 
> It goes as follows:
> 
> 1) First CPU:
>    css_killed_work_fn() -> mem_cgroup_css_offline() ->
> drain_all_stock() -> obj_stock_flush_required()
>         if (stock->cached_objcg) {
> 
> This check sees a non-NULL pointer for *another* CPU's `memcg_stock`
> instance.
> 
> 2) Second CPU:
>   css_free_rwork_fn() -> __mem_cgroup_free() -> free_percpu() ->
> obj_cgroup_uncharge() -> drain_obj_stock()
> It frees `cached_objcg` pointer in its own `memcg_stock` instance:
>         struct obj_cgroup *old = stock->cached_objcg;
>         < ... >
>         obj_cgroup_put(old);
>         stock->cached_objcg = NULL;
> 
> 3) First CPU continues after the 'if' check and re-reads the pointer
> again, now it is NULL and dereferencing it leads to kernel panic:
> static bool obj_stock_flush_required(struct memcg_stock_pcp *stock,
>                                      struct mem_cgroup *root_memcg)
> {
> < ... >
>         if (stock->cached_objcg) {
>                 memcg = obj_cgroup_memcg(stock->cached_objcg);

Great catch!

I'm not sure about switching to rcu primitives though. In all other cases
stock->cached_objcg is accessed only from a local cpu, so using rcu_*
function is an overkill.

How's something about this? (completely untested)

Also, please add
Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API")

Thank you!

--

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b69979c9ced5..93e9637108f0 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3245,10 +3245,18 @@ static struct obj_cgroup *drain_obj_stock(struct memcg_stock_pcp *stock)
 static bool obj_stock_flush_required(struct memcg_stock_pcp *stock,
                                     struct mem_cgroup *root_memcg)
 {
+       struct obj_cgroup *objcg;
        struct mem_cgroup *memcg;

-       if (stock->cached_objcg) {
-               memcg = obj_cgroup_memcg(stock->cached_objcg);
+       /*
+        * stock->cached_objcg can be changed asynchronously, so read
+        * it using READ_ONCE(). The objcg can't go away though because
+        * obj_stock_flush_required() is called from within a rcu read
+        * section.
+        */
+       objcg = READ_ONCE(stock->cached_objcg);
+       if (objcg) {
+               memcg = obj_cgroup_memcg(objcg);
                if (memcg && mem_cgroup_is_descendant(memcg, root_memcg))
                        return true;
        }

Thank you!