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@xxxxxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx > > > > > > > > > > > > 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. > 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? ;-) 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 ------------------------------------------------------------------------ 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!!!
