Message-ID: <20241214232333.3c49c1e3@gandalf.local.home>
Date: Sat, 14 Dec 2024 23:23:33 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Mark Rutland <mark.rutland@....com>, Mathieu
 Desnoyers <mathieu.desnoyers@...icios.com>, Al Viro
 <viro@...iv.linux.org.uk>, Michal Simek <monstr@...str.eu>
Subject: Re: [GIT PULL] ftrace: Fixes for v6.13

On Sat, 14 Dec 2024 20:00:57 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Sat, 14 Dec 2024 at 19:03, Steven Rostedt <rostedt@...dmis.org> wrote:
> >
> > It gets set by the persistent ring buffer code, via a call to
> > ring_buffer_last_boot_delta().  
> 
> That seems entirely broken, since it basically depends on the kernel
> being the same one. Which isn't even checked for.
> 
> Not to mention that it wouldn't work for modules anyway.
> 
> This kind of random hackery needs to DIE.
> 
> Tell people to turn of KASLR for cross-boot tracing. Or just not do it.

We are using this for ChromeOS in ChromeBooks where we have to have KASLR
on. And yes, I've documented that it has to be the same kernel to work and
will not work for modules. That has been accepted by the users.

> 
> > > If "people use stale pointers for '%s' and we actually care" is a real
> > > issue, we could very possibly teach vsnprintf() about that. The code
> > > already has a "check_pointer()" thing for just NULL pointer magic
> > > printouts.  
> >
> > The check code was added because I was sick and tired of fixing bugs in
> > trace events. People would use the TRACE_EVENT() TP_printk() like a normal
> > printk with things like:
> >
> >   TP_fast_assign(
> >       __entry->ipv6 = ptr_to_ipv6;
> >   )
> >
> >   TP_printk("ipv6=%pI6", __entry->ipv6)  
> 
> I think you are confused.
> 
> You are talking about the checks in test_event_printk(). That is fine
> - that's a sanity check of the trace fmt string at trace event
> registration time.

Ah, I used the wrong example. But it is still fixing the same bug. For %pI6
it is caught at registration time. But the trace_check_vprintk() was added
at the same time test_event_printk() to handle strings. That's because
RCU's trace events (and other trace events) pointed to static strings in
kernel data, and because the string pointer is in the ring buffer, it had
to be checked at run time and not registration time.

My mistake in using the "%pI6" as an example because, as you stated, that
is fixed at registration time. But we have the same bug with strings:

   TP_fast_assign(
       __entry->str = field->name;
   )

   TP_printk("str=%s", __entry->str)  

Which is also a bug and actually a more common bug than the "%pX" ones. But
because of RCU and other trace events that point to static strings, it has to be
tested at run time.

> 
> But that's not at all the code that the new "fix" is all about.
> 
> The new "fix" is for the disgusting horror that is
> trace_check_vprintf(), which is the "I'll take the trace format
> string, and I'll print it out partly using the regular vprintf()
> machinery, but I'll partly dig into it and do horrendous things to
> it".
> 
> And *THAT* is the disgusting code, and it only deals with '%s' (and
> the odd symbol relocation case that I think is completely broken).

The symbol relocation is a special case that just hooked into this code.

> 
> And the '%s' case we could at least partly handle in lib/vsprintf.c -
> not the odd tracing special cases (that you might as well just handle
> in test_event_printk()), but the "we can check that it doesn't fault".

Again, it was added to fix the same issue that test_event_printk(), but for
strings because there's several trace events that point to static strings,
and that will be flagged as being a bug if it was done in test_event_printk().

-- Steve

Powered by blists - more mailing lists