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