Re: [PATCH] mm: slub: Panic if the object corruption is checked.

[Hyesoo Yu <hyesoo.yu@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 2872 bytes --]

On Tue, Jan 21, 2025 at 10:48:08AM +0900, Hyeonggon Yoo wrote:
> 
> 
> On 1/21/2025 9:54 AM, Hyesoo Yu wrote:
> > On Tue, Jan 21, 2025 at 12:41:01AM +0900, Hyeonggon Yoo wrote:
> > > On Mon, Jan 20, 2025 at 5:30 PM Hyesoo Yu <hyesoo.yu@samsung.com> 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. A flag has been added that can cause a panic when there
> > > > is a problem with the object.
> > > 
> > > Hi Hyesoo,
> > > 
> > > I'm concerned about this because it goes against the effort to avoid
> > > introducing new BUG() calls [1].
> > > 
> > > And I think it would be more appropriate to use existing panic_on_warn
> > > functionality [2] which causes
> > > a panic on WARN(), rather than introducing a SLUB-specific knob to do
> > > the same thing.
> > > 
> > > However SLUB does not call WARN() and uses pr_err() instead when
> > > reporting an error.
> > > Vlastimil and I talked about changing it to use WARN() a while ago
> > > [3], but neither of us
> > > have done that yet.
> > > 
> > > Probably you may want to look at it, as it also aligns with your purpose?
> > > FYI, if you would like to work on it, please make sure that it WARN()
> > > is suppressed during kunit test.
> > > 
> > > [1] https://docs.kernel.org/process/deprecated.html#bug-and-bug-on
> > > [2] https://www.kernel.org/doc/html/v6.9/admin-guide/sysctl/kernel.html#panic-on-warn
> > > [3] https://lore.kernel.org/linux-mm/d4219cd9-32d3-4697-93b9-6a44bf77d50c@suse.cz
> > > 
> > > Best,
> > > Hyeonggon
> > 
> > Thanks for response.
> > 
> > Using warn() instead of panic, is a great idea.
> > Thanks for pointing out what I missed.
> 
> Just for clarification, I think changing the common error reporting
> logic (like, slab_bug()) to use WARN() will be preferable to inserting
> new WARN()s at random points, which is what this patch does now.
> 
> Best,
> Hyeonggon
> 

Thanks you for clarification.

Actually, I considered adding BUG_ON() to slab_bug. However if we add BUG_ON() to slab_bug,
it will prevent many meaningful error log from being printed subsequently.
As you know, slab_bug is the log that usually is printed at the biginning of a bug
in the slab. As a result, it would be difficult to figure out the problems based on
the logs during our large-scale test-bed.

Similary, even if I use WARN() in slab_bug, we won't be able to obtain the logs
when panic_on_warn is enabled. I don't think it is useful to include WARN in slab_bug.

Instead, I will implement a solution where WARN is only used in slab_fix before
slab object is restored. If I add it to slab_fix, I think warning is suppressed on
kunit test by slab_add_kunit_errors handling.

Thanks,
Regards.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]