Re: [PATCH] mm/kmemleak: Fix sleeping function called from invalid context in kmemleak_seq_show

[Catalin Marinas <catalin.marinas@arm.com>]

On Tue, Nov 26, 2024 at 09:11:31AM +0100, Thomas Weissschuh wrote:
> On Fri, Nov 22, 2024 at 11:48:04AM +0100, Alessandro Carminati wrote:
> > On Thu, Nov 21, 2024 at 6:04 PM Sebastian Andrzej Siewior
> > <bigeasy@linutronix.de> wrote:
> > > On 2024-11-21 17:50:06 [+0100], Alessandro Carminati wrote:
> > > > On Wed, Nov 20, 2024 at 5:40 PM Sebastian Andrzej Siewior
> > > > <bigeasy@linutronix.de> wrote:
> > > > >
> > > > > On 2024-11-20 10:26:02 [-0500], Steven Rostedt wrote:
> > > > > > The "%pK" dereferences a pointer and there's some SELinux hooks attached to
> > > > > > that code. The problem is that the SELinux hooks take spinlocks. This would
> > > > > > not have been an issue if it wasn't for that "%pK" in the format.
> > > > >
> > > > > That is missing check and I think Thomas Weissschuh wanted to add it. So
> > > > > we don't call into selinux.
> > > >
> > > > Your comment confuses me a bit, as I'm unsure what Thomas is actually
> > > > working on.
> > > > Am I correct in assuming he's addressing a fix in lib/vsprintf.c to ensure
> > > > that sleeping functions aren't called, allowing these functions to work in
> > > > any context?
> > >
> > > restricted_pointer() has a check for in_hardirq() among others. This
> > > needs an additional PREEMPT_RT check.
> > > I would be actual in favour to get rid of case 1 for kptr_restrict and
> > > have either 0 or 2.
> > >
> > > > However, his mention of "This fix for kmemleak is still needed as the
> > > > pointers in the kmemleak report are useful" adds to my confusion.
> > > > Meanwhile, Steven suggests reworking SELinux to resolve the issue.
> > > > Could you clarify what you mean by "So we don't call into selinux"?
> > >
> > > This getting out of hand. By adding the PREEMPT_RT check to
> > > restricted_pointer() we don't call in selinux so the problem is gone.
> > 
> > I am really glad that now we have a clear solution, however practically
> > speaking is Thomas working on such a patch or is he working on something
> > related that does not fully solve the problem?
> 
> I am working on a change to lib/vsprintf.c. Something like the
> following:
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index c5e2ec9303c5..54ad175a22bd 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -870,6 +870,9 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
>                         return error_string(buf, end, "pK-error", spec);
>                 }
> 
> +               if (IS_ENABLED(CONFIG_PREEMPT_RT) && ...)
> +                       return error_string(buf, end, "pK-error", spec);
> +
>                 /*
>                  * Only print the real pointer value if the current
>                  * process has CAP_SYSLOG and is running with the
> 
> 
> Hoping to replace the ... with something that can detect actual
> problematic situations at runtime.
> I'm fairly new to PREEMPT_RT so need to do some digging.
> 
> > Even if he is working on a partial solution, I am happy to coordinate
> > off-list working on his own private branch
> > (or else I would just give up and review the Thomas' patchset when it is
> > out...)
> 
> Given that there is no direct interaction between your kmemleak patches
> and the vsprintf ones I don't think coordination is necessary.
> I'll Cc you for review.
> 
> Please do go ahead with your kmemleak patches.

If the above fixes the kmemleak warnings by not triggering a call into
SELinux, I'd rather not change kmemleak. I don't know how important it
is to run kmemleak with kptr_restrict=1 and get meaningful pointers.

-- 
Catalin