| From a0fcaaed0c46cf9399d3a2d6e0c87ddb3df0e044 Mon Sep 17 00:00:00 2001 |
| From: "Steven Rostedt (Google)" <rostedt@goodmis.org> |
| Date: Thu, 29 Sep 2022 10:49:09 -0400 |
| Subject: ring-buffer: Fix race between reset page and reading page |
| |
| From: Steven Rostedt (Google) <rostedt@goodmis.org> |
| |
| commit a0fcaaed0c46cf9399d3a2d6e0c87ddb3df0e044 upstream. |
| |
| The ring buffer is broken up into sub buffers (currently of page size). |
| Each sub buffer has a pointer to its "tail" (the last event written to the |
| sub buffer). When a new event is requested, the tail is locally |
| incremented to cover the size of the new event. This is done in a way that |
| there is no need for locking. |
| |
| If the tail goes past the end of the sub buffer, the process of moving to |
| the next sub buffer takes place. After setting the current sub buffer to |
| the next one, the previous one that had the tail go passed the end of the |
| sub buffer needs to be reset back to the original tail location (before |
| the new event was requested) and the rest of the sub buffer needs to be |
| "padded". |
| |
| The race happens when a reader takes control of the sub buffer. As readers |
| do a "swap" of sub buffers from the ring buffer to get exclusive access to |
| the sub buffer, it replaces the "head" sub buffer with an empty sub buffer |
| that goes back into the writable portion of the ring buffer. This swap can |
| happen as soon as the writer moves to the next sub buffer and before it |
| updates the last sub buffer with padding. |
| |
| Because the sub buffer can be released to the reader while the writer is |
| still updating the padding, it is possible for the reader to see the event |
| that goes past the end of the sub buffer. This can cause obvious issues. |
| |
| To fix this, add a few memory barriers so that the reader definitely sees |
| the updates to the sub buffer, and also waits until the writer has put |
| back the "tail" of the sub buffer back to the last event that was written |
| on it. |
| |
| To be paranoid, it will only spin for 1 second, otherwise it will |
| warn and shutdown the ring buffer code. 1 second should be enough as |
| the writer does have preemption disabled. If the writer doesn't move |
| within 1 second (with preemption disabled) something is horribly |
| wrong. No interrupt should last 1 second! |
| |
| Link: https://lore.kernel.org/all/20220830120854.7545-1-jiazi.li@transsion.com/ |
| Link: https://bugzilla.kernel.org/show_bug.cgi?id=216369 |
| Link: https://lkml.kernel.org/r/20220929104909.0650a36c@gandalf.local.home |
| |
| Cc: Ingo Molnar <mingo@kernel.org> |
| Cc: Andrew Morton <akpm@linux-foundation.org> |
| Cc: stable@vger.kernel.org |
| Fixes: c7b0930857e22 ("ring-buffer: prevent adding write in discarded area") |
| Reported-by: Jiazi.Li <jiazi.li@transsion.com> |
| Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| kernel/trace/ring_buffer.c | 33 +++++++++++++++++++++++++++++++++ |
| 1 file changed, 33 insertions(+) |
| |
| --- a/kernel/trace/ring_buffer.c |
| +++ b/kernel/trace/ring_buffer.c |
| @@ -2191,6 +2191,9 @@ rb_reset_tail(struct ring_buffer_per_cpu |
| /* Mark the rest of the page with padding */ |
| rb_event_set_padding(event); |
| |
| + /* Make sure the padding is visible before the write update */ |
| + smp_wmb(); |
| + |
| /* Set the write back to the previous setting */ |
| local_sub(length, &tail_page->write); |
| return; |
| @@ -2202,6 +2205,9 @@ rb_reset_tail(struct ring_buffer_per_cpu |
| /* time delta must be non zero */ |
| event->time_delta = 1; |
| |
| + /* Make sure the padding is visible before the tail_page->write update */ |
| + smp_wmb(); |
| + |
| /* Set write to end of buffer */ |
| length = (tail + length) - BUF_PAGE_SIZE; |
| local_sub(length, &tail_page->write); |
| @@ -3864,6 +3870,33 @@ rb_get_reader_page(struct ring_buffer_pe |
| arch_spin_unlock(&cpu_buffer->lock); |
| local_irq_restore(flags); |
| |
| + /* |
| + * The writer has preempt disable, wait for it. But not forever |
| + * Although, 1 second is pretty much "forever" |
| + */ |
| +#define USECS_WAIT 1000000 |
| + for (nr_loops = 0; nr_loops < USECS_WAIT; nr_loops++) { |
| + /* If the write is past the end of page, a writer is still updating it */ |
| + if (likely(!reader || rb_page_write(reader) <= BUF_PAGE_SIZE)) |
| + break; |
| + |
| + udelay(1); |
| + |
| + /* Get the latest version of the reader write value */ |
| + smp_rmb(); |
| + } |
| + |
| + /* The writer is not moving forward? Something is wrong */ |
| + if (RB_WARN_ON(cpu_buffer, nr_loops == USECS_WAIT)) |
| + reader = NULL; |
| + |
| + /* |
| + * Make sure we see any padding after the write update |
| + * (see rb_reset_tail()) |
| + */ |
| + smp_rmb(); |
| + |
| + |
| return reader; |
| } |
| |