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

From: "Xu, Yanfei" <yanfei.xu@windriver.com>
To: paulmck@kernel.org
Cc: Dmitry Vyukov <dvyukov@google.com>,
	syzbot <syzbot+7b2b13f4943374609532@syzkaller.appspotmail.com>,
	rcu@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>, Jens Axboe <axboe@kernel.dk>,
	bpf <bpf@vger.kernel.org>,
	Christian Brauner <christian@brauner.io>,
	Daniel Borkmann <daniel@iogearbox.net>,
	John Fastabend <john.fastabend@gmail.com>,
	Martin KaFai Lau <kafai@fb.com>, KP Singh <kpsingh@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>,
	Shakeel Butt <shakeelb@google.com>,
	Song Liu <songliubraving@fb.com>,
	syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
	Yonghong Song <yhs@fb.com>
Subject: Re: [syzbot] KASAN: use-after-free Read in check_all_holdout_tasks_trace
Date: Wed, 26 May 2021 14:03:38 +0800	[thread overview]
Message-ID: <fb2ee999-1796-29af-c0ef-60923dc82e12@windriver.com> (raw)
In-Reply-To: <20210526042104.GZ4441@paulmck-ThinkPad-P17-Gen-1>

On 5/26/21 12:21 PM, Paul E. McKenney wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Wed, May 26, 2021 at 10:22:59AM +0800, Xu, Yanfei wrote:
>> 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 ;-)
> 
> But it should be possible to make this happen...  Is it possible to
> add lots of short-lived tasks to the test that failed?
> 

Agree.

>>> 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?
> 
> Or look at these commits that I queued this past morning (Pacific Time)
> on the "dev" branch of the -rcu tree:
> 
> aac385ea2494 rcu-tasks: Don't delete holdouts within trc_inspect_reader()
> bf30dc63947c rcu-tasks: Don't delete holdouts within trc_wait_for_one_reader()

Got it, Thanks!

Regards,
Yanfei

> 
> They pass initial testing, but then again, such tests passed before
> these patches were queued.  :-/
> 
>                                                          Thanx, Paul
> 