| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210525142835.GO4441@paulmck-ThinkPad-P17-Gen-1>
Date: Tue, 25 May 2021 07:28:35 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: "Xu, Yanfei" <yanfei.xu@...driver.com>
Cc: Dmitry Vyukov <dvyukov@...gle.com>,
syzbot <syzbot+7b2b13f4943374609532@...kaller.appspotmail.com>,
rcu@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
Andrii Nakryiko <andrii@...nel.org>,
Alexei Starovoitov <ast@...nel.org>,
Jens Axboe <axboe@...nel.dk>, bpf <bpf@...r.kernel.org>,
Christian Brauner <christian@...uner.io>,
Daniel Borkmann <daniel@...earbox.net>,
John Fastabend <john.fastabend@...il.com>,
Martin KaFai Lau <kafai@...com>,
KP Singh <kpsingh@...nel.org>,
LKML <linux-kernel@...r.kernel.org>,
netdev <netdev@...r.kernel.org>,
Shakeel Butt <shakeelb@...gle.com>,
Song Liu <songliubraving@...com>,
syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
Yonghong Song <yhs@...com>
Subject: Re: [syzbot] KASAN: use-after-free Read in
check_all_holdout_tasks_trace
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@...kaller.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@...kaller.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.
> 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!!!
Powered by blists - more mailing lists