linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] slab: clear OBJEXTS_ALLOC_FAIL when freeing a slab
@ 2025-10-15 12:59 Hao Ge
  2025-10-15 13:11 ` Vlastimil Babka
  2025-10-15 18:47 ` [syzbot ci] " syzbot ci
  0 siblings, 2 replies; 4+ messages in thread
From: Hao Ge @ 2025-10-15 12:59 UTC (permalink / raw)
  To: Vlastimil Babka, Andrew Morton, Christoph Lameter,
	David Rientjes, Roman Gushchin, Harry Yoo
  Cc: Alexei Starovoitov, Shakeel Butt, Suren Baghdasaryan, linux-mm,
	linux-kernel, Hao Ge

From: Hao Ge <gehao@kylinos.cn>

If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL,
But we did not clear it when freeing the slab. Since OBJEXTS_ALLOC_FAIL and
MEMCG_DATA_OBJEXTS currently share the same bit position, during the
release of the associated folio, a VM_BUG_ON_FOLIO() check in
folio_memcg_kmem() is triggered because it was mistakenly assumed that
a valid folio->memcg_data was not cleared before freeing the folio.

When freeing a slab, we clear slab->obj_exts if the obj_ext array has been
successfully allocated. So let's clear OBJEXTS_ALLOC_FAIL when freeing
a slab if the obj_ext array allocated fail to allow them to be returned
to the buddy system more smoothly.

Fixes: 7612833192d5 ("slab: Reuse first bit for OBJEXTS_ALLOC_FAIL")
Suggested-by: Harry Yoo <harry.yoo@oracle.com>
Signed-off-by: Hao Ge <gehao@kylinos.cn>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
---
v4: Based on the discussion between Vlastimil and Harry,
    modify the solution to clear OBJEXTS_ALLOC_FAIL when freeing a slab.
    This does seem more reasonable. Thank you both.
---
 mm/slab.h | 26 ++++++++++++++++++++++++++
 mm/slub.c |  6 ++++++
 2 files changed, 32 insertions(+)

diff --git a/mm/slab.h b/mm/slab.h
index 078daecc7cf5..52424d6871bd 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -547,6 +547,28 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 	return (struct slabobj_ext *)(obj_exts & ~OBJEXTS_FLAGS_MASK);
 }
 
+/*
+ * objexts_clear_alloc_fail -  Clear the OBJEXTS_ALLOC_FAIL for
+ * the slab object extension vector associated with a slab.
+ * @slab: a pointer to the slab struct
+ */
+static inline void objexts_clear_alloc_fail(struct slab *slab)
+{
+	unsigned long obj_exts = READ_ONCE(slab->obj_exts);
+
+#ifdef CONFIG_MEMCG
+	/*
+	 * obj_exts should be either NULL, a valid pointer with
+	 * MEMCG_DATA_OBJEXTS bit set or be equal to OBJEXTS_ALLOC_FAIL.
+	 */
+	VM_BUG_ON_PAGE(obj_exts && !(obj_exts & MEMCG_DATA_OBJEXTS) &&
+		       obj_exts != OBJEXTS_ALLOC_FAIL, slab_page(slab));
+	VM_BUG_ON_PAGE(obj_exts & MEMCG_DATA_KMEM, slab_page(slab));
+#endif
+
+	obj_exts &= ~OBJEXTS_ALLOC_FAIL;
+	WRITE_ONCE(slab->obj_exts, obj_exts);
+}
 int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
                         gfp_t gfp, bool new_slab);
 
@@ -557,6 +579,10 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 	return NULL;
 }
 
+static inline void objexts_clear_alloc_fail(struct slab *slab)
+{
+}
+
 #endif /* CONFIG_SLAB_OBJ_EXT */
 
 static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
diff --git a/mm/slub.c b/mm/slub.c
index b1f15598fbfd..80166a4a62f9 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2169,6 +2169,12 @@ static inline void free_slab_obj_exts(struct slab *slab)
 {
 	struct slabobj_ext *obj_exts;
 
+	/*
+	 * If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL,
+	 * Therefore, we should clear the OBJEXTS_ALLOC_FAIL flag first when freeing a slab.
+	 */
+	objexts_clear_alloc_fail(slab);
+
 	obj_exts = slab_obj_exts(slab);
 	if (!obj_exts)
 		return;
-- 
2.25.1



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] slab: clear OBJEXTS_ALLOC_FAIL when freeing a slab
  2025-10-15 12:59 [PATCH v4] slab: clear OBJEXTS_ALLOC_FAIL when freeing a slab Hao Ge
