Messages in this thread | | | Subject | Re: [syzbot] KASAN: use-after-free Read in check_all_holdout_tasks_trace | From | "Xu, Yanfei" <> | Date | Wed, 26 May 2021 10:22:59 +0800 |
| |
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!!! >
| |