Re: [syzbot] KASAN: use-after-free Read in check_all_holdout_tasks_trace

["Xu, Yanfei" <yanfei.xu@windriver.com>]

On 5/25/21 10:28 PM, Paul E. McKenney wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Tue, May 25, 2021 at 06:24:10PM +0800, Xu, Yanfei wrote:
>>
>>
>> On 5/25/21 11:33 AM, Paul E. McKenney wrote:
>>> [Please note: This e-mail is from an EXTERNAL e-mail address]
>>>
>>> On Tue, May 25, 2021 at 10:31:55AM +0800, Xu, Yanfei wrote:
>>>>
>>>>
>>>> On 5/25/21 6:46 AM, Paul E. McKenney wrote:
>>>>> [Please note: This e-mail is from an EXTERNAL e-mail address]
>>>>>
>>>>> On Sun, May 23, 2021 at 09:13:50PM -0700, Paul E. McKenney wrote:
>>>>>> On Sun, May 23, 2021 at 08:51:56AM +0200, Dmitry Vyukov wrote:
>>>>>>> On Fri, May 21, 2021 at 7:29 PM syzbot
>>>>>>> <syzbot+7b2b13f4943374609532@syzkaller.appspotmail.com> wrote:
>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> syzbot found the following issue on:
>>>>>>>>
>>>>>>>> HEAD commit:    f18ba26d libbpf: Add selftests for TC-BPF management API
>>>>>>>> git tree:       bpf-next
>>>>>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=17f50d1ed00000
>>>>>>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=8ff54addde0afb5d
>>>>>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=7b2b13f4943374609532
>>>>>>>>
>>>>>>>> Unfortunately, I don't have any reproducer for this issue yet.
>>>>>>>>
>>>>>>>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>>>>>>>> Reported-by: syzbot+7b2b13f4943374609532@syzkaller.appspotmail.com
>>>>>>>
>>>>>>> This looks rcu-related. +rcu mailing list
>>>>>>
>>>>>> I think I see a possible cause for this, and will say more after some
>>>>>> testing and after becoming more awake Monday morning, Pacific time.
>>>>>
>>>>> No joy.  From what I can see, within RCU Tasks Trace, the calls to
>>>>> get_task_struct() are properly protected (either by RCU or by an earlier
>>>>> get_task_struct()), and the calls to put_task_struct() are balanced by
>>>>> those to get_task_struct().
>>>>>
>>>>> I could of course have missed something, but at this point I am suspecting
>>>>> an unbalanced put_task_struct() has been added elsewhere.
>>>>>
>>>>> As always, extra eyes on this code would be a good thing.
>>>>>
>>>>> If it were reproducible, I would of course suggest bisection.  :-/
>>>>>
>>>>>                                                            Thanx, Paul
>>>>>
>>>> Hi Paul,
>>>>
>>>> Could it be?
>>>>
>>>>          CPU1                                        CPU2
>>>> trc_add_holdout(t, bhp)
>>>> //t->usage==2
>>>>                                         release_task
>>>>                                           put_task_struct_rcu_user
>>>>                                             delayed_put_task_struct
>>>>                                               ......
>>>>                                               put_task_struct(t)
>>>>                                               //t->usage==1
>>>>
>>>> check_all_holdout_tasks_trace
>>>>     ->trc_wait_for_one_reader
>>>>       ->trc_del_holdout
>>>>         ->put_task_struct(t)
>>>>         //t->usage==0 and task_struct freed
>>>>     READ_ONCE(t->trc_reader_checked)
>>>>     //ops， t had been freed.
>>>>
>>>> So, after excuting trc_wait_for_one_reader（）, task might had been removed
>>>> from holdout list and the corresponding task_struct was freed.
>>>> And we shouldn't do READ_ONCE(t->trc_reader_checked).
>>>
>>> I was suspicious of that call to trc_del_holdout() from within
>>> trc_wait_for_one_reader(), but the only time it executes is in the
>>> context of the current running task, which means that CPU 2 had better
>>> not be invoking release_task() on it just yet.
>>>
>>> Or am I missing your point?
>>
>> Two times.
>> 1. the task is current.
>>
>>                 trc_wait_for_one_reader
>>                   ->trc_del_holdout
> 
> This one should be fine because the task cannot be freed until it
> actually exits, and the grace-period kthread never exits.  But it
> could also be removed without any problem that I see. >

Agree, current task's task_struct should be high probably safe.  If you 
think it is safe to remove, I prefer to remove it. Because it can make 
trc_wait_for_one_reader's behavior about deleting task from holdout more 
unified. And there should be a very small racy that the task is checked 
as a current and then turn into a exiting task before its task_struct is 
accessed in trc_wait_for_one_reader or check_all_holdout_tasks_trace.（or 
I misunderstand something about rcu tasks）

>> 2. task isn't current.
>>
>>                 trc_wait_for_one_reader
>>                   ->get_task_struct
>>                   ->try_invoke_on_locked_down_task（trc_inspect_reader）
>>                     ->trc_del_holdout
>>                   ->put_task_struct
> 
> Ah, this one is more interesting, thank you!
> 
> Yes, it is safe from the list's viewpoint to do the removal in the
> trc_inspect_reader() callback, but you are right that the grace-period
> kthread may touch the task structure after return, and there might not
> be anything else holding that task structure in place.
> 
>>> Of course, if you can reproduce it, the following patch might be
>>
>> Sorry...I can't reproduce it, just analyse syzbot's log. :(
> 
> Well, if it could be reproduced, that would mean that it was too easy,
> wouldn't it?  ;-)

Ha ;-)
> 
> How about the (untested) patch below, just to make sure that we are
> talking about the same thing?  I have started testing, but then
> again, I have not yet been able to reproduce this, either.
> 
>                                                          Thanx, Paul
> 

Yes! we are talking the same thing, Should I send a new patch?

Thanks,
Yanfei

> ------------------------------------------------------------------------
> 
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index efb8127f3a36..8b25551d10db 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -957,10 +957,9 @@ static bool trc_inspect_reader(struct task_struct *t, void *arg)
>                  in_qs = likely(!t->trc_reader_nesting);
>          }
> 
> -       // Mark as checked.  Because this is called from the grace-period
> -       // kthread, also remove the task from the holdout list.
> +       // Mark as checked so that the grace-period kthread will
> +       // remove it from the holdout list.
>          t->trc_reader_checked = true;
> -       trc_del_holdout(t);
> 
>          if (in_qs)
>                  return true;  // Already in quiescent state, done!!!
>