| Message ID | 20200811124656.10308-1-erosca@de.adit-jv.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: slub: fix conversion of freelist_corrupted() | expand |
On Tue, 11 Aug 2020 14:46:56 +0200 Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > Commit 52f23478081ae0 ("mm/slub.c: fix corrupted freechain in > deactivate_slab()") suffered an update when picked up from LKML [1]. > > Specifically, relocating 'freelist = NULL' into 'freelist_corrupted()' > created a no-op statement. Fix it by sticking to the behavior intended > in the original patch [1]. Prefer the lowest-line-count solution. > > [1] https://lore.kernel.org/linux-mm/20200331031450.12182-1-dongli.zhang@oracle.com/ > > ... > > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -677,7 +677,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page, > if ((s->flags & SLAB_CONSISTENCY_CHECKS) && > !check_valid_pointer(s, page, nextfree)) { > object_err(s, page, freelist, "Freechain corrupt"); > - freelist = NULL; > slab_fix(s, "Isolate corrupted freechain"); > return true; > } > @@ -2184,8 +2183,10 @@ static void deactivate_slab(struct kmem_cache *s, struct page *page, > * 'freelist' is already corrupted. So isolate all objects > * starting at 'freelist'. > */ > - if (freelist_corrupted(s, page, freelist, nextfree)) > + if (freelist_corrupted(s, page, freelist, nextfree)) { > + freelist = NULL; > break; > + } > > do { > prior = page->freelist; Looks right. What are the runtime effects of this change? In other words, what are the end user visible effects of the present defect?
Dear Andrew, On Tue, Aug 11, 2020 at 01:49:09PM -0700, Andrew Morton wrote: > On Tue, 11 Aug 2020 14:46:56 +0200 Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > Commit 52f23478081ae0 ("mm/slub.c: fix corrupted freechain in > > deactivate_slab()") suffered an update when picked up from LKML [1]. > > > > Specifically, relocating 'freelist = NULL' into 'freelist_corrupted()' > > created a no-op statement. Fix it by sticking to the behavior intended > > in the original patch [1]. Prefer the lowest-line-count solution. > > > > [1] https://lore.kernel.org/linux-mm/20200331031450.12182-1-dongli.zhang@oracle.com/ > > > > ... > > > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -677,7 +677,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page, > > if ((s->flags & SLAB_CONSISTENCY_CHECKS) && > > !check_valid_pointer(s, page, nextfree)) { > > object_err(s, page, freelist, "Freechain corrupt"); > > - freelist = NULL; > > slab_fix(s, "Isolate corrupted freechain"); > > return true; > > } > > @@ -2184,8 +2183,10 @@ static void deactivate_slab(struct kmem_cache *s, struct page *page, > > * 'freelist' is already corrupted. So isolate all objects > > * starting at 'freelist'. > > */ > > - if (freelist_corrupted(s, page, freelist, nextfree)) > > + if (freelist_corrupted(s, page, freelist, nextfree)) { > > + freelist = NULL; > > break; > > + } > > > > do { > > prior = page->freelist; > > Looks right. > > What are the runtime effects of this change? In other words, what are > the end user visible effects of the present defect? Thank you for the prompt feedback. The issue popped up as a result of static analysis and code review. Therefore, I lack any specific runtime behavior example being fixed. Nevertheless, I think this does not diminish the concern expressed in the description of the patch.
On 8/11/20 5:46 AM, Eugeniu Rosca wrote: > Commit 52f23478081ae0 ("mm/slub.c: fix corrupted freechain in > deactivate_slab()") suffered an update when picked up from LKML [1]. > > Specifically, relocating 'freelist = NULL' into 'freelist_corrupted()' > created a no-op statement. Fix it by sticking to the behavior intended > in the original patch [1]. Prefer the lowest-line-count solution. > > [1] https://lore.kernel.org/linux-mm/20200331031450.12182-1-dongli.zhang@oracle.com/ > > Fixes: 52f23478081ae0 ("mm/slub.c: fix corrupted freechain in deactivate_slab()") > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Dongli Zhang <dongli.zhang@oracle.com> > Cc: <stable@vger.kernel.org> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > --- > mm/slub.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 68c02b2eecd9..9a3e963b02a3 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -677,7 +677,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page, > if ((s->flags & SLAB_CONSISTENCY_CHECKS) && > !check_valid_pointer(s, page, nextfree)) { > object_err(s, page, freelist, "Freechain corrupt"); > - freelist = NULL; > slab_fix(s, "Isolate corrupted freechain"); > return true; > } > @@ -2184,8 +2183,10 @@ static void deactivate_slab(struct kmem_cache *s, struct page *page, > * 'freelist' is already corrupted. So isolate all objects > * starting at 'freelist'. > */ > - if (freelist_corrupted(s, page, freelist, nextfree)) > + if (freelist_corrupted(s, page, freelist, nextfree)) { > + freelist = NULL; This is good to me. However, this would confuse people when CONFIG_SLUB_DEBUG is not defined. While reading the source code, people may be curious why to reset freelist when CONFIG_SLUB_DEBUG is even not defined. Dongli Zhang
Hello Dongli, On Thu, Aug 13, 2020 at 11:57:51PM -0700, Dongli Zhang wrote: > On 8/11/20 5:46 AM, Eugeniu Rosca wrote: > > Commit 52f23478081ae0 ("mm/slub.c: fix corrupted freechain in > > deactivate_slab()") suffered an update when picked up from LKML [1]. > > > > Specifically, relocating 'freelist = NULL' into 'freelist_corrupted()' > > created a no-op statement. Fix it by sticking to the behavior intended > > in the original patch [1]. Prefer the lowest-line-count solution. > > > > [1] https://lore.kernel.org/linux-mm/20200331031450.12182-1-dongli.zhang@oracle.com/ > > > > Fixes: 52f23478081ae0 ("mm/slub.c: fix corrupted freechain in deactivate_slab()") > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Dongli Zhang <dongli.zhang@oracle.com> > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > --- > > mm/slub.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/mm/slub.c b/mm/slub.c > > index 68c02b2eecd9..9a3e963b02a3 100644 > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -677,7 +677,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page, > > if ((s->flags & SLAB_CONSISTENCY_CHECKS) && > > !check_valid_pointer(s, page, nextfree)) { > > object_err(s, page, freelist, "Freechain corrupt"); > > - freelist = NULL; > > slab_fix(s, "Isolate corrupted freechain"); > > return true; > > } > > @@ -2184,8 +2183,10 @@ static void deactivate_slab(struct kmem_cache *s, struct page *page, > > * 'freelist' is already corrupted. So isolate all objects > > * starting at 'freelist'. > > */ > > - if (freelist_corrupted(s, page, freelist, nextfree)) > > + if (freelist_corrupted(s, page, freelist, nextfree)) { > > + freelist = NULL; > > This is good to me. > > However, this would confuse people when CONFIG_SLUB_DEBUG is not defined. > > While reading the source code, people may be curious why to reset freelist when > CONFIG_SLUB_DEBUG is even not defined. This is a fair point. To address it, the `freelist = NULL` assignment should be then moved into the body of freelist_corrupted(). If no concerns on that, I will soon push a v2 implementing this proposal.
On 8/14/20 12:46 AM, Eugeniu Rosca wrote: > Hello Dongli, > > On Thu, Aug 13, 2020 at 11:57:51PM -0700, Dongli Zhang wrote: >> On 8/11/20 5:46 AM, Eugeniu Rosca wrote: >>> Commit 52f23478081ae0 ("mm/slub.c: fix corrupted freechain in >>> deactivate_slab()") suffered an update when picked up from LKML [1]. >>> >>> Specifically, relocating 'freelist = NULL' into 'freelist_corrupted()' >>> created a no-op statement. Fix it by sticking to the behavior intended >>> in the original patch [1]. Prefer the lowest-line-count solution. >>> >>> [1] https://urldefense.com/v3/__https://lore.kernel.org/linux-mm/20200331031450.12182-1-dongli.zhang@oracle.com/__;!!GqivPVa7Brio!LkxH4qJ3WzKnO_nmONLWV-HAougEaefnp8UnI6qC_8j0SS9_9fkO6bOe68flixlQzx8$ >>> >>> Fixes: 52f23478081ae0 ("mm/slub.c: fix corrupted freechain in deactivate_slab()") >>> Cc: Andrew Morton <akpm@linux-foundation.org> >>> Cc: Dongli Zhang <dongli.zhang@oracle.com> >>> Cc: <stable@vger.kernel.org> >>> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> >>> --- >>> mm/slub.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/mm/slub.c b/mm/slub.c >>> index 68c02b2eecd9..9a3e963b02a3 100644 >>> --- a/mm/slub.c >>> +++ b/mm/slub.c >>> @@ -677,7 +677,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page, >>> if ((s->flags & SLAB_CONSISTENCY_CHECKS) && >>> !check_valid_pointer(s, page, nextfree)) { >>> object_err(s, page, freelist, "Freechain corrupt"); >>> - freelist = NULL; >>> slab_fix(s, "Isolate corrupted freechain"); >>> return true; >>> } >>> @@ -2184,8 +2183,10 @@ static void deactivate_slab(struct kmem_cache *s, struct page *page, >>> * 'freelist' is already corrupted. So isolate all objects >>> * starting at 'freelist'. >>> */ >>> - if (freelist_corrupted(s, page, freelist, nextfree)) >>> + if (freelist_corrupted(s, page, freelist, nextfree)) { >>> + freelist = NULL; >> >> This is good to me. >> >> However, this would confuse people when CONFIG_SLUB_DEBUG is not defined. >> >> While reading the source code, people may be curious why to reset freelist when >> CONFIG_SLUB_DEBUG is even not defined. > > This is a fair point. To address it, the `freelist = NULL` assignment > should be then moved into the body of freelist_corrupted(). If no > concerns on that, I will soon push a v2 implementing this proposal. > I do have have concern with that if that can make both of static analysis tool and the people reading code happy :) Thank you very much! Dongli Zhang
diff --git a/mm/slub.c b/mm/slub.c index 68c02b2eecd9..9a3e963b02a3 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -677,7 +677,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page, if ((s->flags & SLAB_CONSISTENCY_CHECKS) && !check_valid_pointer(s, page, nextfree)) { object_err(s, page, freelist, "Freechain corrupt"); - freelist = NULL; slab_fix(s, "Isolate corrupted freechain"); return true; } @@ -2184,8 +2183,10 @@ static void deactivate_slab(struct kmem_cache *s, struct page *page, * 'freelist' is already corrupted. So isolate all objects * starting at 'freelist'. */ - if (freelist_corrupted(s, page, freelist, nextfree)) + if (freelist_corrupted(s, page, freelist, nextfree)) { + freelist = NULL; break; + } do { prior = page->freelist;
Commit 52f23478081ae0 ("mm/slub.c: fix corrupted freechain in deactivate_slab()") suffered an update when picked up from LKML [1]. Specifically, relocating 'freelist = NULL' into 'freelist_corrupted()' created a no-op statement. Fix it by sticking to the behavior intended in the original patch [1]. Prefer the lowest-line-count solution. [1] https://lore.kernel.org/linux-mm/20200331031450.12182-1-dongli.zhang@oracle.com/ Fixes: 52f23478081ae0 ("mm/slub.c: fix corrupted freechain in deactivate_slab()") Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Dongli Zhang <dongli.zhang@oracle.com> Cc: <stable@vger.kernel.org> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> --- mm/slub.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)