@ 2025-10-15 13:11 ` Vlastimil Babka
  2025-10-15 13:37   ` Hao Ge
  2025-10-15 18:47 ` [syzbot ci] " syzbot ci
  1 sibling, 1 reply; 4+ messages in thread
From: Vlastimil Babka @ 2025-10-15 13:11 UTC (permalink / raw)
  To: Hao Ge, Andrew Morton, Christoph Lameter, David Rientjes,
	Roman Gushchin, Harry Yoo
  Cc: Alexei Starovoitov, Shakeel Butt, Suren Baghdasaryan, linux-mm,
	linux-kernel, Hao Ge

On 10/15/25 14:59, Hao Ge wrote:
> From: Hao Ge <gehao@kylinos.cn>
> 
> If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL,
> But we did not clear it when freeing the slab. Since OBJEXTS_ALLOC_FAIL and
> MEMCG_DATA_OBJEXTS currently share the same bit position, during the
> release of the associated folio, a VM_BUG_ON_FOLIO() check in
> folio_memcg_kmem() is triggered because it was mistakenly assumed that
> a valid folio->memcg_data was not cleared before freeing the folio.
> 
> When freeing a slab, we clear slab->obj_exts if the obj_ext array has been
> successfully allocated. So let's clear OBJEXTS_ALLOC_FAIL when freeing
> a slab if the obj_ext array allocated fail to allow them to be returned
> to the buddy system more smoothly.

Thanks!

> Fixes: 7612833192d5 ("slab: Reuse first bit for OBJEXTS_ALLOC_FAIL")
> Suggested-by: Harry Yoo <harry.yoo@oracle.com>
> Signed-off-by: Hao Ge <gehao@kylinos.cn>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

Since we changed the approach completely, I think we should not carry over
the previously added review tags in this case.

> ---
> v4: Based on the discussion between Vlastimil and Harry,
>     modify the solution to clear OBJEXTS_ALLOC_FAIL when freeing a slab.
>     This does seem more reasonable. Thank you both.
> ---
>  mm/slab.h | 26 ++++++++++++++++++++++++++
>  mm/slub.c |  6 ++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/mm/slab.h b/mm/slab.h
> index 078daecc7cf5..52424d6871bd 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -547,6 +547,28 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
>  	return (struct slabobj_ext *)(obj_exts & ~OBJEXTS_FLAGS_MASK);
>  }
>  
> +/*
> + * objexts_clear_alloc_fail -  Clear the OBJEXTS_ALLOC_FAIL for
> + * the slab object extension vector associated with a slab.
> + * @slab: a pointer to the slab struct
> + */
> +static inline void objexts_clear_alloc_fail(struct slab *slab)
> +{
> +	unsigned long obj_exts = READ_ONCE(slab->obj_exts);
> +
> +#ifdef CONFIG_MEMCG
> +	/*
> +	 * obj_exts should be either NULL, a valid pointer with
> +	 * MEMCG_DATA_OBJEXTS bit set or be equal to OBJEXTS_ALLOC_FAIL.
> +	 */
> +	VM_BUG_ON_PAGE(obj_exts && !(obj_exts & MEMCG_DATA_OBJEXTS) &&
> +		       obj_exts != OBJEXTS_ALLOC_FAIL, slab_page(slab));
> +	VM_BUG_ON_PAGE(obj_exts & MEMCG_DATA_KMEM, slab_page(slab));
> +#endif
> +
> +	obj_exts &= ~OBJEXTS_ALLOC_FAIL;
> +	WRITE_ONCE(slab->obj_exts, obj_exts);
> +}

This is much larger than necessary I think. See below.

>  int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
>                          gfp_t gfp, bool new_slab);
>  
> @@ -557,6 +579,10 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
>  	return NULL;
>  }
>  
> +static inline void objexts_clear_alloc_fail(struct slab *slab)
> +{
> +}
> +
>  #endif /* CONFIG_SLAB_OBJ_EXT */
>  
>  static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
> diff --git a/mm/slub.c b/mm/slub.c
> index b1f15598fbfd..80166a4a62f9 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2169,6 +2169,12 @@ static inline void free_slab_obj_exts(struct slab *slab)
>  {
>  	struct slabobj_ext *obj_exts;
>  
> +	/*
> +	 * If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL,
> +	 * Therefore, we should clear the OBJEXTS_ALLOC_FAIL flag first when freeing a slab.
> +	 */
> +	objexts_clear_alloc_fail(slab);
> +
>  	obj_exts = slab_obj_exts(slab);
>  	if (!obj_exts)
>  		return;

It should be enough that this return path does "slab->obj_exts = 0;" no?




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] slab: clear OBJEXTS_ALLOC_FAIL when freeing a slab
  2025-10-15 13:11 ` Vlastimil Babka
