Re: [PATCH v2] mm/kmemleak: Fix sleeping function called from invalid context at print message

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2024-12-18 09:38:05 [+0100], Alessandro Carminati wrote:
> On Tue, Dec 17, 2024 at 6:55 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Tue, 17 Dec 2024 14:20:33 +0000 Alessandro Carminati <acarmina@redhat.com> wrote:
> >
> > > Address a bug in the kernel that triggers a "sleeping function called from
> > > invalid context" warning when /sys/kernel/debug/kmemleak is printed under
> > > specific conditions:
> > > - CONFIG_PREEMPT_RT=y
> > > - Set SELinux as the LSM for the system
> > > - Set kptr_restrict to 1
> > > - kmemleak buffer contains at least one item
> > >
> > > BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
> > > in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 136, name: cat
> >
> > -rt is a bit annoying this way.  Things which we expect to work OK are
> > no longer doing so.

Yeah. But lockdep should have complained here earlier. The RT just
enforces that part.

> > > --- a/mm/kmemleak.c
> > > +++ b/mm/kmemleak.c
> > > @@ -373,7 +373,7 @@ static void print_unreferenced(struct seq_file *seq,
> > >
> > >       for (i = 0; i < nr_entries; i++) {
> > >               void *ptr = (void *)entries[i];
> > > -             warn_or_seq_printf(seq, "    [<%pK>] %pS\n", ptr, ptr);
> > > +             warn_or_seq_printf(seq, "    %pS\n", ptr);
> > >       }
> > >  }
> >
> > Before 3a6f33d86baa we were still printing the address, with plain old
> > %p.  Should we restore that, or is %p always useless?
> >
> 
> The intent of 3a6f33d86baa8 is to make pointers available for debugging
> purposes.
> My argument for this change is that %pS provides already sufficient
> information for debugging purposes.
> 
> We essentially have two scenarios concerning %pS:
> 1. When the symbol can be resolved by kallsyms: In this case, a
>    symbol+offset style pointer is emitted, which I find more useful than
>    just the address.

indeed. With the relocation the information is usually hard to make
sense.

> 2. When the symbol cannot be resolved by kallsyms: Here, the raw pointer
>    is emitted. This undermines the original intent of %pK, as both the
>    masked address and the plain address are exposed.
> 
> If we want to be picky, there's an additional factor to consider: KASLR.
> %pS emits its output accounting for randomization, but in most cases, the
> randomized offset information is lost.
> The exception is when the symbol cannot be resolved by kallsyms, though
> this should be rare. It's still worth mentioning.
> 
> Back to your question: %p emits an hashed address that is not
> reversible by design.
> Therefore, in my opinion, it is not particularly useful in a debug
> message.

There is still %px if you want your debug message to show the raw
pointer regardless of pointer hashing. That is something that you could
disable. The %pK format is something that was meant to be used files
like proc or sysfs. With the restrictions you already have on dmesg it
does not seem necessary to have yet another check.

Thank you the patch.

Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Sebastian