Re: A racy reading spot on n->free_objects in slab.c

["Gong, Sishuai" <sishuai@purdue.edu>]

> On Apr 14, 2021, at 7:06 AM, Vlastimil Babka <vbabka@suse.cz> wrote:
> 
> On 4/14/21 9:32 AM, Christoph Lameter wrote:
>> On Tue, 13 Apr 2021, Gong, Sishuai wrote:
>> 
>>> We found a racy reading spot on shared variable n->free_objects in
> 
> I'm assuming this was found with some research tool you're developing?
> Did it also flag the line "shared = READ_ONCE(n->shared);" as that's basically
> the same thing.
Yes, the read of n->shared is also racy but we didn’t observe any data races on it. 

We saw cache_alloc_refill() could run in parallel with multi writers (free_block(), cache_grow_end() and even itself cache_alloc_refill()), but none of these writers would change n->shared.
> 
>>> slab.c and it can be data-racing with several writers that update this
>>> variable. As shown below, in function cache_alloc_refill(),
>>> n->free_objects will be read without any protection. It could be
>>> possible that the read value immediately becomes out-of-date when
>>> another writer is changing it (e.g. free_block())
>> 
>> Ok that is fine. If we mistakenly fill up the per cpu cache with new
>> objects to the slab then so be it.
> 
>> If we mistakenly take the lock and fail to get an object then we can still
>> reverse that decision and do the other thing.
> 
> Agreed. It's common in the kernel to do optimistic reads outside of lock to
> decide what to do and avoid locking at all in some cases. This may sacrifice
> some precision of these decisions. but not correctness, as locks are taken later
> for the critical parts.
> 
>> Maybe we need to add a comment there?
> 
> Or maybe some construct that makes no difference for the compiler, but does for
> the tool? READ_ONCE() maybe?