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

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

On Mon, Oct 03, 2022 at 06:01:35PM +0300, Alexander Fedorov wrote:
> 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?

Hm, Idk, I'm still somewhat resistant to the idea of putting rcu primitives,
but maybe it's the right thing. Maybe instead we should always schedule draining
on all cpus instead and perform a cpu-local check and bail out if a flush is not
required? Michal, Johannes, what do you think?

Btw the same approach is used for the memcg part (stock->cached),
so if we're going to change anything, we need to change it too.

Thanks!