@ 2025-10-15 13:37   ` Hao Ge
  0 siblings, 0 replies; 4+ messages in thread
From: Hao Ge @ 2025-10-15 13:37 UTC (permalink / raw)
  To: Vlastimil Babka, Andrew Morton, Christoph Lameter,
	David Rientjes, Roman Gushchin, Harry Yoo
  Cc: Alexei Starovoitov, Shakeel Butt, Suren Baghdasaryan, linux-mm,
	linux-kernel, Hao Ge

Hi Vlastimil


On 2025/10/15 21:11, Vlastimil Babka wrote:
> On 10/15/25 14:59, Hao Ge wrote:
>> From: Hao Ge <gehao@kylinos.cn>
>>
>> If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL,
>> But we did not clear it when freeing the slab. Since OBJEXTS_ALLOC_FAIL and
>> MEMCG_DATA_OBJEXTS currently share the same bit position, during the
>> release of the associated folio, a VM_BUG_ON_FOLIO() check in
>> folio_memcg_kmem() is triggered because it was mistakenly assumed that
>> a valid folio->memcg_data was not cleared before freeing the folio.
>>
>> When freeing a slab, we clear slab->obj_exts if the obj_ext array has been
>> successfully allocated. So let's clear OBJEXTS_ALLOC_FAIL when freeing
>> a slab if the obj_ext array allocated fail to allow them to be returned
>> to the buddy system more smoothly.
> Thanks!
>
>> Fixes: 7612833192d5 ("slab: Reuse first bit for OBJEXTS_ALLOC_FAIL")
>> Suggested-by: Harry Yoo <harry.yoo@oracle.com>
>> Signed-off-by: Hao Ge <gehao@kylinos.cn>
>> Reviewed-by: Suren Baghdasaryan <surenb@google.com>
>> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> Since we changed the approach completely, I think we should not carry over
> the previously added review tags in this case.
got it.
>
>> ---
>> v4: Based on the discussion between Vlastimil and Harry,
>>      modify the solution to clear OBJEXTS_ALLOC_FAIL when freeing a slab.
>>      This does seem more reasonable. Thank you both.
>> ---
>>   mm/slab.h | 26 ++++++++++++++++++++++++++
>>   mm/slub.c |  6 ++++++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/mm/slab.h b/mm/slab.h
>> index 078daecc7cf5..52424d6871bd 100644
>> --- a/mm/slab.h
>> +++ b/mm/slab.h
>> @@ -547,6 +547,28 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
>>   	return (struct slabobj_ext *)(obj_exts & ~OBJEXTS_FLAGS_MASK);
>>   }
>>   
>> +/*
>> + * objexts_clear_alloc_fail -  Clear the OBJEXTS_ALLOC_FAIL for
>> + * the slab object extension vector associated with a slab.
>> + * @slab: a pointer to the slab struct
>> + */
>> +static inline void objexts_clear_alloc_fail(struct slab *slab)
>> +{
>> +	unsigned long obj_exts = READ_ONCE(slab->obj_exts);
>> +
>> +#ifdef CONFIG_MEMCG
>> +	/*
>> +	 * obj_exts should be either NULL, a valid pointer with
>> +	 * MEMCG_DATA_OBJEXTS bit set or be equal to OBJEXTS_ALLOC_FAIL.
>> +	 */
>> +	VM_BUG_ON_PAGE(obj_exts && !(obj_exts & MEMCG_DATA_OBJEXTS) &&
>> +		       obj_exts != OBJEXTS_ALLOC_FAIL, slab_page(slab));
>> +	VM_BUG_ON_PAGE(obj_exts & MEMCG_DATA_KMEM, slab_page(slab));
>> +#endif
>> +
>> +	obj_exts &= ~OBJEXTS_ALLOC_FAIL;
>> +	WRITE_ONCE(slab->obj_exts, obj_exts);
>> +}
> This is much larger than necessary I think. See below.
>
>>   int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
>>                           gfp_t gfp, bool new_slab);
>>   
>> @@ -557,6 +579,10 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
>>   	return NULL;
>>   }
>>   
>> +static inline void objexts_clear_alloc_fail(struct slab *slab)
>> +{
>> +}
>> +
>>   #endif /* CONFIG_SLAB_OBJ_EXT */
>>   
>>   static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
>> diff --git a/mm/slub.c b/mm/slub.c
>> index b1f15598fbfd..80166a4a62f9 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -2169,6 +2169,12 @@ static inline void free_slab_obj_exts(struct slab *slab)
>>   {
>>   	struct slabobj_ext *obj_exts;
>>   
>> +	/*
>> +	 * If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL,
>> +	 * Therefore, we should clear the OBJEXTS_ALLOC_FAIL flag first when freeing a slab.
>> +	 */
>> +	objexts_clear_alloc_fail(slab);
>> +
>>   	obj_exts = slab_obj_exts(slab);
>>   	if (!obj_exts)
>>   		return;
> It should be enough that this return path does "slab->obj_exts = 0;" no?

I might have had a momentary mental block just now,sorry,

this modification method is indeed much simpler.

I will send v5 as soon as possible.


>
>


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [syzbot ci] Re: slab: clear OBJEXTS_ALLOC_FAIL when freeing a slab
  2025-10-15 12:59 [PATCH v4] slab: clear OBJEXTS_ALLOC_FAIL when freeing a slab Hao Ge
  2025-10-15 13:11 ` Vlastimil Babka
