Re: [PATCH 2/2] stackdepot: make fast paths lock-less again

[Marco Elver <elver@google.com>]

On Sat, Feb 24, 2024 at 07:03PM +0100, Marco Elver wrote:
[...]
> 
> stackdepot users who do not use STACK_DEPOT_FLAG_GET must never call
> stack_depot_put() on such entries.
> 
> Violation of this contract will lead to UAF errors.
> 
> From the report I see this is a KMSAN error. There is a high chance
> this is a false positive. Have you tried it with this patch:
> https://lore.kernel.org/all/20240124173134.1165747-1-glider@google.com/T/#u
	^ [2]

I see what's going on now. The series [1] (+ the kmsan fix above [2])
that's in -next that brings back variable-sized records fixes it.

[1] https://lore.kernel.org/all/20240129100708.39460-1-elver@google.com/

The reason [2] alone on top of mainline doesn't fix it is because
stackdepot in mainline still pre-populates the freelist, and then does
depot_pop_free(), which in turn calls list_del() to remove from the
freelist. However, the stackdepot "pool" is never unpoisoned by KMSAN
before depot_pop_free() (remember that KMSAN uses stackdepot itself, so
we have to be careful when to unpoison stackdepot memory), and we see
the KMSAN false positive report.

Only after the entry has already been removed from the freelist is
kmsan_unpoison_memory(stack, ...) called to unpoison the entry (which is
too late).

Therefore, the bug you've observed is a KMSAN false positive. This diff
confirms the issue:

diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index 5caa1f566553..3c18aad3f833 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -423,6 +423,7 @@ static struct stack_record *depot_pop_free(void)
 	if (stack->size && !poll_state_synchronize_rcu(stack->rcu_state))
 		return NULL;
 
+	kmsan_unpoison_memory(stack, DEPOT_STACK_RECORD_SIZE);
 	list_del(&stack->free_list);
 	counters[DEPOT_COUNTER_FREELIST_SIZE]--;
 
@@ -467,7 +468,7 @@ depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
 	 * Let KMSAN know the stored stack record is initialized. This shall
 	 * prevent false positive reports if instrumented code accesses it.
 	 */
-	kmsan_unpoison_memory(stack, DEPOT_STACK_RECORD_SIZE);
+	//kmsan_unpoison_memory(stack, DEPOT_STACK_RECORD_SIZE);
 
 	counters[DEPOT_COUNTER_ALLOCS]++;
 	counters[DEPOT_COUNTER_INUSE]++;

But that's not a good fix. Besides reducing KMSAN memory usage back to
v6.7 levels, the series [1] completely removes pre-populating the
freelist and entries are only ever inserted into the freelist when they
are actually "evicted", i.e. after kmsan_unpoison_memory() has been
called in depot_alloc_stack, and any subsequent list_del() actually
operates on unpoisoned memory.

If we want this fixed in mainline, I propose that [1] + [2] are sent for
6.8-rc inclusion.

Thanks,
-- Marco