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

[Alexander Fedorov <halcien@gmail.com>]

On 03.10.2022 17:27, Michal Hocko wrote:
> On Mon 03-10-22 17:09:15, Alexander Fedorov wrote:
>> On 03.10.2022 16:32, Michal Hocko wrote:
>>> On Mon 03-10-22 15:47:10, Alexander Fedorov wrote:
>>>> @@ -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.
>>
>> obj_cgroup_put() -> kfree_rcu() -> synchronize_rcu() should take care
>> of this:
> 
> This is a very subtle guarantee. Also it would only apply if this is the
> last reference, right?

Hmm, yes, for the last reference only, also not sure about pcp ref
counter ordering rules for previous references.

> Is there any reason to not use
> 	WRITE_ONCE(stock->cached_objcg, NULL);
> 	obj_cgroup_put(old);
> 
> IIRC this should prevent any reordering. 

Now that I think about it we actually must use WRITE_ONCE everywhere
when writing cached_objcg because otherwise compiler might split the
pointer-sized store into several smaller-sized ones (store tearing),
and obj_stock_flush_required() would read garbage instead of pointer.

And thinking about memory barriers, maybe we need them too alongside
WRITE_ONCE when setting pointer to non-null value?  Otherwise
drain_all_stock() -> obj_stock_flush_required() might read old data.
Since that's exactly what rcu_assign_pointer() does, it seems
that we are going back to using rcu_*() primitives everywhere?