From: Steven Rostedt <rostedt@goodmis.org>
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: kernel test robot <rong.a.chen@intel.com>,
linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Peter Zijlstra <peterz@infradead.org>,
Alexei Starovoitov <alexei.starovoitov@gmail.com>,
Peter Wu <peter@lekensteyn.nl>, Jonathan Corbet <corbet@lwn.net>,
Tom Zanussi <zanussi@kernel.org>,
Shuah Khan <shuahkhan@gmail.com>, bpf <bpf@vger.kernel.org>,
lkp@lists.01.org
Subject: Re: [tracing] cd8f62b481: BUG:sleeping_function_called_from_invalid_context_at_mm/slab.h
Date: Fri, 3 Apr 2020 09:16:42 -0400 [thread overview]
Message-ID: <20200403091642.5ce182f1@gandalf.local.home> (raw)
In-Reply-To: <20200403154702.bc3478c84d70fb48b07d9985@kernel.org>
On Fri, 3 Apr 2020 15:47:02 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > +#define STATIC_TEMP_BUF_SIZE 128
> > +static char static_temp_buf[STATIC_TEMP_BUF_SIZE];
> > +
> > /* Find the next real entry, without updating the iterator itself */
> > struct trace_entry *trace_find_next_entry(struct trace_iterator *iter,
> > int *ent_cpu, u64 *ent_ts)
> > @@ -3480,13 +3483,26 @@ struct trace_entry *trace_find_next_entry(struct trace_iterator *iter,
> > int ent_size = iter->ent_size;
> > struct trace_entry *entry;
> >
> > + /*
> > + * If called from ftrace_dump(), then the iter->temp buffer
> > + * will be the static_temp_buf and not created from kmalloc.
> > + * If the entry size is greater than the buffer, we can
> > + * not save it. Just return NULL in that case. This is only
> > + * used to add markers when two consecutive events' time
> > + * stamps have a large delta. See trace_print_lat_context()
> > + */
> > + if (iter->temp == static_temp_buf &&
> > + STATIC_TEMP_BUF_SIZE < ent_size)
> > + return NULL;
> > +
> > /*
> > * The __find_next_entry() may call peek_next_entry(), which may
> > * call ring_buffer_peek() that may make the contents of iter->ent
> > * undefined. Need to copy iter->ent now.
> > */
> > if (iter->ent && iter->ent != iter->temp) {
> > - if (!iter->temp || iter->temp_size < iter->ent_size) {
> > + if ((!iter->temp || iter->temp_size < iter->ent_size) &&
> > + !WARN_ON_ONCE(iter->temp == static_temp_buf)) {
>
> This must not happen because ent_size == iter->ent_size.
> If it happens, it should return NULL without any trial of kfree() and
> kmalloc(), becuase it will cause illegal freeing memory and memory leak.
> (Note that the iter->temp never be freed in ftrace_dump() path)
Correct, which is why there's a ! in there. It's a paranoid check which
should never trigger, which is why there's a WARN_ON_ONCE() there. But as
the "!" is not easy to see, the above is the same logic as:
if ((!iter->temp || iter->temp_size < iter->ent_size) &&
(iter->temp != static_temp_buf)) {
Thus, if we get to that test against static_temp_buf, and it's true, then
we will trigger the WARN_ON, but it wont call the kfree().
>
> Anyway, this condition is completery same as above return code.
>
> > kfree(iter->temp);
> > iter->temp = kmalloc(iter->ent_size, GFP_KERNEL);
> > if (!iter->temp)
> > @@ -9203,6 +9219,8 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
> >
> > /* Simulate the iterator */
> > trace_init_global_iter(&iter);
> > + /* Can not use kmalloc for iter.temp */
> > + iter.temp = static_temp_buf;
> >
>
> You may miss initializing temp_size here.
>
> iter.temp_size = STATIC_TEMP_BUF_SIZE;
Oh, damn! You're right.
>
> BTW, as I pointed, if the iter->temp is for avoiding the data overwritten
> by ringbuffer writer, would we need to use it for ftrace_dump() too?
> It seems that ftrace_dump() stops tracing.
Yes, it is still needed. That's because the old way use to just leave the
iter->ent pointing into the ring buffer itself. The new way, the ring
buffer makes a copy of the event, and passes that back. When you do another
read, it overwrites the copy. It doesn't matter if the ring buffer is
stopped or not.
-- Steve
next prev parent reply other threads:[~2020-04-03 13:16 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-19 23:22 [PATCH 00/12 v2] ring-buffer/tracing: Remove disabling of ring buffer while reading trace file Steven Rostedt
2020-03-19 23:22 ` [PATCH 01/12 v2] selftest/ftrace: Fix function trigger test to handle trace not disabling the tracer Steven Rostedt
2020-03-19 23:22 ` [PATCH 02/12 v2] tracing: Save off entry when peeking at next entry Steven Rostedt
2020-03-20 2:57 ` Masami Hiramatsu
2020-03-26 9:12 ` [tracing] cd8f62b481: BUG:sleeping_function_called_from_invalid_context_at_mm/slab.h kernel test robot
2020-04-01 14:07 ` Masami Hiramatsu
2020-04-01 14:21 ` Steven Rostedt
2020-04-01 15:04 ` Steven Rostedt
2020-04-02 7:19 ` Masami Hiramatsu
2020-04-02 18:14 ` Steven Rostedt
2020-04-03 6:47 ` Masami Hiramatsu
2020-04-03 13:16 ` Steven Rostedt [this message]
2020-03-19 23:22 ` [PATCH 03/12 v2] ring-buffer: Have ring_buffer_empty() not depend on tracing stopped Steven Rostedt
2020-03-19 23:22 ` [PATCH 04/12 v2] ring-buffer: Rename ring_buffer_read() to read_buffer_iter_advance() Steven Rostedt
2020-03-19 23:22 ` [PATCH 05/12 v2] ring-buffer: Add page_stamp to iterator for synchronization Steven Rostedt
2020-03-19 23:22 ` [PATCH 06/12 v2] ring-buffer: Have rb_iter_head_event() handle concurrent writer Steven Rostedt
2020-03-19 23:22 ` [PATCH 07/12 v2] ring-buffer: Do not die if rb_iter_peek() fails more than thrice Steven Rostedt
2020-03-19 23:22 ` [PATCH 08/12 v2] ring-buffer: Optimize rb_iter_head_event() Steven Rostedt
2020-03-19 23:22 ` [PATCH 09/12 v2] ring-buffer: Do not disable recording when there is an iterator Steven Rostedt
2020-03-19 23:22 ` [PATCH 10/12 v2] tracing: Do not disable tracing when reading the trace file Steven Rostedt
2020-03-19 23:22 ` [PATCH 11/12 v2] ring-buffer/tracing: Have iterator acknowledge dropped events Steven Rostedt
2020-03-19 23:22 ` [PATCH 12/12 v2] tracing: Have the document reflect that the trace file keeps tracing enabled Steven Rostedt
2020-03-21 19:13 ` [PATCH 00/12 v2] ring-buffer/tracing: Remove disabling of ring buffer while reading trace file David Laight
2020-03-22 18:07 ` Steven Rostedt
2020-03-27 1:46 ` Steven Rostedt
2020-03-27 10:07 ` David Laight
2020-03-27 14:30 ` Steven Rostedt
2020-03-27 14:56 ` David Laight
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200403091642.5ce182f1@gandalf.local.home \
--to=rostedt@goodmis.org \
--cc=akpm@linux-foundation.org \
--cc=alexei.starovoitov@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=corbet@lwn.net \
--cc=linux-kernel@vger.kernel.org \
--cc=lkp@lists.01.org \
--cc=mhiramat@kernel.org \
--cc=mingo@kernel.org \
--cc=peter@lekensteyn.nl \
--cc=peterz@infradead.org \
--cc=rong.a.chen@intel.com \
--cc=shuahkhan@gmail.com \
--cc=zanussi@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).