Re: [PATCH 2/5] mm/slub: restrict sysfs validation to debug caches and make it safe

[Vlastimil Babka <vbabka@suse.cz>]

On 8/14/22 16:39, Hyeonggon Yoo wrote:
> On Fri, Aug 12, 2022 at 11:14:23AM +0200, Vlastimil Babka wrote:
>> Rongwei Wang reports [1] that cache validation triggered by writing to
>> /sys/kernel/slab/<cache>/validate is racy against normal cache
>> operations (e.g. freeing) in a way that can cause false positive
>> inconsistency reports for caches with debugging enabled. The problem is
>> that debugging actions that mark object free or active and actual
>> freelist operations are not atomic, and the validation can see an
>> inconsistent state.
>> 
>> For caches that do or don't have debugging enabled, additional races
>> involving n->nr_slabs are possible that result in false reports of wrong
>> slab counts.
>> 
>> This patch attempts to solve these issues while not adding overhead to
>> normal (especially fastpath) operations for caches that do not have
>> debugging enabled. Such overhead would not be justified to make possible
>> userspace-triggered validation safe. Instead, disable the validation for
>> caches that don't have debugging enabled and make their sysfs validate
>> handler return -EINVAL.
>> 
>> For caches that do have debugging enabled, we can instead extend the
>> existing approach of not using percpu freelists to force all alloc/free
>> perations to the slow paths where debugging flags is checked and acted
>> upon. There can adjust the debug-specific paths to increase n->list_lock
>> coverage against concurrent validation as necessary.
> 
> s/perations/operations

OK

>> @@ -1604,9 +1601,9 @@ static inline
>>  void setup_slab_debug(struct kmem_cache *s, struct slab *slab, void *addr) {}
>>  
>>  static inline int alloc_debug_processing(struct kmem_cache *s,
>> -	struct slab *slab, void *object, unsigned long addr) { return 0; }
>> +	struct slab *slab, void *object) { return 0; }
>>  
>> -static inline int free_debug_processing(
>> +static inline void free_debug_processing(
>>  	struct kmem_cache *s, struct slab *slab,
>>  	void *head, void *tail, int bulk_cnt,
>>  	unsigned long addr) { return 0; }
> 
> IIRC As reported by bot on earlier patch, void function
> should not return 0;

OK

>> +/*
>> + * Called only for kmem_cache_debug() caches to allocate from a freshly
>> + * allocated slab. Allocate a single object instead of whole freelist
>> + * and put the slab to the partial (or full) list.
>> + */
>> +static void *alloc_single_from_new_slab(struct kmem_cache *s,
>> +					struct slab *slab)
>> +{
>> +	int nid = slab_nid(slab);
>> +	struct kmem_cache_node *n = get_node(s, nid);
>> +	unsigned long flags;
>> +	void *object;
>> +
>> +	spin_lock_irqsave(&n->list_lock, flags);
>> +
>> +	object = slab->freelist;
>> +	slab->freelist = get_freepointer(s, object);
>> +	slab->inuse = 1;
>> +
>> +	if (!alloc_debug_processing(s, slab, object)) {
>> +		/*
>> +		 * It's not really expected that this would fail on a
>> +		 * freshly allocated slab, but a concurrent memory
>> +		 * corruption in theory could cause that.
>> +		 */
>> +		spin_unlock_irqrestore(&n->list_lock, flags);
>> +		return NULL;
>> +	}
>> +
> 
> Nit: spin_lock_irqsave() can be here as freshly allocated
> slab has no other reference.

Right.

>> +out:
>> +	if (checks_ok) {
>> +		void *prior = slab->freelist;
>> +
>> +		/* Perform the actual freeing while we still hold the locks */
>> +		slab->inuse -= cnt;
>> +		set_freepointer(s, tail, prior);
>> +		slab->freelist = head;
>> +
>> +		/* Do we need to remove the slab from full or partial list? */
>> +		if (!prior) {
>> +			remove_full(s, n, slab);
>> +		} else if (slab->inuse == 0) {
>> +			remove_partial(n, slab);
>> +			stat(s, FREE_REMOVE_PARTIAL);
>> +		}
>> +
>> +		/* Do we need to discard the slab or add to partial list? */
>> +		if (slab->inuse == 0) {
>> +			slab_free = slab;
>> +		} else if (!prior) {
>> +			add_partial(n, slab, DEACTIVATE_TO_TAIL);
>> +			stat(s, FREE_ADD_PARTIAL);
>> +		}
>> +	}
>> +
>> +	if (slab_free) {
>> +		/*
>> +		 * Update the counters while still holding n->list_lock to
>> +		 * prevent spurious validation warnings
>> +		 */
>> +		dec_slabs_node(s, slab_nid(slab_free), slab_free->objects);
>> +	}
> 
> This looks good but maybe kmem_cache_shrink() can lead to
> spurious validation warnings?

Good catch, I'll fix that too.

> 
> Otherwise looks good to me!
> 

Thanks!