Re: [GIT PULL] ftrace: Fixes for v6.13

[Steven Rostedt <rostedt@goodmis.org>]

On Sun, 15 Dec 2024 09:23:18 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> And dammit, you must have known this code was garbage, because when I
> do a "git blame", I see your name on that test_can_verify_check()
> function.

Yes, that was added in the same commit that added the
trace_check_vprintk() code.

> 
> Basically, you are walking the va_list in invalid ways, because you
> want to use vsnprintf() in invalid ways. As part of that walk, you are
> (a) fundamentally mis-using va_list and (b) modifying the format
> string as you go along (both in the sense of actually changing the
> format string, but also switching that format pointer around).
> 
> My issue with this is that (b) was buggy, but it was a bug that came
> from a much more *fundamental* bug - the misuse of vsnprintf.   Which
> is why I'm saying "rip it out". Because your fix is fixing symptoms.
> It's not fixing the fact that you are doing things that are wrong.

I'm not disagreeing with you. I didn't like the code when I wrote it,
but I did write it because it was the only way I could stop the "%s"
bug. Last night, thinking about this, I think I have another solution
that can be added to the test_event_printk() and flag the fields that
can be printed via "%s" to make sure that they are correct *before*
processing the vsnprintf() code. That should give the same warning and
checks as the trace_check_vprintf().

I would also add a check in the vsnprintf() that uses
__get_kernel_nofault() like you suggested.

> 
> Looking up more, I also think the whole "replace "%p" with "%px" is
> broken and wrong.
> 
> Isn't every single case of '%p' in this context from a TP_printk() situation?
> 
> IOW, instead of dynamically creating a temporary buffer and adding
> that 'x' by hand, why wasn't that just a 'sed' script and done
> statically?
> 
> In fact, wouldn't *most* of the sanity checking be possible to just do
> entirely statically instead of at runtime?
> 
> Yeah, yeah, there's more than a few '%p' users, but
> 
>      git grep 'TP_printk.*%p[^A-Za-z]' | wc
> 
> shows that it's manageable. That probably misses some multi-line
> cases, but still - doing this kind of "rewrite constant string at
> runtime because we didn't do it statically" seems *wrong*.
> 
> And in this case, that wrongness was literally the cause of the bug.

I'm also OK with that. Should that be done for 6.13 or something to be
added for 6.14?

-- Steve