@ 2025-10-15 18:47 ` syzbot ci
  1 sibling, 0 replies; 4+ messages in thread
From: syzbot ci @ 2025-10-15 18:47 UTC (permalink / raw)
  To: akpm, ast, cl, gehao, hao.ge, harry.yoo, linux-kernel, linux-mm,
	rientjes, roman.gushchin, shakeel.butt, surenb, vbabka
  Cc: syzbot, syzkaller-bugs

syzbot ci has tested the following series

[v4] slab: clear OBJEXTS_ALLOC_FAIL when freeing a slab
https://lore.kernel.org/all/20251015125945.481950-1-hao.ge@linux.dev
* [PATCH v4] slab: clear OBJEXTS_ALLOC_FAIL when freeing a slab

and found the following issue:
kernel BUG in __free_slab

Full report is available here:
https://ci.syzbot.org/series/61ff4fe1-2e84-410d-ad85-42ead772d9c8

***

kernel BUG in __free_slab

tree:      torvalds
URL:       https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux
base:      3a8660878839faadb4f1a6dd72c3179c1df56787
arch:      amd64
compiler:  Debian clang version 20.1.8 (++20250708063551+0c9f909b7976-1~exp1~20250708183702.136), Debian LLD 20.1.8
config:    https://ci.syzbot.org/builds/e5875084-ea86-418c-979d-5b00cded86ca/config
syz repro: https://ci.syzbot.org/findings/1f8913d8-6d9a-448a-9419-90b758c82c3b/syz_repro

 ___sys_sendmsg+0x21f/0x2a0 net/socket.c:2684
 __sys_sendmsg net/socket.c:2716 [inline]
 __do_sys_sendmsg net/socket.c:2721 [inline]
 __se_sys_sendmsg net/socket.c:2719 [inline]
 __x64_sys_sendmsg+0x19b/0x260 net/socket.c:2719
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
------------[ cut here ]------------
kernel BUG at mm/slab.h:544!
Oops: invalid opcode: 0000 [#1] SMP KASAN PTI
CPU: 0 UID: 0 PID: 15 Comm: ksoftirqd/0 Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
RIP: 0010:slab_obj_exts mm/slab.h:543 [inline]
RIP: 0010:free_slab_obj_exts mm/slub.c:2178 [inline]
RIP: 0010:unaccount_slab mm/slub.c:3186 [inline]
RIP: 0010:__free_slab+0x1b8/0x1e0 mm/slub.c:3286
Code: e8 2d 8a 0c ff 90 0f 0b 48 89 df 48 c7 c6 18 ad 91 8d e8 1b 8a 0c ff 90 0f 0b 48 89 df 48 c7 c6 9d 66 95 8d e8 09 8a 0c ff 90 <0f> 0b 48 89 df 48 c7 c6 18 ad 91 8d e8 f7 89 0c ff 90 0f 0b 48 89
RSP: 0000:ffffc900001478b0 EFLAGS: 00010246
RAX: 680205e52e87ff00 RBX: ffffea00044c3c80 RCX: 680205e52e87ff00
RDX: 0000000000000000 RSI: ffffffff8d7e835a RDI: ffffffff8bc076e0
RBP: 0000000000000001 R08: ffffffff8f9e1177 R09: 1ffffffff1f3c22e
R10: dffffc0000000000 R11: fffffbfff1f3c22f R12: ffffffff821b61b0
R13: ffffffff81a82877 R14: ffff888160415a00 R15: ffffea00044c3c98
FS:  0000000000000000(0000) GS:ffff88818e70c000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fef42bb12f8 CR3: 000000000df38000 CR4: 00000000000006f0
Call Trace:
 <TASK>
 rcu_do_batch kernel/rcu/tree.c:2605 [inline]
 rcu_core+0xcab/0x1770 kernel/rcu/tree.c:2861
 handle_softirqs+0x286/0x870 kernel/softirq.c:622
 run_ksoftirqd+0x9b/0x100 kernel/softirq.c:1063
 smpboot_thread_fn+0x542/0xa60 kernel/smpboot.c:160
 kthread+0x711/0x8a0 kernel/kthread.c:463
 ret_from_fork+0x4bc/0x870 arch/x86/kernel/process.c:158
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:slab_obj_exts mm/slab.h:543 [inline]
RIP: 0010:free_slab_obj_exts mm/slub.c:2178 [inline]
RIP: 0010:unaccount_slab mm/slub.c:3186 [inline]
RIP: 0010:__free_slab+0x1b8/0x1e0 mm/slub.c:3286
Code: e8 2d 8a 0c ff 90 0f 0b 48 89 df 48 c7 c6 18 ad 91 8d e8 1b 8a 0c ff 90 0f 0b 48 89 df 48 c7 c6 9d 66 95 8d e8 09 8a 0c ff 90 <0f> 0b 48 89 df 48 c7 c6 18 ad 91 8d e8 f7 89 0c ff 90 0f 0b 48 89
RSP: 0000:ffffc900001478b0 EFLAGS: 00010246
RAX: 680205e52e87ff00 RBX: ffffea00044c3c80 RCX: 680205e52e87ff00
RDX: 0000000000000000 RSI: ffffffff8d7e835a RDI: ffffffff8bc076e0
RBP: 0000000000000001 R08: ffffffff8f9e1177 R09: 1ffffffff1f3c22e
R10: dffffc0000000000 R11: fffffbfff1f3c22f R12: ffffffff821b61b0
R13: ffffffff81a82877 R14: ffff888160415a00 R15: ffffea00044c3c98
FS:  0000000000000000(0000) GS:ffff88818e70c000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fef42bb12f8 CR3: 000000000df38000 CR4: 00000000000006f0


***

If these findings have caused you to resend the series or submit a
separate fix, please add the following tag to your commit message:
  Tested-by: syzbot@syzkaller.appspotmail.com

---
This report is generated by a bot. It may contain errors.
syzbot ci engineers can be reached at syzkaller@googlegroups.com.


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-10-15 18:47 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-10-15 12:59 [PATCH v4] slab: clear OBJEXTS_ALLOC_FAIL when freeing a slab Hao Ge
2025-10-15 13:11 ` Vlastimil Babka
2025-10-15 13:37   ` Hao Ge
2025-10-15 18:47 ` [syzbot ci] " syzbot ci

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox