Re: [PATCH] ring-buffer: Check for NULL cpu_buffer in ring_buffer_wake_waiters()

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Linux trace kernel <linux-trace-kernel@vger.kernel.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Roland Ruckerbauer <roland.rucky@gmail.com>,
	Takashi Iwai <tiwai@suse.de>,
	regressions@lists.linux.dev,
	Steven Noonan <steven.noonan@gmail.com>
Subject: Re: [PATCH] ring-buffer: Check for NULL cpu_buffer in ring_buffer_wake_waiters()
Date: Thu, 3 Nov 2022 11:53:07 +0900	[thread overview]
Message-ID: <20221103115307.3d253be73d1259dfa6417b3c@kernel.org> (raw)
In-Reply-To: <20221101191009.1e7378c8@rorschach.local.home>

On Tue, 1 Nov 2022 19:10:09 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> On some machines the number of listed CPUs may be bigger than the actual
> CPUs that exist. The tracing subsystem allocates a per_cpu directory with
> access to the per CPU ring buffer via a cpuX file. But to save space, the
> ring buffer will only allocate buffers for online CPUs, even though the
> CPU array will be as big as the nr_cpu_ids.
> 
> With the addition of waking waiters on the ring buffer when closing the
> file, the ring_buffer_wake_waiters() now needs to make sure that the
> buffer is allocated (with the irq_work allocated with it) before trying to
> wake waiters, as it will cause a NULL pointer dereference.
> 
> While debugging this, I added a NULL check for the buffer itself (which is
> OK to do), and also NULL pointer checks against buffer->buffers (which is
> not fine, and will WARN) as well as making sure the CPU number passed in
> is within the nr_cpu_ids (which is also not fine if it isn't).
> 
> Link: https://lore.kernel.org/all/87h6zklb6n.wl-tiwai@suse.de/
> Link: https://lore.kernel.org/all/CAM6Wdxc0KRJMXVAA0Y=u6Jh2V=uWB-_Fn6M4xRuNppfXzL1mUg@mail.gmail.com/
> 

This looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks,

> Cc: stable@vger.kernel.org
> Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=1204705
> Reported-by: Takashi Iwai <tiwai@suse.de>
> Reported-by: Roland Ruckerbauer <roland.rucky@gmail.com>
> Fixes: f3ddb74ad079 ("tracing: Wake up ring buffer waiters on closing of the file")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  kernel/trace/ring_buffer.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 199759c73519..9712083832f4 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -937,6 +937,9 @@ void ring_buffer_wake_waiters(struct trace_buffer *buffer, int cpu)
>  	struct ring_buffer_per_cpu *cpu_buffer;
>  	struct rb_irq_work *rbwork;
>  
> +	if (!buffer)
> +		return;
> +
>  	if (cpu == RING_BUFFER_ALL_CPUS) {
>  
>  		/* Wake up individual ones too. One level recursion */
> @@ -945,7 +948,15 @@ void ring_buffer_wake_waiters(struct trace_buffer *buffer, int cpu)
>  
>  		rbwork = &buffer->irq_work;
>  	} else {
> +		if (WARN_ON_ONCE(!buffer->buffers))
> +			return;
> +		if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> +			return;
> +
>  		cpu_buffer = buffer->buffers[cpu];
> +		/* The CPU buffer may not have been initialized yet */
> +		if (!cpu_buffer)
> +			return;
>  		rbwork = &cpu_buffer->irq_work;
>  	}
>  
> -- 
> 2.35.1
> 

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>