Re: [PATCH v2 2/2] mm/kasan/kunit: Add double-free tests involving kfree_rcu_sheaf().

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm/kasan: missing Kconfig guards in __kmem_cache_double_free_rcu()
- [Medium] mm/kasan/kunit: CPU and NUMA migration race in __kmem_cache_double_free_rcu()
--

commit 2267f8620b312bfdd7e76e11214e3ba726322685
Author: Kuniyuki Iwashima <kuniyu@google.com>

mm/kasan/kunit: Add double-free tests involving kfree_rcu_sheaf().

This commit adds KUnit test cases to verify that KASAN immediately detects
double-free combinations involving kfree_rcu_sheaf() and prevents a
reallocated victim object from being poisoned when the RCU sheaf is flushed.

> diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
> index b9e167ed5be32..83c418c0ef878 100644
> --- a/mm/kasan/kasan_test_c.c
> +++ b/mm/kasan/kasan_test_c.c
[ ... ]
> @@ -1070,6 +1071,134 @@ static void kmem_cache_double_free(struct kunit *test)
>  	kmem_cache_destroy(cache);
>  }
>  
> +#ifndef MODULE
> +static void __kmem_cache_double_free_rcu(struct kunit *test, int mode)
> +{
> +	struct kasan_rcu_info *p, *victim = NULL, **objs;
> +	struct kmem_cache *cache;
> +	int i, n = 0;
> +
> +	KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_GENERIC);
> +
> +	cache = kmem_cache_create("test_cache", sizeof(*p), 0, SLAB_NO_MERGE,
> +				  NULL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, cache);
> +
> +	if (!cache->sheaf_capacity) {
> +		kmem_cache_destroy(cache);
> +		kunit_skip(test, "Test requires sheaves");
> +	}

[Severity: Medium]
Should __kmem_cache_double_free_rcu() also check that
CONFIG_KVFREE_RCU_BATCHED is enabled and CONFIG_PREEMPT_RT is disabled?

In calculate_sheaf_capacity(), cache->sheaf_capacity is set to a non-zero
value whenever CONFIG_SLUB_TINY is disabled and debug flags are not set on
the cache, regardless of CONFIG_PREEMPT_RT or CONFIG_KVFREE_RCU_BATCHED.
However, kvfree_call_rcu() in mm/slab_common.c only calls kfree_rcu_sheaf()
when CONFIG_KVFREE_RCU_BATCHED is enabled and CONFIG_PREEMPT_RT is disabled:

mm/slab_common.c:
#ifndef CONFIG_KVFREE_RCU_BATCHED
void kvfree_call_rcu(struct kvfree_rcu_head *head, void *ptr)
{
	if (head) {
		kasan_record_aux_stack(ptr);
		call_rcu(&head->head, kvfree_rcu_cb);
		return;
	}
...
#endif

and in the batched implementation:

mm/slab_common.c:kvfree_call_rcu() {
	...
	if (!IS_ENABLED(CONFIG_PREEMPT_RT) && kfree_rcu_sheaf(ptr))
		return;
	...
}

When CONFIG_PREEMPT_RT=y or CONFIG_KVFREE_RCU_BATCHED=n (for example with
CONFIG_RCU_STRICT_GRACE_PERIOD=y or CONFIG_TINY_RCU=y), cache->sheaf_capacity
is non-zero so the test does not skip, but kfree_rcu(p, rcu) bypasses
__kfree_rcu_sheaf() where kasan_slab_pre_free() and
kasan_mark_rcu_free_pending() are called.

Wouldn't this cause KUNIT_EXPECT_KASAN_FAIL() to fail across all three modes,
free and poison the reallocated live victim object during
kvfree_rcu_barrier_on_cache(cache) in modes 1 and 2, and queue the same
p->rcu head twice in mode 3?

Adding KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KVFREE_RCU_BATCHED) and
KASAN_TEST_NEEDS_CONFIG_OFF(test, CONFIG_PREEMPT_RT) would skip the test on
configurations where kfree_rcu_sheaf() is not used.

