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

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

On Wed, Oct 12, 2022 at 01:23:11PM -0400, Johannes Weiner wrote:
> On Tue, Oct 04, 2022 at 09:18:26AM -0700, Roman Gushchin wrote:
> > 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?
> 
> I agree it's overkill.
> 
> This is a speculative check, and we don't need any state coherency,
> just basic lifetime. READ_ONCE should fully address this problem. That
> said, I think the code could be a bit clearer and better documented.
> 
> How about the below?

I'm fine with using READ_ONCE() to fix this immediate issue (I suggested it
in the thread above), please feel free to add my ack:
Acked-by: Roman Gushchin <roman.gushchin@linux.dev> .

We might need a barrier() between zeroing stock->cached and dropping the last
reference, as discussed above, however I don't think this issue can be
realistically trgiggered in the real life.

However I think our overall approach to flushing is questionable:
1) we often don't flush when it's necessary: if there is a concurrent flushing
we just bail out, even if that flushing is related to a completely different
part of the cgroup tree (e.g. a leaf node belonging to a distant branch).
2) we can race and flush when it's not necessarily: if another cpu is busy,
likely by the time when work will be executed there will be already another
memcg cached. So IMO we need to move this check into the flushing thread.

I'm working on a different approach, but it will take time and also likely be
too invasive for @stable, so fixing the crash discovered by Alexander with
READ_ONCE() is a good idea.

Thanks!