linux-kernel - Re: races when reserving an event in the unified trace buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <5df78e1d0812221555r1af88b05x8d5f3e0b066b4623@mail.gmail.com>
Date:	Mon, 22 Dec 2008 15:55:16 -0800
From:	Jiaying Zhang <jiayingz@...gle.com>
To:	Steven Rostedt <srostedt@...hat.com>
Cc:	linux-kernel@...r.kernel.org, Michael Rubin <mrubin@...gle.com>,
	Michael Davidson <md@...gle.com>,
	Martin Bligh <mbligh@...gle.com>,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: races when reserving an event in the unified trace buffer

On Mon, Dec 22, 2008 at 3:42 PM, Steven Rostedt <srostedt@...hat.com> wrote:
> Hi Jiaying,
>
> Again, sorry for taking so long to get back to you on this.
Again. Thank you for looking at the problem.

>
> On Wed, 2008-12-17 at 16:00 -0800, Jiaying Zhang wrote:
>> Hi Steve,
>>
>> I mentioned in my last email that I saw the warning about the trace buffer
>> became full because of an interrupt storm even with my posted patch applied.
>> After adding more debugging messages, I found that the problem was actually
>> caused by another race in the code.
>
> Actually, the other email did find a bug.
>
>
>>  I saw the events in the buffer were
>> not just interrupts but include other kernel events as well. Looks
>> like the commit
>> page failed to advance because of a race between the update of tail_page in
>> __rb_reserve_next_event and the following lines in the rb_set_commit_to_write:
>
>>         while (cpu_buffer->commit_page != cpu_buffer->tail_page) {
>>                 cpu_buffer->commit_page->page->commit =
>>                         cpu_buffer->commit_page->write;
>>                 rb_inc_page(cpu_buffer, &cpu_buffer->commit_page);
>>                 ...
>>         }
>
> You are saying an interrupt happens here that pushes the tail page one.
>
>>         while (rb_commit_index(cpu_buffer) !=
>>                rb_page_write(cpu_buffer->commit_page)) {
>>                 cpu_buffer->commit_page->page->commit =
>>                         cpu_buffer->commit_page->write;
>>                 barrier();
>>         }
>
> Now here we are left with the tail page moved one forward but the commit
> left back one, and thus we do not increment the commit.

Actually, I was saying that an interrupt happens after the last loop
and we are left with the tail page moved one forward but the 'commit page'
left back one. The function still updates 'commit' pointer correctly after
the interrupt because it is the commit event, but it does not update the
'commit page' since it already jumped out from the last loop.

>
>
>
>>
>> The problem is that an interrupt can happen right after a kernel event finishes
>> the condition check "cpu_buffer->commit_page != cpu_buffer->tail_page" but
>> before it updates the commit value of the commit_page. If we were at the tail
>> of the tail_page and the commit_page was the same as the tail_page when
>> the first kernel event checked that, neither of the events would advance the
>> commit_page pointer because the interrupt event was NOT the commit event
>> before the kernel event updated the commit pointer and the kernel event did
>> NOT see the change of the tail_page made by the interrupt event. Once we
>> got into this situation, the trace buffer would soon become full and reject
>> any further reservation requests.
>>
>> A possible fix I think is to update the commit pointer of the commit_page
>> both before and after updating the commit_page in rb_set_commit_to_write().
>> Here is the proposed fix. Please let me know if my analysis makes sense
>> to you. Thanks a lot!
>>
>> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
>> index 7f69cfe..b345ba7 100644
>> --- a/kernel/trace/ring_buffer.c
>> +++ b/kernel/trace/ring_buffer.c
>> @@ -839,6 +839,12 @@ rb_set_commit_to_write(struct ring_buffer_per_cpu
>> *cpu_buffer)
>>          * back to us). This allows us to do a simple loop to
>>          * assign the commit to the tail.
>>          */
>> +       while (rb_commit_index(cpu_buffer) !=
>> +              rb_page_write(cpu_buffer->commit_page)) {
>> +               cpu_buffer->commit_page->page->commit =
>> +                       cpu_buffer->commit_page->write;
>> +               barrier();
>> +       }
>
> I do not think this solves it for us. Because the interrupt comes in
> after the next check, and this is where we are left with the dangling
> commit.
>
>>         while (cpu_buffer->commit_page != cpu_buffer->tail_page) {
>>                 cpu_buffer->commit_page->page->commit =
>>                         cpu_buffer->commit_page->write;
>
> What we need is to add do while over the entire code with:
>
> do {
>        [...]
> } while (cpu_buffer->commit_page != cpu_buffer->tail_page);
>
I was thinking to use this kind of loop but has not come up with
one that looks clean and nice. Could you post the full patch?
Thanks!

Jiaying

> Thanks!
>
> -- Steve
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/