Messages in this thread Patch in this message | | | Date | Wed, 17 Nov 2021 18:38:27 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH] tracing: fix va_list breakage in trace_check_vprintf() |
| |
On Wed, 17 Nov 2021 21:37:20 +0300 Nikita Yushchenko <nikita.yushchenko@virtuozzo.com> wrote:
> When trace_check_vprintf() extracts parts of the format string and > passes those to trace_seq_printf() together with va_list, it expects > that trace_seq_printf() consumes arguments from va_list, as defined > by the passed format substring. > > However, trace_seq_printf() has a special path for overflow handling, > that does not consume any arguments from va_list. This causes va_list > to get out of sync with format string processing, the next va_arg() > inside trace_check_vprintf() gets wrong argument, and WARN_ON_ONCE() > hits. > > This situation easily triggers by ftrace_stress_test from LTP. > > Fix that by adding a dummy vsnprintf() call to the overflow path inside > trace_seq_printf() to ensure args from va_list are still consumed.
Hi Nikita,
> > Signed-off-by: Nikita Yushchenko <nikita.yushchenko@virtuozzo.com> > --- > kernel/trace/trace_seq.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c > index 9c90b3a7dce2..3551b5e18aa2 100644 > --- a/kernel/trace/trace_seq.c > +++ b/kernel/trace/trace_seq.c > @@ -141,9 +141,15 @@ EXPORT_SYMBOL_GPL(trace_seq_bitmask); > void trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args) > { > unsigned int save_len = s->seq.len; > + char buf[1]; > > - if (s->full) > + if (s->full) { > + /* Consume args from va_list before returning, some callers > + * expect that.
First, only the networking code uses the /* comment ... for multiline comments. The rest of the kernel uses:
/* * Consume ... */
format for multi line comments.
But regardless. Consumers do not expect va_list to be consumed if it is full. The one use case that does is buggy.
> + */ > + vsnprintf(buf, sizeof(buf), fmt, args); > return; > + } > > __trace_seq_init(s); >
The real fix is:
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index f9139dc1262c..7aa5ea5ca912 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -3654,6 +3654,10 @@ static bool trace_safe_str(struct trace_iterator *iter, const char *str) struct trace_event *trace_event; struct trace_event_call *event; + /* if seq is full, then we can't test it */ + if (iter->seq->full) + return true; + /* OK if part of the event data */ if ((addr >= (unsigned long)iter->ent) && (addr < (unsigned long)iter->ent + iter->ent_size))
Cheers,
-- Steve
| |