| Message ID | 20230113125501.760324-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3e4272b9954094907f16861199728f14002fcaf6 |
| Headers | show |
| Series | trace: Add NULL checks for buffer in ring_buffer_free_read_page() | expand |
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;
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;
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> --- kernel/trace/ring_buffer.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)