[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9ceea6aa-b07c-fb10-0954-5e3d188bfd7e@huawei.com>
Date: Thu, 9 Feb 2023 11:50:59 +0800
From: Zheng Yejian <zhengyejian1@...wei.com>
To: Steven Rostedt <rostedt@...dmis.org>
CC: <mhiramat@...nel.org>, <linux-kernel@...r.kernel.org>,
<linux-trace-kernel@...r.kernel.org>, <wanghai38@...wei.com>
Subject: Re: [PATCH] tracing/ring-buffer: Remove integrity check at end of
iter read
On 2023/2/9 06:36, Steven Rostedt wrote:
> On Wed, 8 Feb 2023 17:08:14 +0800
> Zheng Yejian <zhengyejian1@...wei.com> wrote:
>
>> Concurrently closing "trace" file and writing into ring buffer [1] can
>> cause WARNINGs [2]. It has been reported in
>> Link: https://lore.kernel.org/all/20230203035608.2336906-1-zhengyejian1@huawei.com/
>>
>> It seems a data race between ring_buffer writing and integrity check.
>> That is, RB_FLAG of head_page is been updating, while at same time RB_FLAG
>> was cleared when doing integrity check:
>> rb_check_pages() rb_handle_head_page():
>> -------- --------
>> rb_head_page_deactivate()
>> rb_head_page_set_normal()
>> rb_head_page_activate()
>>
>
> Good catch!
Thanks!
>
>> Integrity check at end of iter read was added since commit 659f451ff213
>> ("ring-buffer: Add integrity check at end of iter read"). As it's commit
>> message said:
>> > As reading via an iterator requires disabling the ring buffer, it
>> > is a perfect place to have it.
>> However, since commit 1039221cc278 ("ring-buffer: Do not disable recording
>> when there is an iterator"), ring buffer was not disabled at that place,
>> so that integrity check should be removed.
>>
>> 1:
>> ``` read_trace.sh
>> while true;
>> do
>> # the "trace" file is closed after read
>> head -1 /sys/kernel/tracing/trace > /dev/null
>> done
>> ```
>> ``` repro.sh
>> sysctl -w kernel.panic_on_warn=1
>> # function tracer will writing enough data into ring_buffer
>> echo function > /sys/kernel/tracing/current_tracer
>> ./read_trace.sh &
>> ./read_trace.sh &
>> ./read_trace.sh &
>> ./read_trace.sh &
>> ./read_trace.sh &
>> ./read_trace.sh &
>> ./read_trace.sh &
>> ./read_trace.sh &
>> ```
>>
>
>
>> Fixes: 1039221cc278 ("ring-buffer: Do not disable recording when there is an iterator")
>> Signed-off-by: Zheng Yejian <zhengyejian1@...wei.com>
>> ---
>> kernel/trace/ring_buffer.c | 11 -----------
>> 1 file changed, 11 deletions(-)
>>
>> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
>> index c366a0a9ddba..34e955bd1e59 100644
>> --- a/kernel/trace/ring_buffer.c
>> +++ b/kernel/trace/ring_buffer.c
>> @@ -5203,17 +5203,6 @@ void
>> ring_buffer_read_finish(struct ring_buffer_iter *iter)
>> {
>> struct ring_buffer_per_cpu *cpu_buffer = iter->cpu_buffer;
>> - unsigned long flags;
>> -
>> - /*
>> - * Ring buffer is disabled from recording, here's a good place
>> - * to check the integrity of the ring buffer.
>> - * Must prevent readers from trying to read, as the check
>> - * clears the HEAD page and readers require it.
>> - */
>> - raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
>> - rb_check_pages(cpu_buffer);
>> - raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
>
> I would rather find a way to make this still work than just removing it.
Yes, we can try to find the way.
>
> Perhaps there's no reason to clear the flags, and change rb_check_pages()
> to mask them out before testing. Something like:
>
> static int rb_check_pages(struct ring_buffer_per_cpu *cpu_buffer)
> {
> struct list_head *head = cpu_buffer->pages;
> struct buffer_page *bpage, *tmp;
>
> if (RB_WARN_ON(cpu_buffer, rb_list_head(rb_list_head(head->next)->prev) != head))
> return -1;
> if (RB_WARN_ON(cpu_buffer, rb_list_head(rb_list_head(head->prev)->next) != head))
> return -1;
>
> if (rb_check_list(cpu_buffer, head))
rb_check_list() expect to check a page with RB_FLAG being cleared,
but in this solution, rb_head_page_deactivate() is not called before,
so we may not call it directly? The same problem with below check for
"bpage->list".
> return -1;
>
> list_for_each_entry_safe(bpage, tmp, head, list) {
I'd like to know if there is a case that "head" happens to be a
"reader_page", and the ring buffer is not exactly being traversed?
> if (RB_WARN_ON(cpu_buffer,
> rb_list_head(rb_list_head(bpage->list.next)->prev) != &bpage->list))
> return -1;
> if (RB_WARN_ON(cpu_buffer,
> rb_list_head(rb_list_head(bpage->list.prev)->next) != &bpage->list))
> return -1;
> if (rb_check_list(cpu_buffer, &bpage->list))
> return -1;
> }
>
> return 0;
> }
>
> I haven't tested the above.
>
> ?
>
> -- Steve
>
>
>>
>> atomic_dec(&cpu_buffer->resize_disabled);
>> kfree(iter->event);
>
Powered by blists - more mailing lists