[SPAM]Re: [PATCH] ring-buffer: Do not read at &event->array[0] if it across the page

Tze-nan Wu (吳澤南) Tze-nan.Wu at mediatek.com
Fri Sep 8 00:51:36 PDT 2023


On Thu, 2023-09-07 at 08:14 -0400, Steven Rostedt wrote:
> On Thu, 7 Sep 2023 10:10:57 +0000
> Tze-nan Wu (吳澤南) <Tze-nan.Wu at mediatek.com> wrote:
> 
> > > Same as my thought,
> > > since every time the reader try to access the address in next
> > > page,
> > > the below condition hold in rb_iter_head_event function:
> > > 
> > >         if (iter->page_stamp != iter_head_page->page->time_stamp
> > > ||
> > >                 commit > rb_page_commit(iter_head_page))
> > >                         goto reset;
> > > 
> > > I observe the result by the debug patch provided below:
> > > 
> > >         @@ -2378,6 +2378,19 @@ rb_iter_head_event(struct
> > > ring_buffer_iter *iter)
> > >                 commit = rb_page_commit(iter_head_page);
> > >                 smp_rmb();
> > >         +       if ((iter->head >= 0xFECUL) && commit == 0xFF0UL)
> > > {
> > >         +               pr_info("rbdbg: cpu = %d, event = 0x%lx,
> > > iter-  
> > > > head = 0x%lx,\  
> > > 
> > >         +                       commit = 0xFF0, type = 0x%x, ts =
> > > 0x%x,
> > > array addr = 0x%lx\n",\
> > >         +                       iter->cpu_buffer->cpu, (unsigned
> > > long)event, iter->head,\
> > >         +                       event->type_len, event-
> > > >time_delta,
> > > (unsigned long)(&(event->array[0])));
> > >         +               mdelay(500);
> > >         +               pr_info("rbdbg2: cpu = %d, event = 0x%lx,
> > > iter-  
> > > > head = 0x%lx,\  
> > > 
> > >         +                       commit = 0xFF0, type = 0x%x, ts =
> > > 0x%x,
> > > array addr = 0x%lx\n",\
> > >         +                       iter->cpu_buffer->cpu, (unsigned
> > > long)event, iter->head,\
> > >         +                       event->type_len, event-
> > > >time_delta,
> > > (unsigned long)(&(event->array[0])));
> > >         +               if (iter->page_stamp != iter_head_page-
> > > >page-  
> > > > time_stamp || commit > rb_page_commit(iter_head_page))  
> > > 
> > >         +                       pr_info("rbdbg: writer corrupt
> > > reader\n");
> > >         +       }
> > >                 event = __rb_page_index(iter_head_page, iter-
> > > >head);
> > >                 length = rb_event_length(event);
> > > 
> > > Note that the mdelay(500) in the debug patch can reproduce the
> > > issue
> > > easier with same test in my environmnet,
> > > I am now able to reproduce the issue within 15 minutes if the
> > > debug
> > > patch on.
> > >   
> > 
> > The debug patch may give something similar to the below log just
> > before
> > the invalid access happened, for me it looks like the padding event
> > had
> > just corrupted by the writer before the reader invoke
> > rb_event_length
> > function on it.
> > 
> > [  338.156772] cat: [name:ring_buffer&]rbdbg: cpu = 0, event =
> > 0x????????????dffc, iter->head = 0xfec, commit = 0xFF0, type =
> > 0x1d, ts
> > = 0x0, array addr = 0x????????????e000
> > [  338.656796] cat: [name:ring_buffer&]rbdbg2: cpu = 0, event =
> > 0x????????????dffc, iter->head = 0xfec, commit = 0xFF0, type = 0x0,
> > ts
> > = 0x0, array addr = 0x????????????e000
> > [  338.656803] cat: [name:ring_buffer&]rbdbg: writer corrupt reader
> > [  338.656810] cat:
> > [name:report&]=====================================================
> > ====
> > =========
> > [  338.656819] cat: [name:report&]BUG: KASAN: invalid-access in
> > rb_event_length
> > 
> > 
> > > > > Since the content data start at address 0x71FFFF8111A17FFC
> > > > > are  
> > > > 
> > > > 0xFFFFFFC0.  
> > > > > event->type will be interpret as 0x0, than the reader will
> > > > > try to  
> > > > 
> > > > get the  
> > > > > length by accessing event->array[0], which is an invalid
> > > > > address:
> > > > >     &event->array[0] = 0x71FFFF8111A18000
> > > > > 
> > > > > Signed-off-by: Tze-nan Wu <Tze-nan.Wu at mediatek.com>
> > > > > ---
> > > > > Following patch may not become a solution, it merely checks
> > > > > if
> > > > > the  
> > > > 
> > > > address  
> > > > > to be accessed is valid or not within the rb_event_length
> > > > > before  
> > > > 
> > > > access.  
> > > > > And not sure if there is any side-effect it can lead to.
> > > > > 
> > > > > I am curious about what a better solution for this issue
> > > > > would
> > > > > look  
> > > > 
> > > > like.  
> > > > > Should we address the problem from the writer or the reader?
> > > > > 
> > > > > Also I wonder if the cause of the issue is exactly as I
> > > > > suspected.
> > > > > Any Suggestion will be appreciated.  
> > > > 
> > > > I guess this depends on if you have the fixes or not?
> > > >   
> > > 
> > > yes, I could try to pick the patches that only included in
> > > mainline
> > > but
> > > not in kernel-6.1.52 for ring_buffer.c file,
> > > and do the same test to see if the issue is still reproducible.
> > >   
> > > > > 
> > > > > Test below can reproduce the issue in 2 hours on kernel-
> > > > > 6.1.24:
> > > > >     $cd /sys/kernel/tracing/
> > > > >     # make the reader and writer race more through resize
> > > > > the  
> > > > 
> > > > buffer to 8kb  
> > > > >     $echo 8 > buffer_size_kn
> > > > >     # enable all events
> > > > >     $echo 1 > event/enable
> > > > >     # enable trace
> > > > >     $echo 1 > tracing_on
> > > > >  
> > > > >     # write and run a script that keep reading trace
> > > > >     $./read_trace.sh
> > > > > 
> > > > >     ``` read_trace.sh
> > > > >        while :
> > > > >        do
> > > > >            cat /sys/kernel/tracing/trace > /dev/null
> > > > >        done
> > > > > 
> > > > >     ```  
> > > > 
> > > > Thanks, I'll look at that when I finish debugging the eventfs
> > > > bug.
> > > > 
> > > > -- Steve  
> > > 
> > > Also thank for your reply,
> > >   
> > 
> > And the temporary fix patch in my first mail should have some
> > modified
> > as shown below.
> > +               if (((unsigned long)event & 0xfffUL) >= PAGE_SIZE -
> > 4)
> >                                              ^^^^^^
> >                                             PAGE_SIZE-1
> > +                       return -1;
> 
> 
> Your email is getting really hard to read due to the line wrap
> formatting.
> 
> Anyway, can you try this patch?
> 
> -- Steve
> 

Sorry for the inconvenience, will be carefull next time.

The patch has passed my test in my environment, no more KASAN report
during the same test.
Thanks, and I've learned a lot.

-- Tze-nan

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 72ccf75defd0..5b0653378089 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -2390,6 +2390,10 @@ rb_iter_head_event(struct ring_buffer_iter
> *iter)
>  	 */
>  	commit = rb_page_commit(iter_head_page);
>  	smp_rmb();
> +	/* An event needs to be at least 8 bytes in size */
> +	if (iter->head > commit - 8)
> +		goto reset;
> +
>  	event = __rb_page_index(iter_head_page, iter->head);
>  	length = rb_event_length(event);
>  


More information about the Linux-mediatek mailing list