* [PATCH v5] slab: reset obj_ext when it is not actually valid during freeing
@ 2025-10-15 14:16 Hao Ge
2025-10-15 16:29 ` Suren Baghdasaryan
0 siblings, 1 reply; 8+ messages in thread
From: Hao Ge @ 2025-10-15 14:16 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 and reset it to 0
if the obj_ext array has been successfully allocated.
So let's reset slab->obj_exts to 0 when freeing a slab if
the obj_ext array allocated fail to allow them to be returned
to the buddy system more smoothly.
Signed-off-by: Hao Ge <gehao@kylinos.cn>
---
v5: Adopt the simpler solution proposed by Vlastimil;
Many thanks to him
---
mm/slub.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c
index b1f15598fbfd..2e4340c75be2 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2170,8 +2170,16 @@ static inline void free_slab_obj_exts(struct slab *slab)
struct slabobj_ext *obj_exts;
obj_exts = slab_obj_exts(slab);
- if (!obj_exts)
+ if (!obj_exts) {
+ /*
+ * If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL,
+ * In this case, we will end up here.
+ * Therefore, we should clear the OBJEXTS_ALLOC_FAIL flag first when freeing a slab.
+ * Then let's set it to 0 as below.
+ */
+ slab->obj_exts = 0;
return;
+ }
/*
* obj_exts was created with __GFP_NO_OBJ_EXT flag, therefore its
--
2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v5] slab: reset obj_ext when it is not actually valid during freeing 2025-10-15 14:16 [PATCH v5] slab: reset obj_ext when it is not actually valid during freeing Hao Ge @ 2025-10-15 16:29 ` Suren Baghdasaryan 2025-10-15 16:37 ` Vlastimil Babka 2025-10-15 16:44 ` Vlastimil Babka 0 siblings, 2 replies; 8+ messages in thread From: Suren Baghdasaryan @ 2025-10-15 16:29 UTC (permalink / raw) To: Hao Ge Cc: Vlastimil Babka, Andrew Morton, Christoph Lameter, David Rientjes, Roman Gushchin, Harry Yoo, Alexei Starovoitov, Shakeel Butt, linux-mm, linux-kernel, Hao Ge On Wed, Oct 15, 2025 at 7:17 AM Hao Ge <hao.ge@linux.dev> 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 and reset it to 0 > if the obj_ext array has been successfully allocated. > So let's reset slab->obj_exts to 0 when freeing a slab if > the obj_ext array allocated fail to allow them to be returned > to the buddy system more smoothly. > > Signed-off-by: Hao Ge <gehao@kylinos.cn> > --- > v5: Adopt the simpler solution proposed by Vlastimil; > Many thanks to him > --- > mm/slub.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/mm/slub.c b/mm/slub.c > index b1f15598fbfd..2e4340c75be2 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -2170,8 +2170,16 @@ static inline void free_slab_obj_exts(struct slab *slab) > struct slabobj_ext *obj_exts; > > obj_exts = slab_obj_exts(slab); > - if (!obj_exts) > + if (!obj_exts) { > + /* > + * If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL, > + * In this case, we will end up here. > + * Therefore, we should clear the OBJEXTS_ALLOC_FAIL flag first when freeing a slab. > + * Then let's set it to 0 as below. > + */ > + slab->obj_exts = 0; > return; > + } How about this instead: static inline void free_slab_obj_exts(struct slab *slab) { struct slabobj_ext *obj_exts; obj_exts = slab_obj_exts(slab); + /* + * Reset obj_exts to ensure all bits including OBJEXTS_ALLOC_FAIL + * are always cleared. + */ + slab->obj_exts = 0; if (!obj_exts) return; /* * obj_exts was created with __GFP_NO_OBJ_EXT flag, therefore its * corresponding extension will be NULL. alloc_tag_sub() will throw a * warning if slab has extensions but the extension of an object is * NULL, therefore replace NULL with CODETAG_EMPTY to indicate that * the extension for obj_exts is expected to be NULL. */ mark_objexts_empty(obj_exts); kfree(obj_exts); - slab->obj_exts = 0; } > > /* > * obj_exts was created with __GFP_NO_OBJ_EXT flag, therefore its > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] slab: reset obj_ext when it is not actually valid during freeing 2025-10-15 16:29 ` Suren Baghdasaryan @ 2025-10-15 16:37 ` Vlastimil Babka 2025-10-15 16:51 ` Suren Baghdasaryan 2025-10-15 16:44 ` Vlastimil Babka 1 sibling, 1 reply; 8+ messages in thread From: Vlastimil Babka @ 2025-10-15 16:37 UTC (permalink / raw) To: Suren Baghdasaryan, Hao Ge Cc: Andrew Morton, Christoph Lameter, David Rientjes, Roman Gushchin, Harry Yoo, Alexei Starovoitov, Shakeel Butt, linux-mm, linux-kernel, Hao Ge On 10/15/25 18:29, Suren Baghdasaryan wrote: > On Wed, Oct 15, 2025 at 7:17 AM Hao Ge <hao.ge@linux.dev> 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 and reset it to 0 >> if the obj_ext array has been successfully allocated. >> So let's reset slab->obj_exts to 0 when freeing a slab if >> the obj_ext array allocated fail to allow them to be returned >> to the buddy system more smoothly. >> >> Signed-off-by: Hao Ge <gehao@kylinos.cn> >> --- >> v5: Adopt the simpler solution proposed by Vlastimil; >> Many thanks to him >> --- >> mm/slub.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/mm/slub.c b/mm/slub.c >> index b1f15598fbfd..2e4340c75be2 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -2170,8 +2170,16 @@ static inline void free_slab_obj_exts(struct slab *slab) >> struct slabobj_ext *obj_exts; >> >> obj_exts = slab_obj_exts(slab); >> - if (!obj_exts) >> + if (!obj_exts) { >> + /* >> + * If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL, >> + * In this case, we will end up here. >> + * Therefore, we should clear the OBJEXTS_ALLOC_FAIL flag first when freeing a slab. >> + * Then let's set it to 0 as below. >> + */ >> + slab->obj_exts = 0; >> return; >> + } > > How about this instead: > > static inline void free_slab_obj_exts(struct slab *slab) > { > struct slabobj_ext *obj_exts; > > obj_exts = slab_obj_exts(slab); > + /* > + * Reset obj_exts to ensure all bits including OBJEXTS_ALLOC_FAIL > + * are always cleared. > + */ > + slab->obj_exts = 0; > if (!obj_exts) > return; > > /* > * obj_exts was created with __GFP_NO_OBJ_EXT flag, therefore its > * corresponding extension will be NULL. alloc_tag_sub() will throw a > * warning if slab has extensions but the extension of an object is > * NULL, therefore replace NULL with CODETAG_EMPTY to indicate that > * the extension for obj_exts is expected to be NULL. > */ > mark_objexts_empty(obj_exts); > kfree(obj_exts); > - slab->obj_exts = 0; You have an older base, check current mainline, we evaluate slab->obj_exts later in the function > } > >> >> /* >> * obj_exts was created with __GFP_NO_OBJ_EXT flag, therefore its >> -- >> 2.25.1 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] slab: reset obj_ext when it is not actually valid during freeing 2025-10-15 16:37 ` Vlastimil Babka @ 2025-10-15 16:51 ` Suren Baghdasaryan 0 siblings, 0 replies; 8+ messages in thread From: Suren Baghdasaryan @ 2025-10-15 16:51 UTC (permalink / raw) To: Vlastimil Babka Cc: Hao Ge, Andrew Morton, Christoph Lameter, David Rientjes, Roman Gushchin, Harry Yoo, Alexei Starovoitov, Shakeel Butt, linux-mm, linux-kernel, Hao Ge On Wed, Oct 15, 2025 at 9:37 AM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 10/15/25 18:29, Suren Baghdasaryan wrote: > > On Wed, Oct 15, 2025 at 7:17 AM Hao Ge <hao.ge@linux.dev> 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 and reset it to 0 > >> if the obj_ext array has been successfully allocated. > >> So let's reset slab->obj_exts to 0 when freeing a slab if > >> the obj_ext array allocated fail to allow them to be returned > >> to the buddy system more smoothly. > >> > >> Signed-off-by: Hao Ge <gehao@kylinos.cn> > >> --- > >> v5: Adopt the simpler solution proposed by Vlastimil; > >> Many thanks to him > >> --- > >> mm/slub.c | 10 +++++++++- > >> 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/mm/slub.c b/mm/slub.c > >> index b1f15598fbfd..2e4340c75be2 100644 > >> --- a/mm/slub.c > >> +++ b/mm/slub.c > >> @@ -2170,8 +2170,16 @@ static inline void free_slab_obj_exts(struct slab *slab) > >> struct slabobj_ext *obj_exts; > >> > >> obj_exts = slab_obj_exts(slab); > >> - if (!obj_exts) > >> + if (!obj_exts) { > >> + /* > >> + * If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL, > >> + * In this case, we will end up here. > >> + * Therefore, we should clear the OBJEXTS_ALLOC_FAIL flag first when freeing a slab. > >> + * Then let's set it to 0 as below. > >> + */ > >> + slab->obj_exts = 0; > >> return; > >> + } > > > > How about this instead: > > > > static inline void free_slab_obj_exts(struct slab *slab) > > { > > struct slabobj_ext *obj_exts; > > > > obj_exts = slab_obj_exts(slab); > > + /* > > + * Reset obj_exts to ensure all bits including OBJEXTS_ALLOC_FAIL > > + * are always cleared. > > + */ > > + slab->obj_exts = 0; > > if (!obj_exts) > > return; > > > > /* > > * obj_exts was created with __GFP_NO_OBJ_EXT flag, therefore its > > * corresponding extension will be NULL. alloc_tag_sub() will throw a > > * warning if slab has extensions but the extension of an object is > > * NULL, therefore replace NULL with CODETAG_EMPTY to indicate that > > * the extension for obj_exts is expected to be NULL. > > */ > > mark_objexts_empty(obj_exts); > > kfree(obj_exts); > > - slab->obj_exts = 0; > > You have an older base, check current mainline, we evaluate slab->obj_exts > later in the function Ah, sorry about that. Yeah, then this looks good. > > > } > > > >> > >> /* > >> * obj_exts was created with __GFP_NO_OBJ_EXT flag, therefore its > >> -- > >> 2.25.1 > >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] slab: reset obj_ext when it is not actually valid during freeing 2025-10-15 16:29 ` Suren Baghdasaryan 2025-10-15 16:37 ` Vlastimil Babka @ 2025-10-15 16:44 ` Vlastimil Babka 2025-10-15 16:52 ` Suren Baghdasaryan 2025-10-16 13:09 ` Harry Yoo 1 sibling, 2 replies; 8+ messages in thread From: Vlastimil Babka @ 2025-10-15 16:44 UTC (permalink / raw) To: Suren Baghdasaryan, Hao Ge Cc: Andrew Morton, Christoph Lameter, David Rientjes, Roman Gushchin, Harry Yoo, Alexei Starovoitov, Shakeel Butt, linux-mm, linux-kernel, Hao Ge On 10/15/25 18:29, Suren Baghdasaryan wrote: > On Wed, Oct 15, 2025 at 7:17 AM Hao Ge <hao.ge@linux.dev> 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 and reset it to 0 >> if the obj_ext array has been successfully allocated. >> So let's reset slab->obj_exts to 0 when freeing a slab if >> the obj_ext array allocated fail to allow them to be returned >> to the buddy system more smoothly. >> >> Signed-off-by: Hao Ge <gehao@kylinos.cn> >> --- >> v5: Adopt the simpler solution proposed by Vlastimil; >> Many thanks to him I've massaged the commit log and comments a bit and also realized that AFAICS we're actually fixing an issue that predates 7612833192d5 ("slab: Reuse first bit for OBJEXTS_ALLOC_FAIL"). Am I wrong? ----8<---- From 8151384e5baf34db5812ed51e2e463796ab6e973 Mon Sep 17 00:00:00 2001 From: Hao Ge <gehao@kylinos.cn> Date: Wed, 15 Oct 2025 22:16:42 +0800 Subject: [PATCH] slab: reset slab->obj_ext when freeing and it is OBJEXTS_ALLOC_FAIL If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL, But we do 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. Another problem that predates sharing the OBJEXTS_ALLOC_FAIL and MEMCG_DATA_OBJEXTS bits is that on configurations with is_check_pages_enabled(), the non-cleared bit in page->memcg_data will trigger a free_page_is_bad() failure "page still charged to cgroup" When freeing a slab, we clear slab->obj_exts if the obj_ext array has been successfully allocated. So let's clear it also when the allocation has failed. Fixes: 09c46563ff6d ("codetag: debug: introduce OBJEXTS_ALLOC_FAIL to mark failed slab_ext allocations") Link: https://lore.kernel.org/all/20251015141642.700170-1-hao.ge@linux.dev/ Cc: <stable@vger.kernel.org> Signed-off-by: Hao Ge <gehao@kylinos.cn> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slub.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/mm/slub.c b/mm/slub.c index 13ae4491136a..a8fcc7e6f25a 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -2170,8 +2170,15 @@ static inline void free_slab_obj_exts(struct slab *slab) struct slabobj_ext *obj_exts; obj_exts = slab_obj_exts(slab); - if (!obj_exts) + if (!obj_exts) { + /* + * If obj_exts allocation failed, slab->obj_exts is set to + * OBJEXTS_ALLOC_FAIL. In this case, we end up here and should + * clear the flag. + */ + slab->obj_exts = 0; return; + } /* * obj_exts was created with __GFP_NO_OBJ_EXT flag, therefore its -- 2.51.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] slab: reset obj_ext when it is not actually valid during freeing 2025-10-15 16:44 ` Vlastimil Babka @ 2025-10-15 16:52 ` Suren Baghdasaryan 2025-10-16 13:09 ` Harry Yoo 1 sibling, 0 replies; 8+ messages in thread From: Suren Baghdasaryan @ 2025-10-15 16:52 UTC (permalink / raw) To: Vlastimil Babka Cc: Hao Ge, Andrew Morton, Christoph Lameter, David Rientjes, Roman Gushchin, Harry Yoo, Alexei Starovoitov, Shakeel Butt, linux-mm, linux-kernel, Hao Ge On Wed, Oct 15, 2025 at 9:44 AM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 10/15/25 18:29, Suren Baghdasaryan wrote: > > On Wed, Oct 15, 2025 at 7:17 AM Hao Ge <hao.ge@linux.dev> 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 and reset it to 0 > >> if the obj_ext array has been successfully allocated. > >> So let's reset slab->obj_exts to 0 when freeing a slab if > >> the obj_ext array allocated fail to allow them to be returned > >> to the buddy system more smoothly. > >> > >> Signed-off-by: Hao Ge <gehao@kylinos.cn> > >> --- > >> v5: Adopt the simpler solution proposed by Vlastimil; > >> Many thanks to him > > I've massaged the commit log and comments a bit and also realized that > AFAICS we're actually fixing an issue that predates 7612833192d5 ("slab: > Reuse first bit for OBJEXTS_ALLOC_FAIL"). Am I wrong? > > ----8<---- > From 8151384e5baf34db5812ed51e2e463796ab6e973 Mon Sep 17 00:00:00 2001 > From: Hao Ge <gehao@kylinos.cn> > Date: Wed, 15 Oct 2025 22:16:42 +0800 > Subject: [PATCH] slab: reset slab->obj_ext when freeing and it is > OBJEXTS_ALLOC_FAIL > > If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL, > But we do 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. > > Another problem that predates sharing the OBJEXTS_ALLOC_FAIL and > MEMCG_DATA_OBJEXTS bits is that on configurations with > is_check_pages_enabled(), the non-cleared bit in page->memcg_data will > trigger a free_page_is_bad() failure "page still charged to cgroup" > > When freeing a slab, we clear slab->obj_exts if the obj_ext array has > been successfully allocated. So let's clear it also when the allocation > has failed. > > Fixes: 09c46563ff6d ("codetag: debug: introduce OBJEXTS_ALLOC_FAIL to mark failed slab_ext allocations") > Link: https://lore.kernel.org/all/20251015141642.700170-1-hao.ge@linux.dev/ > Cc: <stable@vger.kernel.org> > Signed-off-by: Hao Ge <gehao@kylinos.cn> > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Reviewed-by: Suren Baghdasaryan <surenb@google.com> > --- > mm/slub.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 13ae4491136a..a8fcc7e6f25a 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -2170,8 +2170,15 @@ static inline void free_slab_obj_exts(struct slab *slab) > struct slabobj_ext *obj_exts; > > obj_exts = slab_obj_exts(slab); > - if (!obj_exts) > + if (!obj_exts) { > + /* > + * If obj_exts allocation failed, slab->obj_exts is set to > + * OBJEXTS_ALLOC_FAIL. In this case, we end up here and should > + * clear the flag. > + */ > + slab->obj_exts = 0; > return; > + } > > /* > * obj_exts was created with __GFP_NO_OBJ_EXT flag, therefore its > -- > 2.51.0 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] slab: reset obj_ext when it is not actually valid during freeing 2025-10-15 16:44 ` Vlastimil Babka 2025-10-15 16:52 ` Suren Baghdasaryan @ 2025-10-16 13:09 ` Harry Yoo 2025-10-16 13:18 ` Vlastimil Babka 1 sibling, 1 reply; 8+ messages in thread From: Harry Yoo @ 2025-10-16 13:09 UTC (permalink / raw) To: Vlastimil Babka Cc: Suren Baghdasaryan, Hao Ge, Andrew Morton, Christoph Lameter, David Rientjes, Roman Gushchin, Alexei Starovoitov, Shakeel Butt, linux-mm, linux-kernel, Hao Ge On Wed, Oct 15, 2025 at 06:44:38PM +0200, Vlastimil Babka wrote: > On 10/15/25 18:29, Suren Baghdasaryan wrote: > > On Wed, Oct 15, 2025 at 7:17 AM Hao Ge <hao.ge@linux.dev> 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 and reset it to 0 > >> if the obj_ext array has been successfully allocated. > >> So let's reset slab->obj_exts to 0 when freeing a slab if > >> the obj_ext array allocated fail to allow them to be returned > >> to the buddy system more smoothly. > >> > >> Signed-off-by: Hao Ge <gehao@kylinos.cn> > >> --- > >> v5: Adopt the simpler solution proposed by Vlastimil; > >> Many thanks to him > > I've massaged the commit log and comments a bit and also realized that > AFAICS we're actually fixing an issue that predates 7612833192d5 ("slab: > Reuse first bit for OBJEXTS_ALLOC_FAIL"). Am I wrong? You're not wrong! > ----8<---- > From 8151384e5baf34db5812ed51e2e463796ab6e973 Mon Sep 17 00:00:00 2001 > From: Hao Ge <gehao@kylinos.cn> > Date: Wed, 15 Oct 2025 22:16:42 +0800 > Subject: [PATCH] slab: reset slab->obj_ext when freeing and it is > OBJEXTS_ALLOC_FAIL > > If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL, > But we do 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. nit: maybe this can be massaged as "a VM_BUG_ON_FOLIO() check in folio_memcg_kmem() is triggered because the OBJEXTS_ALLOC_FAIL flag was not cleared, causing it to be interpreted as a kmem folio (non-slab) with MEMCG_OBJEXTS_DATA flag set, which is invalid because MEMCG_OBJEXTS_DATA is supposed to be set only on slabs." > Another problem that predates sharing the OBJEXTS_ALLOC_FAIL and > MEMCG_DATA_OBJEXTS bits is that on configurations with > is_check_pages_enabled(), the non-cleared bit in page->memcg_data will > trigger a free_page_is_bad() failure "page still charged to cgroup" > > When freeing a slab, we clear slab->obj_exts if the obj_ext array has > been successfully allocated. So let's clear it also when the allocation > has failed. > > Fixes: 09c46563ff6d ("codetag: debug: introduce OBJEXTS_ALLOC_FAIL to mark failed slab_ext allocations") > Link: https://lore.kernel.org/all/20251015141642.700170-1-hao.ge@linux.dev/ > Cc: <stable@vger.kernel.org> > Signed-off-by: Hao Ge <gehao@kylinos.cn> > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > --- Looks good to me, Reviewed-by: Harry Yoo <harry.yoo@oracle.com> -- Cheers, Harry / Hyeonggon > mm/slub.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 13ae4491136a..a8fcc7e6f25a 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -2170,8 +2170,15 @@ static inline void free_slab_obj_exts(struct slab *slab) > struct slabobj_ext *obj_exts; > > obj_exts = slab_obj_exts(slab); > - if (!obj_exts) > + if (!obj_exts) { > + /* > + * If obj_exts allocation failed, slab->obj_exts is set to > + * OBJEXTS_ALLOC_FAIL. In this case, we end up here and should > + * clear the flag. > + */ > + slab->obj_exts = 0; > return; > + } > > /* > * obj_exts was created with __GFP_NO_OBJ_EXT flag, therefore its > -- > 2.51.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] slab: reset obj_ext when it is not actually valid during freeing 2025-10-16 13:09 ` Harry Yoo @ 2025-10-16 13:18 ` Vlastimil Babka 0 siblings, 0 replies; 8+ messages in thread From: Vlastimil Babka @ 2025-10-16 13:18 UTC (permalink / raw) To: Harry Yoo Cc: Suren Baghdasaryan, Hao Ge, Andrew Morton, Christoph Lameter, David Rientjes, Roman Gushchin, Alexei Starovoitov, Shakeel Butt, linux-mm, linux-kernel, Hao Ge On 10/16/25 15:09, Harry Yoo wrote: >> ----8<---- >> From 8151384e5baf34db5812ed51e2e463796ab6e973 Mon Sep 17 00:00:00 2001 >> From: Hao Ge <gehao@kylinos.cn> >> Date: Wed, 15 Oct 2025 22:16:42 +0800 >> Subject: [PATCH] slab: reset slab->obj_ext when freeing and it is >> OBJEXTS_ALLOC_FAIL >> >> If obj_exts allocation failed, slab->obj_exts is set to OBJEXTS_ALLOC_FAIL, >> But we do 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. > > nit: maybe this can be massaged as "a VM_BUG_ON_FOLIO() check in folio_memcg_kmem() > is triggered because the OBJEXTS_ALLOC_FAIL flag was not cleared, > causing it to be interpreted as a kmem folio (non-slab) with > MEMCG_OBJEXTS_DATA flag set, which is invalid because MEMCG_OBJEXTS_DATA > is supposed to be set only on slabs." Thanks, applied >> Another problem that predates sharing the OBJEXTS_ALLOC_FAIL and >> MEMCG_DATA_OBJEXTS bits is that on configurations with >> is_check_pages_enabled(), the non-cleared bit in page->memcg_data will >> trigger a free_page_is_bad() failure "page still charged to cgroup" >> >> When freeing a slab, we clear slab->obj_exts if the obj_ext array has >> been successfully allocated. So let's clear it also when the allocation >> has failed. >> >> Fixes: 09c46563ff6d ("codetag: debug: introduce OBJEXTS_ALLOC_FAIL to mark failed slab_ext allocations") >> Link: https://lore.kernel.org/all/20251015141642.700170-1-hao.ge@linux.dev/ >> Cc: <stable@vger.kernel.org> >> Signed-off-by: Hao Ge <gehao@kylinos.cn> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >> --- > > Looks good to me, > Reviewed-by: Harry Yoo <harry.yoo@oracle.com> and this too. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-10-16 13:18 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-10-15 14:16 [PATCH v5] slab: reset obj_ext when it is not actually valid during freeing Hao Ge 2025-10-15 16:29 ` Suren Baghdasaryan 2025-10-15 16:37 ` Vlastimil Babka 2025-10-15 16:51 ` Suren Baghdasaryan 2025-10-15 16:44 ` Vlastimil Babka 2025-10-15 16:52 ` Suren Baghdasaryan 2025-10-16 13:09 ` Harry Yoo 2025-10-16 13:18 ` Vlastimil Babka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox