Date:   Tue, 24 Jan 2023 12:44:03 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Jia-Ju Bai <baijiaju1990@gmail.com>
Cc:     mhiramat@kernel.org, linux-kernel@vger.kernel.org,
        linux-trace-kernel@vger.kernel.org,
        TOTE Robot <oslab@tsinghua.edu.cn>
Subject: Re: [PATCH] trace: Add NULL checks for buffer in
 ring_buffer_free_read_page()
Message-ID: <20230124124403.2727b6bf@gandalf.local.home>
In-Reply-To: <20230113125501.760324-1-baijiaju1990@gmail.com>
References: <20230113125501.760324-1-baijiaju1990@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 13 Jan 2023 20:55:01 +0800
Jia-Ju Bai <baijiaju1990@gmail.com> wrote:

> In a previous commit 7433632c9ff6, buffer, buffer->buffers and 
> buffer->buffers[cpu] in ring_buffer_wake_waiters() can be NULL, 
> and thus the related checks are added.
> 
> However, in the same call stack, these variables are also used in
> ring_buffer_free_read_page():
> 
> tracing_buffers_release()
>   ring_buffer_wake_waiters(iter->array_buffer->buffer)
>     cpu_buffer = buffer->buffers[cpu] -> Add checks by previous commit
>   ring_buffer_free_read_page(iter->array_buffer->buffer)
>     cpu_buffer = buffer->buffers[cpu] -> No check
> 
> Thus, to avod possible null-pointer derefernces, the related checks
> should be added.
> 
> These results are reported by a static tool designed by myself.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>

Although I'm not adverse to this patch (it doesn't hurt), but the code has:

	ring_buffer_wake_waiters(iter->array_buffer->buffer, iter->cpu_file);

	if (info->spare)
		ring_buffer_free_read_page(iter->array_buffer->buffer,
					   info->spare_cpu, info->spare);

Where none of those will be NULL if "info->spare" is set.

But as I said, I have no problem applying this, so I will ;-)

-- Steve


> ---
>  kernel/trace/ring_buffer.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index c366a0a9ddba..45d4a23d6044 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -5626,11 +5626,16 @@ EXPORT_SYMBOL_GPL(ring_buffer_alloc_read_page);
>   */
>  void ring_buffer_free_read_page(struct trace_buffer *buffer, int cpu, void *data)
>  {
> -	struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
> +	struct ring_buffer_per_cpu *cpu_buffer;
>  	struct buffer_data_page *bpage = data;
>  	struct page *page = virt_to_page(bpage);
>  	unsigned long flags;
>  
> +	if (!buffer || !buffer->buffers || !buffer->buffers[cpu])
> +		return;
> +
> +	cpu_buffer = buffer->buffers[cpu];
> +
>  	/* If the page is still in use someplace else, we can't reuse it */
>  	if (page_ref_count(page) > 1)
>  		goto out;