diff mbox series

ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

Message ID tencent_5EC64EB49686EE61593AE541DB14CE490A08@qq.com
State New
Headers
Series ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment |

Commit Message

linke li Feb. 25, 2024, 3:05 a.m. UTC 
  In function ring_buffer_iter_empty(), cpu_buffer->commit_page and
curr_commit_page->page->time_stamp is read using READ_ONCE() in 
line 4354, 4355

4354    curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
4355    curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);

while they are read directly in line 4340, 4341

4340    commit_page = cpu_buffer->commit_page;
4341    commit_ts = commit_page->page->time_stamp;

There is patch similar to this. commit c1c0ce31b242 ("r8169: fix the KCSAN reported data-race in rtl_tx() while reading tp->cur_tx")
This patch find two read of same variable while one is protected, another
is not. And READ_ONCE() is added to protect.

Signed-off-by: linke li <lilinke99@qq.com>
---
 kernel/trace/ring_buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Steven Rostedt Feb. 25, 2024, 8:03 p.m. UTC | #1 
On Sun, 25 Feb 2024 11:05:06 +0800
linke li <lilinke99@qq.com> wrote:

> In function ring_buffer_iter_empty(), cpu_buffer->commit_page and
> curr_commit_page->page->time_stamp is read using READ_ONCE() in 
> line 4354, 4355
> 
> 4354    curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
> 4355    curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);
> 
> while they are read directly in line 4340, 4341
> 
> 4340    commit_page = cpu_buffer->commit_page;
> 4341    commit_ts = commit_page->page->time_stamp;

Just because it's used in one place does not mean it's required in
another.

> 
> There is patch similar to this. commit c1c0ce31b242 ("r8169: fix the KCSAN reported data-race in rtl_tx() while reading tp->cur_tx")
> This patch find two read of same variable while one is protected, another
> is not. And READ_ONCE() is added to protect.
> 

Here's the entire code:

        cpu_buffer = iter->cpu_buffer;
        reader = cpu_buffer->reader_page;
        head_page = cpu_buffer->head_page;
        commit_page = cpu_buffer->commit_page;
        commit_ts = commit_page->page->time_stamp;

        /*
         * When the writer goes across pages, it issues a cmpxchg which
         * is a mb(), which will synchronize with the rmb here.
         * (see rb_tail_page_update())
         */
        smp_rmb();

The above smp_rmb() is a full read barrier. The commit_page and
timestamp are not going to be read again after this.

        commit = rb_page_commit(commit_page);
        /* We want to make sure that the commit page doesn't change */
        smp_rmb();

        /* Make sure commit page didn't change */
        curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
        curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);

Now the reason for the above READ_ONCE() is because the variables *are*
going to be used again. We do *not* want the compiler to play any games
with that.

Thus, the first read of commit_page and time_stamp are read properly as
the compiler will not do anything that can hurt us beyond that
smp_rmb(). The second time we read those variables, we are using them
in the below code.


        /* If the commit page changed, then there's more data */
        if (curr_commit_page != commit_page ||
            curr_commit_ts != commit_ts)
                return 0;

        /* Still racy, as it may return a false positive, but that's OK */
        return ((iter->head_page == commit_page && iter->head >= commit) ||
                (iter->head_page == reader && commit_page == head_page &&
                 head_page->read == commit &&
                 iter->head == rb_page_commit(cpu_buffer->reader_page)));
}

*But* looking at this deeper, the commit_page may need a READ_ONCE()
but not for the reason you suggested.

        commit_page = cpu_buffer->commit_page;
        commit_ts = commit_page->page->time_stamp;

The commit_page above *is* used again, and we want commit_ts to be part
of the commit_page that was originally read and not a second reading.

So, I think for the commit_page we do need a READ_ONCE() but that's
because it is referenced again just below it and we don't want the
compiler to read the memory location again for the second reference.

-- Steve
Steven Rostedt Feb. 26, 2024, 6:53 p.m. UTC | #2 
On Sun, 25 Feb 2024 15:03:02 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> *But* looking at this deeper, the commit_page may need a READ_ONCE()
> but not for the reason you suggested.
> 
>         commit_page = cpu_buffer->commit_page;
>         commit_ts = commit_page->page->time_stamp;
> 
> The commit_page above *is* used again, and we want commit_ts to be part
> of the commit_page that was originally read and not a second reading.
> 
> So, I think for the commit_page we do need a READ_ONCE() but that's
> because it is referenced again just below it and we don't want the
> compiler to read the memory location again for the second reference.

Care to send a v2 patch that just adds READ_ONCE() for the commit_page, and
change the change log stating that it is to fix the possibility that the
time_stamp may come from a different page.

-- Steve
diff mbox series

Patch

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 0699027b4f4c..eb3fa629b837 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -4337,8 +4337,8 @@  int ring_buffer_iter_empty(struct ring_buffer_iter *iter)
 	cpu_buffer = iter->cpu_buffer;
 	reader = cpu_buffer->reader_page;
 	head_page = cpu_buffer->head_page;
-	commit_page = cpu_buffer->commit_page;
-	commit_ts = commit_page->page->time_stamp;
+	commit_page = READ_ONCE(cpu_buffer->commit_page);
+	commit_ts = READ_ONCE(commit_page->page->time_stamp);
 
 	/*
 	 * When the writer goes across pages, it issues a cmpxchg which