[PATCH] mm,slub: do not call do_slab_free for kfence object

[PATCH] mm,slub: do not call do_slab_free for kfence object

[Rik van Riel]

In 782f8906f805 the freeing of kfence objects was moved from deep
inside do_free_slab to the wrapper functions outside. This is a nice
change, but unfortunately it missed one spot in __kmem_cache_free_bulk.

This results in a crash like this:

BUG skbuff_head_cache (Tainted: G S  B       E     ): Padding overwritten. 0xffff88907fea0f00-0xffff88907fea0fff @offset=3840

slab_err (mm/slub.c:1129)
free_to_partial_list (mm/slub.c:? mm/slub.c:4036)
slab_pad_check (mm/slub.c:864 mm/slub.c:1290)
check_slab (mm/slub.c:?)
free_to_partial_list (mm/slub.c:3171 mm/slub.c:4036)
kmem_cache_alloc_bulk (mm/slub.c:? mm/slub.c:4495 mm/slub.c:4586 mm/slub.c:4635)
napi_build_skb (net/core/skbuff.c:348 net/core/skbuff.c:527 net/core/skbuff.c:549)

All the other callers to do_free_slab appear to be ok.

Add a kfence_free check in __kmem_cache_free_bulk to avoid the crash.

Reported-by: Chris Mason <clm@meta.com>
Fixes: 782f8906f805 ("mm/slub: free KFENCE objects in slab_free_hook()")
Cc: stable@kernel.org
Signed-off-by: Rik van Riel <riel@surriel.com>
---
 mm/slub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/slub.c b/mm/slub.c
index 3520acaf9afa..c9d8a2497fd6 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4690,6 +4690,9 @@ static void __kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p)
 		if (!df.slab)
 			continue;
 
+		if (kfence_free(df.freelist))
+			continue;
+
 		do_slab_free(df.s, df.slab, df.freelist, df.tail, df.cnt,
 			     _RET_IP_);
 	} while (likely(size));
-- 
2.45.2

Re: [PATCH] mm,slub: do not call do_slab_free for kfence object

[Chris Mason]

On 7/29/24 2:19 PM, Rik van Riel wrote:
> In 782f8906f805 the freeing of kfence objects was moved from deep
> inside do_free_slab to the wrapper functions outside. This is a nice
> change, but unfortunately it missed one spot in __kmem_cache_free_bulk.
> 
> This results in a crash like this:
> 
> BUG skbuff_head_cache (Tainted: G S  B       E     ): Padding overwritten. 0xffff88907fea0f00-0xffff88907fea0fff @offset=3840
> 
> slab_err (mm/slub.c:1129)
> free_to_partial_list (mm/slub.c:? mm/slub.c:4036)
> slab_pad_check (mm/slub.c:864 mm/slub.c:1290)
> check_slab (mm/slub.c:?)
> free_to_partial_list (mm/slub.c:3171 mm/slub.c:4036)
> kmem_cache_alloc_bulk (mm/slub.c:? mm/slub.c:4495 mm/slub.c:4586 mm/slub.c:4635)
> napi_build_skb (net/core/skbuff.c:348 net/core/skbuff.c:527 net/core/skbuff.c:549)
> 
> All the other callers to do_free_slab appear to be ok.
> 
> Add a kfence_free check in __kmem_cache_free_bulk to avoid the crash.
> 
> Reported-by: Chris Mason <clm@meta.com>
> Fixes: 782f8906f805 ("mm/slub: free KFENCE objects in slab_free_hook()")
> Cc: stable@kernel.org
> Signed-off-by: Rik van Riel <riel@surriel.com>

We found this after bisecting a slab corruption down to the kfence
patch, and with this patch applied we're no longer falling over.  So
thanks Rik!

-chris

Re: [PATCH] mm,slub: do not call do_slab_free for kfence object

[Vlastimil Babka]

On 7/29/24 8:46 PM, Chris Mason wrote:
> 
> 
> On 7/29/24 2:19 PM, Rik van Riel wrote:
>> In 782f8906f805 the freeing of kfence objects was moved from deep
>> inside do_free_slab to the wrapper functions outside. This is a nice
>> change, but unfortunately it missed one spot in __kmem_cache_free_bulk.
>>
>> This results in a crash like this:
>>
>> BUG skbuff_head_cache (Tainted: G S  B       E     ): Padding overwritten. 0xffff88907fea0f00-0xffff88907fea0fff @offset=3840
>>
>> slab_err (mm/slub.c:1129)
>> free_to_partial_list (mm/slub.c:? mm/slub.c:4036)
>> slab_pad_check (mm/slub.c:864 mm/slub.c:1290)
>> check_slab (mm/slub.c:?)
>> free_to_partial_list (mm/slub.c:3171 mm/slub.c:4036)
>> kmem_cache_alloc_bulk (mm/slub.c:? mm/slub.c:4495 mm/slub.c:4586 mm/slub.c:4635)
>> napi_build_skb (net/core/skbuff.c:348 net/core/skbuff.c:527 net/core/skbuff.c:549)
>>
>> All the other callers to do_free_slab appear to be ok.

changed do_free_slab to do_slab_free in two places.

>>
>> Add a kfence_free check in __kmem_cache_free_bulk to avoid the crash.
>>
>> Reported-by: Chris Mason <clm@meta.com>
>> Fixes: 782f8906f805 ("mm/slub: free KFENCE objects in slab_free_hook()")
>> Cc: stable@kernel.org
>> Signed-off-by: Rik van Riel <riel@surriel.com>
> 
> We found this after bisecting a slab corruption down to the kfence
> patch, and with this patch applied we're no longer falling over.  So
> thanks Rik!

Indeed thanks and sorry for the trouble! Given that
__kmem_cache_free_bulk is currently only used to unwind a
kmem_cache_bulk_alloc() that runs out of memory in the middle of the
operation, I'm surprised you saw this happen reliably enough to bisect it.

Added to slab/for-6.11-rc1/fixes


> -chris

Re: [PATCH] mm,slub: do not call do_slab_free for kfence object

[Chris Mason]

On 7/30/24 6:01 AM, Vlastimil Babka wrote:
> On 7/29/24 8:46 PM, Chris Mason wrote:
>>
>>
>> On 7/29/24 2:19 PM, Rik van Riel wrote:
>>> Reported-by: Chris Mason <clm@meta.com>
>>> Fixes: 782f8906f805 ("mm/slub: free KFENCE objects in slab_free_hook()")
>>> Cc: stable@kernel.org
>>> Signed-off-by: Rik van Riel <riel@surriel.com>
>>
>> We found this after bisecting a slab corruption down to the kfence
>> patch, and with this patch applied we're no longer falling over.  So
>> thanks Rik!
> 
> Indeed thanks and sorry for the trouble! Given that
> __kmem_cache_free_bulk is currently only used to unwind a
> kmem_cache_bulk_alloc() that runs out of memory in the middle of the
> operation, I'm surprised you saw this happen reliably enough to bisect it.
> 
The repro was just forcing two sequential OOMs during iperf load on top
of mlx5 ethernet:

Test machine:
- iperf -s -V

Load generator:
- iperf -c test_machine -P 10 -w 1k -l 1k -V --time 900

Test machine:
- hog all memory until OOM
- Do it one more time

Since we didn't have memory corruptions on other nics, I was pretty sure
the bisect had gone wrong when all the remaining commits were in MM.
Nothing against our friends in networking, but MM bugs are usually
easier to fix, so I was pretty happy after confirming kfence as the cause.

> Added to slab/for-6.11-rc1/fixes

Thanks!

-chris