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

[Alexander Fedorov <halcien@xxxxxxxxx>]

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);
 }
 
 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;
 	}