> +
> +	objs = kunit_kcalloc(test, cache->sheaf_capacity * 2, sizeof(*objs),
> +			     GFP_KERNEL);
> +	if (!objs) {
> +		kunit_err(test, "Allocation failed: %s\n", __func__);
> +		goto out;
> +	}
> +
> +	p = kmem_cache_alloc(cache, GFP_KERNEL);
> +	if (!p) {
> +		kunit_err(test, "Allocation failed: %s\n", __func__);
> +		goto out;
> +	}
> +
> +	switch (mode) {
> +	case 1:
> +		kmem_cache_free(cache, p);
> +		/* p is already in quarantine. */
> +		KUNIT_EXPECT_KASAN_FAIL(test, kfree_rcu(p, rcu));
> +		break;
> +	case 2:
> +		/* The actual free order is inverse. */
> +		kfree_rcu(p, rcu);
> +		KUNIT_EXPECT_KASAN_FAIL(test, kmem_cache_free(cache, p));
> +		break;
> +	case 3:
> +		migrate_disable();
> +		kfree_rcu(p, rcu);
> +
> +		for (i = 0; i < cache->sheaf_capacity - 1; i++) {
> +			objs[i] = kmem_cache_alloc(cache, GFP_KERNEL);
> +			kfree_rcu(objs[i], rcu);
> +		}
> +
> +		KUNIT_EXPECT_KASAN_FAIL(test, kfree_rcu(p, rcu));
> +		migrate_enable();
> +
> +		/*
> +		 * Move the 1st full sheaf to quarantine, keeping the
> +		 * 2nd in pcs->rcu_free.
> +		 */
> +		rcu_barrier();
> +		break;
> +	}
> +
> +	/*
> +	 * Move p to quarantine if not poisoned yet (case 2,
> +	 * or fallback in case 3).
> +	 */
> +	if (kasan_byte_accessible(p))
> +		kvfree_rcu_barrier_on_cache(cache);
> +
> +	/*
> +	 * Evict p from quarantine and reallocate it as a live victim object
> +	 * before the RCU sheaf is flushed.
> +	 */
> +	kasan_quarantine_remove_cache(cache);
> +
> +	for (i = 0; i < cache->sheaf_capacity * 2; i++) {
> +		objs[n] = kmem_cache_alloc(cache, GFP_KERNEL);
> +		if (!objs[n])
> +			break;
> +		if (objs[n] == p) {
> +			victim = objs[n];
> +			break;
> +		}
> +		n++;
> +	}
> +
> +	for (i = 0; i < n; i++)
> +		kmem_cache_free(cache, objs[i]);
> +
> +	KUNIT_EXPECT_PTR_EQ(test, victim, p);

[Severity: Medium]
Can task migration across CPUs or NUMA nodes cause spurious failures in
__kmem_cache_double_free_rcu()?

Because kunit_try_catch_run() executes the test in an unbound kthread, the
task can migrate whenever migration is not disabled:

1. p = kmem_cache_alloc(cache, GFP_KERNEL) runs before migrate_disable() in
mode 3, and modes 1 and 2 do not call migrate_disable() at all. In
kfree_rcu_sheaf() in mm/slab_common.c:

	s = slab->slab_cache;
	if (likely(!IS_ENABLED(CONFIG_NUMA) || slab_nid(slab) == numa_mem_id()))
		return __kfree_rcu_sheaf(s, obj, SLAB_FREE_DEFAULT);

	return false;

If the task migrates to a CPU on a different NUMA node between allocating p
and calling kfree_rcu(p, rcu), slab_nid(slab) == numa_mem_id() is false and
kfree_rcu_sheaf() returns false, falling back to the non-sheaf
kvfree_call_rcu() path. This bypasses __kfree_rcu_sheaf(), causing
KUNIT_EXPECT_KASAN_FAIL() to fail and queueing the double-freed object into
krcp.

2. Following the switch block, rcu_barrier(),
kvfree_rcu_barrier_on_cache(cache), and kasan_quarantine_remove_cache(cache)
can sleep with migration enabled, and the victim reallocation loop also runs
without migrate_disable(). If the task migrates to another NUMA node before
or during the loop, refill_objects() only refills from local_node while
__refill_objects_any() skips p's node because n->nr_partial <= s->min_partial,
allocating a new slab instead of p. Even on a single NUMA node, if the task
migrates to another CPU mid-loop after refill_sheaf() moves p into the first
CPU's pcs->main->objects[0], p remains in the first CPU's per-CPU sheaf and
KUNIT_EXPECT_PTR_EQ(test, victim, p) fails.

Would it make sense to wrap the initial allocation and free sequence in
migrate_disable() / migrate_enable() for all modes, and either keep the task
on the same CPU/node or use kmem_cache_alloc_node(..., folio_nid(virt_to_folio(p)))
under migrate_disable() when reallocating victim?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027222557.1072105-1-kuniyu@google.com?part=2