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@xxxxxxxxxx/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@xxxxxxxxxx/ 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
