On Wed, Feb 05, 2025 at 06:57:14PM -0800, David Rientjes wrote:
> On Wed, 5 Feb 2025, Vlastimil Babka wrote:
>
> > On 2/5/25 18:10, David Rientjes wrote:
> > > On Wed, 5 Feb 2025, Hyesoo Yu wrote:
> > >
> > >> If a slab object is corrupted or an error occurs in its internal
> > >> value, continuing after restoration may cause other side effects.
> > >> At this point, it is difficult to debug because the problem occurred
> > >> in the past. It is better to use WARN() instead of pr_err to catch
> > >> errors at the point of issue because WARN() could trigger panic for
> > >> system debugging when panic_on_warn is enabled. WARN() should be
> > >> called prior to fixing the value because when a panic is triggered by WARN(),
> > >> it allows us to check corrupted data.
> > >>
> > >
> > > I think this makes sense, but it doesn't document why the other changes
> > > are being made, like moving the setting of *freelist to NULL. This is
> > > presumably something that you want in the crash dump when
> > > kernel.panic_on_warn is enabled. Probably best to call that out, but to
> > > also indicate what you're relying on in the crash dump to make forward
> > > progress on in diagnosing the issue.
> >
> > Well the last sentence of the changelog above says exactly that, no?
> >
>
> Sorry, I should have been more clear. It's unclear in the code why
> choosing WARN() here is helpful given the stack would be known. It makes
> sense to enable kernel.panic_on_warn this way for debugging purposes, but
> thought it should also carry a comment in the code on the rationale (and
> the state we're trying to capture in a crash dump) so a future change
> doesn't go and unravel this for us again.
>
What do you think of this comment ? I will add it in patch v3.
/*
* slab_fix indicates that the value would be restored even if an error occurs.
* Or, it is possible to trigger a panic without restoring using WARN() if panic_on_warn
* is enabled. This can obtain a crash dump at the point of issue to debug.
* It is advisable not to restore the data before calling slab_fix() to check for corrupted
* data in the crash dump.
*/
Thanks,
Regards.
> > >> Changes in v2:
> > >> - Replace direct calling with BUG_ON with the use of WARN in slab_fix.
> > >>
> > >> Signed-off-by: Hyesoo Yu <hyesoo.yu@xxxxxxxxxxx>
> > >> ---
> > >> mm/slub.c | 10 +++++-----
> > >> 1 file changed, 5 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/mm/slub.c b/mm/slub.c
> > >> index 1f50129dcfb3..ea956cb4b8be 100644
> > >> --- a/mm/slub.c
> > >> +++ b/mm/slub.c
> > >> @@ -1043,7 +1043,7 @@ static void slab_fix(struct kmem_cache *s, char *fmt, ...)
> > >> va_start(args, fmt);
> > >> vaf.fmt = fmt;
> > >> vaf.va = &args;
> > >> - pr_err("FIX %s: %pV\n", s->name, &vaf);
> > >> + WARN(1, "FIX %s: %pV\n", s->name, &vaf);
> > >> va_end(args);
> > >> }
> > >>
> > >> @@ -1106,8 +1106,8 @@ static bool freelist_corrupted(struct kmem_cache *s, struct slab *slab,
> > >> if ((s->flags & SLAB_CONSISTENCY_CHECKS) &&
> > >> !check_valid_pointer(s, slab, nextfree) && freelist) {
> > >> object_err(s, slab, *freelist, "Freechain corrupt");
> > >> - *freelist = NULL;
> > >> slab_fix(s, "Isolate corrupted freechain");
> > >> + *freelist = NULL;
> > >> return true;
> > >> }
> > >>
> > >> @@ -1445,9 +1445,9 @@ static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
> > >> set_freepointer(s, object, NULL);
> > >> } else {
> > >> slab_err(s, slab, "Freepointer corrupt");
> > >> + slab_fix(s, "Freelist cleared");
> > >> slab->freelist = NULL;
> > >> slab->inuse = slab->objects;
> > >> - slab_fix(s, "Freelist cleared");
> > >> return 0;
> > >> }
> > >> break;
> > >> @@ -1464,14 +1464,14 @@ static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
> > >> if (slab->objects != max_objects) {
> > >> slab_err(s, slab, "Wrong number of objects. Found %d but should be %d",
> > >> slab->objects, max_objects);
> > >> - slab->objects = max_objects;
> > >> slab_fix(s, "Number of objects adjusted");
> > >> + slab->objects = max_objects;
> > >> }
> > >> if (slab->inuse != slab->objects - nr) {
> > >> slab_err(s, slab, "Wrong object count. Counter is %d but counted were %d",
> > >> slab->inuse, slab->objects - nr);
> > >> - slab->inuse = slab->objects - nr;
> > >> slab_fix(s, "Object count adjusted");
> > >> + slab->inuse = slab->objects - nr;
> > >> }
> > >> return search == NULL;
> > >> }
> > >> --
> > >> 2.48.0
> > >>
> > >>
> >
> >
>
