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

[Thomas Weissschuh <thomas.weissschuh@linutronix.de>]

Hi Alessandro,

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.

> > kmemleak is not the only problem. printk(), as another of vspritf pointer
> > code user, can be called from any place and would also trigger a
> > warning here.
> > As far as "kmemleak need to be usefull" goes: With kptr_restrict == 0
> > then with or without pointer hashing they will be useful. If we need to
> > go via selinux then it ends as a hint.


Thomas