Messages in this thread Patch in this message | | | Date | Tue, 24 Jan 2023 12:44:03 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH] trace: Add NULL checks for buffer in ring_buffer_free_read_page() |
| |
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;
| |