* [syzbot] WARNING in kthread_is_per_cpu @ 2021-04-19 10:36 syzbot 2021-04-19 11:30 ` Thomas Gleixner 2021-04-19 11:31 ` Valentin Schneider 0 siblings, 2 replies; 10+ messages in thread From: syzbot @ 2021-04-19 10:36 UTC (permalink / raw) To: bp, dwmw, hpa, linux-kernel, luto, mingo, syzkaller-bugs, tglx, x86 Hello, syzbot found the following issue on: HEAD commit: 1216f02e Add linux-next specific files for 20210415 git tree: linux-next console output: https://syzkaller.appspot.com/x/log.txt?x=1032ba29d00000 kernel config: https://syzkaller.appspot.com/x/.config?x=3491b04113499f81 dashboard link: https://syzkaller.appspot.com/bug?extid=9362b31a2e0cad8b749d 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+9362b31a2e0cad8b749d@syzkaller.appspotmail.com ------------[ cut here ]------------ WARNING: CPU: 1 PID: 23550 at kernel/kthread.c:83 to_kthread kernel/kthread.c:83 [inline] WARNING: CPU: 1 PID: 23550 at kernel/kthread.c:83 kthread_is_per_cpu+0xc4/0xf0 kernel/kthread.c:519 Modules linked in: CPU: 1 PID: 23550 Comm: syz-executor.3 Not tainted 5.12.0-rc7-next-20210415-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:to_kthread kernel/kthread.c:83 [inline] RIP: 0010:kthread_is_per_cpu+0xc4/0xf0 kernel/kthread.c:519 Code: 00 00 00 00 fc ff df 48 c1 ea 03 80 3c 02 00 75 2e 4c 8b 23 41 83 e4 01 e8 89 d3 27 00 44 89 e0 5b 5d 41 5c c3 e8 7c d3 27 00 <0f> 0b eb 88 e8 33 90 6c 00 e9 68 ff ff ff e8 39 90 6c 00 eb 9a 48 RSP: 0018:ffffc90000dc0c08 EFLAGS: 00010046 RAX: 0000000000000000 RBX: ffff88802533d580 RCX: 0000000000000100 RDX: ffff8880549bb900 RSI: ffffffff814ca4c4 RDI: 0000000000000003 RBP: 0000000000000000 R08: 0000000000000000 R09: ffff88802533d580 R10: ffffffff814ca44c R11: 00000000018a3b90 R12: 0000000000000001 R13: ffffc90000dc0d90 R14: 0000000000000001 R15: ffff88802533d580 FS: 00007f4be57d3700(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000001b2cd24000 CR3: 0000000024626000 CR4: 00000000001506e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <IRQ> can_migrate_task+0x124/0x1630 kernel/sched/fair.c:7610 detach_tasks kernel/sched/fair.c:7774 [inline] load_balance+0xc72/0x2730 kernel/sched/fair.c:9696 rebalance_domains+0x668/0xda0 kernel/sched/fair.c:10075 __do_softirq+0x29b/0x9fe kernel/softirq.c:559 invoke_softirq kernel/softirq.c:433 [inline] __irq_exit_rcu+0x136/0x200 kernel/softirq.c:637 irq_exit_rcu+0x5/0x20 kernel/softirq.c:649 sysvec_apic_timer_interrupt+0x93/0xc0 arch/x86/kernel/apic/apic.c:1100 </IRQ> asm_sysvec_apic_timer_interrupt+0x12/0x20 arch/x86/include/asm/idtentry.h:632 RIP: 0010:__raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:161 [inline] RIP: 0010:_raw_spin_unlock_irqrestore+0x38/0x70 kernel/locking/spinlock.c:191 Code: 74 24 10 e8 5a 05 46 f8 48 89 ef e8 f2 7d 46 f8 81 e3 00 02 00 00 75 25 9c 58 f6 c4 02 75 2d 48 85 db 74 01 fb bf 01 00 00 00 <e8> e3 83 3a f8 65 8b 05 cc e4 ed 76 85 c0 74 0a 5b 5d c3 e8 20 73 RSP: 0018:ffffc90001c0ef30 EFLAGS: 00000206 RAX: 0000000000000006 RBX: 0000000000000200 RCX: 1ffffffff1fbdad2 RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000001 RBP: ffffffff8c076c20 R08: 0000000000000001 R09: ffffffff8fdeb8a7 R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000 R13: 000000000000000c R14: 0000000000000002 R15: 000000000008f9b0 spin_unlock_irqrestore include/linux/spinlock.h:414 [inline] pcpu_alloc+0x4f7/0x17a0 mm/percpu.c:1807 vlan_dev_init+0x9f9/0xe70 net/8021q/vlan_dev.c:614 register_netdevice+0x51e/0x1500 net/core/dev.c:10188 register_vlan_dev+0x360/0x960 net/8021q/vlan.c:179 vlan_newlink+0x477/0x700 net/8021q/vlan_netlink.c:187 __rtnl_newlink+0x1062/0x1710 net/core/rtnetlink.c:3452 rtnl_newlink+0x64/0xa0 net/core/rtnetlink.c:3500 rtnetlink_rcv_msg+0x413/0xaf0 net/core/rtnetlink.c:5562 netlink_rcv_skb+0x153/0x420 net/netlink/af_netlink.c:2502 netlink_unicast_kernel net/netlink/af_netlink.c:1312 [inline] netlink_unicast+0x533/0x7d0 net/netlink/af_netlink.c:1338 netlink_sendmsg+0x84c/0xd90 net/netlink/af_netlink.c:1927 sock_sendmsg_nosec net/socket.c:654 [inline] sock_sendmsg+0xcf/0x120 net/socket.c:674 ____sys_sendmsg+0x6e8/0x810 net/socket.c:2350 ___sys_sendmsg+0xf3/0x170 net/socket.c:2404 __sys_sendmsg+0xe5/0x1b0 net/socket.c:2433 do_syscall_64+0x3a/0xb0 arch/x86/entry/common.c:47 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x466459 Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48 RSP: 002b:00007f4be57d3188 EFLAGS: 00000246 ORIG_RAX: 000000000000002e RAX: ffffffffffffffda RBX: 000000000056bf60 RCX: 0000000000466459 RDX: 0000000000000810 RSI: 0000000020000000 RDI: 0000000000000006 RBP: 00000000004bf9fb R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 000000000056bf60 R13: 00007ffc92da70af R14: 00007f4be57d3300 R15: 0000000000022000 --- This report is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this issue. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] WARNING in kthread_is_per_cpu 2021-04-19 10:36 [syzbot] WARNING in kthread_is_per_cpu syzbot @ 2021-04-19 11:30 ` Thomas Gleixner 2021-04-19 11:31 ` Valentin Schneider 1 sibling, 0 replies; 10+ messages in thread From: Thomas Gleixner @ 2021-04-19 11:30 UTC (permalink / raw) To: syzbot Cc: peterz, bp, dwmw, hpa, linux-kernel, luto, mingo, syzkaller-bugs, x86 On Mon, Apr 19 2021 at 03:36, syzbot wrote: > Hello, > > syzbot found the following issue on: > > HEAD commit: 1216f02e Add linux-next specific files for 20210415 > git tree: linux-next > console output: https://syzkaller.appspot.com/x/log.txt?x=1032ba29d00000 > kernel config: https://syzkaller.appspot.com/x/.config?x=3491b04113499f81 > dashboard link: https://syzkaller.appspot.com/bug?extid=9362b31a2e0cad8b749d > > 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+9362b31a2e0cad8b749d@syzkaller.appspotmail.com > > ------------[ cut here ]------------ > WARNING: CPU: 1 PID: 23550 at kernel/kthread.c:83 to_kthread kernel/kthread.c:83 [inline] > WARNING: CPU: 1 PID: 23550 at kernel/kthread.c:83 kthread_is_per_cpu+0xc4/0xf0 kernel/kthread.c:519 > Modules linked in: > CPU: 1 PID: 23550 Comm: syz-executor.3 Not tainted 5.12.0-rc7-next-20210415-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > RIP: 0010:to_kthread kernel/kthread.c:83 [inline] > RIP: 0010:kthread_is_per_cpu+0xc4/0xf0 kernel/kthread.c:519 > Code: 00 00 00 00 fc ff df 48 c1 ea 03 80 3c 02 00 75 2e 4c 8b 23 41 83 e4 01 e8 89 d3 27 00 44 89 e0 5b 5d 41 5c c3 e8 7c d3 27 00 <0f> 0b eb 88 e8 33 90 6c 00 e9 68 ff ff ff e8 39 90 6c 00 eb 9a 48 > RSP: 0018:ffffc90000dc0c08 EFLAGS: 00010046 > RAX: 0000000000000000 RBX: ffff88802533d580 RCX: 0000000000000100 > RDX: ffff8880549bb900 RSI: ffffffff814ca4c4 RDI: 0000000000000003 > RBP: 0000000000000000 R08: 0000000000000000 R09: ffff88802533d580 > R10: ffffffff814ca44c R11: 00000000018a3b90 R12: 0000000000000001 > R13: ffffc90000dc0d90 R14: 0000000000000001 R15: ffff88802533d580 > FS: 00007f4be57d3700(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000001b2cd24000 CR3: 0000000024626000 CR4: 00000000001506e0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > <IRQ> > can_migrate_task+0x124/0x1630 kernel/sched/fair.c:7610 So this is: if ((p->flags & PF_KTHREAD) && kthread_is_per_cpu(p)) The warning in to_kthread() is: WARN_ON(!(k->flags & PF_KTHREAD)); IOW, the p>flags lost PF_KTHREAD within at max. 50 instructions. Magic, cosmic rays or memory corruption / stray pointer in some other place? Thanks, tglx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] WARNING in kthread_is_per_cpu 2021-04-19 10:36 [syzbot] WARNING in kthread_is_per_cpu syzbot 2021-04-19 11:30 ` Thomas Gleixner @ 2021-04-19 11:31 ` Valentin Schneider 2021-04-19 18:45 ` Peter Zijlstra 1 sibling, 1 reply; 10+ messages in thread From: Valentin Schneider @ 2021-04-19 11:31 UTC (permalink / raw) To: syzbot, bp, dwmw, hpa, linux-kernel, luto, mingo, syzkaller-bugs, tglx, x86 On 19/04/21 03:36, syzbot wrote: > Hello, > > syzbot found the following issue on: > > HEAD commit: 1216f02e Add linux-next specific files for 20210415 > git tree: linux-next > console output: https://syzkaller.appspot.com/x/log.txt?x=1032ba29d00000 > kernel config: https://syzkaller.appspot.com/x/.config?x=3491b04113499f81 > dashboard link: https://syzkaller.appspot.com/bug?extid=9362b31a2e0cad8b749d > > 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+9362b31a2e0cad8b749d@syzkaller.appspotmail.com > > ------------[ cut here ]------------ > WARNING: CPU: 1 PID: 23550 at kernel/kthread.c:83 to_kthread kernel/kthread.c:83 [inline] > WARNING: CPU: 1 PID: 23550 at kernel/kthread.c:83 kthread_is_per_cpu+0xc4/0xf0 kernel/kthread.c:519 > Modules linked in: > CPU: 1 PID: 23550 Comm: syz-executor.3 Not tainted 5.12.0-rc7-next-20210415-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > RIP: 0010:to_kthread kernel/kthread.c:83 [inline] > RIP: 0010:kthread_is_per_cpu+0xc4/0xf0 kernel/kthread.c:519 > Code: 00 00 00 00 fc ff df 48 c1 ea 03 80 3c 02 00 75 2e 4c 8b 23 41 83 e4 01 e8 89 d3 27 00 44 89 e0 5b 5d 41 5c c3 e8 7c d3 27 00 <0f> 0b eb 88 e8 33 90 6c 00 e9 68 ff ff ff e8 39 90 6c 00 eb 9a 48 > RSP: 0018:ffffc90000dc0c08 EFLAGS: 00010046 > RAX: 0000000000000000 RBX: ffff88802533d580 RCX: 0000000000000100 > RDX: ffff8880549bb900 RSI: ffffffff814ca4c4 RDI: 0000000000000003 > RBP: 0000000000000000 R08: 0000000000000000 R09: ffff88802533d580 > R10: ffffffff814ca44c R11: 00000000018a3b90 R12: 0000000000000001 > R13: ffffc90000dc0d90 R14: 0000000000000001 R15: ffff88802533d580 > FS: 00007f4be57d3700(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000001b2cd24000 CR3: 0000000024626000 CR4: 00000000001506e0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > <IRQ> > can_migrate_task+0x124/0x1630 kernel/sched/fair.c:7610 > detach_tasks kernel/sched/fair.c:7774 [inline] > load_balance+0xc72/0x2730 kernel/sched/fair.c:9696 > rebalance_domains+0x668/0xda0 kernel/sched/fair.c:10075 > __do_softirq+0x29b/0x9fe kernel/softirq.c:559 > invoke_softirq kernel/softirq.c:433 [inline] > __irq_exit_rcu+0x136/0x200 kernel/softirq.c:637 > irq_exit_rcu+0x5/0x20 kernel/softirq.c:649 > sysvec_apic_timer_interrupt+0x93/0xc0 arch/x86/kernel/apic/apic.c:1100 > </IRQ> > asm_sysvec_apic_timer_interrupt+0x12/0x20 > arch/x86/include/asm/idtentry.h:632 if ((p->flags & PF_KTHREAD) && kthread_is_per_cpu(p)) `\ to_kthread(p); `\ WARN_ON(!(p->flags & PF_KTHREAD)); ... Huh? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] WARNING in kthread_is_per_cpu 2021-04-19 11:31 ` Valentin Schneider @ 2021-04-19 18:45 ` Peter Zijlstra 2021-04-19 19:58 ` Valentin Schneider 0 siblings, 1 reply; 10+ messages in thread From: Peter Zijlstra @ 2021-04-19 18:45 UTC (permalink / raw) To: Valentin Schneider Cc: syzbot, bp, dwmw, hpa, linux-kernel, luto, mingo, syzkaller-bugs, tglx, x86 On Mon, Apr 19, 2021 at 12:31:22PM +0100, Valentin Schneider wrote: > if ((p->flags & PF_KTHREAD) && kthread_is_per_cpu(p)) > `\ > to_kthread(p); > `\ > WARN_ON(!(p->flags & PF_KTHREAD)); > > ... Huh? Something like so perhaps? diff --git a/kernel/kthread.c b/kernel/kthread.c index 1578973c5740..eeba40df61ac 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -78,6 +78,14 @@ static inline void set_kthread_struct(void *kthread) current->set_child_tid = (__force void __user *)kthread; } +static inline struct kthread *__to_kthread(struct task_struct *k) +{ + void *kthread = (__force void *)k->set_child_tid; + if (kthread && !(k->flags & PF_KTHREAD)) + kthread = NULL; + return kthread; +} + static inline struct kthread *to_kthread(struct task_struct *k) { WARN_ON(!(k->flags & PF_KTHREAD)); @@ -516,7 +524,7 @@ void kthread_set_per_cpu(struct task_struct *k, int cpu) bool kthread_is_per_cpu(struct task_struct *k) { - struct kthread *kthread = to_kthread(k); + struct kthread *kthread = __to_kthread(k); if (!kthread) return false; diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 3384ea74cad4..dc6311bd6986 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -7658,7 +7658,7 @@ static void balance_push(struct rq *rq) * histerical raisins. */ if (rq->idle == push_task || - ((push_task->flags & PF_KTHREAD) && kthread_is_per_cpu(push_task)) || + kthread_is_per_cpu(push_task) || is_migration_disabled(push_task)) { /* ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [syzbot] WARNING in kthread_is_per_cpu 2021-04-19 18:45 ` Peter Zijlstra @ 2021-04-19 19:58 ` Valentin Schneider 2021-04-20 8:51 ` Peter Zijlstra 0 siblings, 1 reply; 10+ messages in thread From: Valentin Schneider @ 2021-04-19 19:58 UTC (permalink / raw) To: Peter Zijlstra Cc: syzbot, bp, dwmw, hpa, linux-kernel, luto, mingo, syzkaller-bugs, tglx, x86 On 19/04/21 20:45, Peter Zijlstra wrote: > On Mon, Apr 19, 2021 at 12:31:22PM +0100, Valentin Schneider wrote: > >> if ((p->flags & PF_KTHREAD) && kthread_is_per_cpu(p)) >> `\ >> to_kthread(p); >> `\ >> WARN_ON(!(p->flags & PF_KTHREAD)); >> >> ... Huh? > > Something like so perhaps? > Looks about right, IIUC the key being: p->flags & PF_KTHREAD + p->set_child_tid => the struct kthread is persistent p->flags & PF_KTHREAD => you may or may not have a struct kthread (see kernel/umh.c kernel_thread() uses). PF_KTHREAD isn't even guaranteed to persist (begin_new_exec()), which seems to be what the syzbot hit. I'd be happy to see is_per_cpu_kthread() die, but that's somewhat orthogonal to this here. For now, this does need the tiny extra below. While we're at it, does free_kthread_struct() want the __to_kthread() treatment as well? The other to_kthread() callsites looked like they only made sense with a "proper" kthread anyway. --- diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 49636a49843f..8b470c2d5680 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -7612,7 +7612,7 @@ int can_migrate_task(struct task_struct *p, struct lb_env *env) return 0; /* Disregard pcpu kthreads; they are where they need to be. */ - if ((p->flags & PF_KTHREAD) && kthread_is_per_cpu(p)) + if (kthread_is_per_cpu(p)) return 0; if (!cpumask_test_cpu(env->dst_cpu, p->cpus_ptr)) { > diff --git a/kernel/kthread.c b/kernel/kthread.c > index 1578973c5740..eeba40df61ac 100644 > --- a/kernel/kthread.c > +++ b/kernel/kthread.c > @@ -78,6 +78,14 @@ static inline void set_kthread_struct(void *kthread) > current->set_child_tid = (__force void __user *)kthread; > } > > +static inline struct kthread *__to_kthread(struct task_struct *k) > +{ > + void *kthread = (__force void *)k->set_child_tid; > + if (kthread && !(k->flags & PF_KTHREAD)) > + kthread = NULL; > + return kthread; > +} > + > static inline struct kthread *to_kthread(struct task_struct *k) > { > WARN_ON(!(k->flags & PF_KTHREAD)); > @@ -516,7 +524,7 @@ void kthread_set_per_cpu(struct task_struct *k, int cpu) > > bool kthread_is_per_cpu(struct task_struct *k) > { > - struct kthread *kthread = to_kthread(k); > + struct kthread *kthread = __to_kthread(k); > if (!kthread) > return false; > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 3384ea74cad4..dc6311bd6986 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -7658,7 +7658,7 @@ static void balance_push(struct rq *rq) > * histerical raisins. > */ > if (rq->idle == push_task || > - ((push_task->flags & PF_KTHREAD) && kthread_is_per_cpu(push_task)) || > + kthread_is_per_cpu(push_task) || > is_migration_disabled(push_task)) { > > /* ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [syzbot] WARNING in kthread_is_per_cpu 2021-04-19 19:58 ` Valentin Schneider @ 2021-04-20 8:51 ` Peter Zijlstra 2021-04-20 9:43 ` Valentin Schneider 2021-04-22 7:36 ` [tip: sched/core] kthread: Fix PF_KTHREAD vs to_kthread() race tip-bot2 for Peter Zijlstra 0 siblings, 2 replies; 10+ messages in thread From: Peter Zijlstra @ 2021-04-20 8:51 UTC (permalink / raw) To: Valentin Schneider Cc: syzbot, bp, dwmw, hpa, linux-kernel, luto, mingo, syzkaller-bugs, tglx, x86 On Mon, Apr 19, 2021 at 08:58:26PM +0100, Valentin Schneider wrote: > Looks about right, IIUC the key being: > > p->flags & PF_KTHREAD + p->set_child_tid => the struct kthread is > persistent > > p->flags & PF_KTHREAD => you may or may not have a struct kthread (see > kernel/umh.c kernel_thread() uses). PF_KTHREAD isn't even guaranteed to > persist (begin_new_exec()), which seems to be what the syzbot hit. Ack, that's nicely put. > While we're at it, does free_kthread_struct() want the __to_kthread() > treatment as well? The other to_kthread() callsites looked like they only > made sense with a "proper" kthread anyway. I think free_kthread_struct() is ok, because a task at that point in its lifetime cannot be also doing exec(). kthread_func() is another 'fun' trainwreck waiting to happen -- luckily the only caller uses current, still let me go fix it. kthread_probe_data() relies on PF_WQ_WORKER implying PF_KTHREAD but otherwise seems very fragile too. Something like so then? --- Subject: kthread: Fix PF_KTHREAD vs to_kthread() race From: Peter Zijlstra <peterz@infradead.org> Date: Tue Apr 20 10:18:17 CEST 2021 The kthread_is_per_cpu() construct relies on only being called on PF_KTHREAD tasks (per the WARN in to_kthread). This gives rise to the following usage pattern: if ((p->flags & PF_KTHREAD) && kthread_is_per_cpu(p)) However, as reported by syzcaller, this is broken. The scenario is: CPU0 CPU1 (running p) (p->flags & PF_KTHREAD) // true begin_new_exec() me->flags &= ~(PF_KTHREAD|...); kthread_is_per_cpu(p) to_kthread(p) WARN(!(p->flags & PF_KTHREAD) <-- *SPLAT* Introduce __to_kthread() that omits the WARN and is sure to check both values. Use this to remove the problematic pattern for kthread_is_per_cpu() and fix a number of other kthread_*() functions that have similar issues but are currently not used in ways that would expose the problem. Notably kthread_func() is only ever called on 'current', while kthread_probe_data() is only used for PF_WQ_WORKER, which implies the task is from kthread_create*(). Fixes: ac687e6e8c26 ("kthread: Extract KTHREAD_IS_PER_CPU") Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- kernel/kthread.c | 33 +++++++++++++++++++++++++++------ kernel/sched/core.c | 2 +- kernel/sched/fair.c | 2 +- 3 files changed, 29 insertions(+), 8 deletions(-) --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -84,6 +84,25 @@ static inline struct kthread *to_kthread return (__force void *)k->set_child_tid; } +/* + * Variant of to_kthread() that doesn't assume @p is a kthread. + * + * Per construction; when: + * + * (p->flags & PF_KTHREAD) && p->set_child_tid + * + * the task is both a kthread and struct kthread is persistent. However + * PF_KTHREAD on it's own is not, kernel_thread() can exec() (See umh.c and + * begin_new_exec()). + */ +static inline struct kthread *__to_kthread(struct task_struct *p) +{ + void *kthread = (__force void *)p->set_child_tid; + if (kthread && !(p->flags & PF_KTHREAD)) + kthread = NULL; + return kthread; +} + void free_kthread_struct(struct task_struct *k) { struct kthread *kthread; @@ -168,8 +187,9 @@ EXPORT_SYMBOL_GPL(kthread_freezable_shou */ void *kthread_func(struct task_struct *task) { - if (task->flags & PF_KTHREAD) - return to_kthread(task)->threadfn; + struct kthread *kthread = __to_kthread(task); + if (kthread) + return kthread->threadfn; return NULL; } EXPORT_SYMBOL_GPL(kthread_func); @@ -199,10 +219,11 @@ EXPORT_SYMBOL_GPL(kthread_data); */ void *kthread_probe_data(struct task_struct *task) { - struct kthread *kthread = to_kthread(task); + struct kthread *kthread = __to_kthread(task); void *data = NULL; - copy_from_kernel_nofault(&data, &kthread->data, sizeof(data)); + if (kthread) + copy_from_kernel_nofault(&data, &kthread->data, sizeof(data)); return data; } @@ -514,9 +535,9 @@ void kthread_set_per_cpu(struct task_str set_bit(KTHREAD_IS_PER_CPU, &kthread->flags); } -bool kthread_is_per_cpu(struct task_struct *k) +bool kthread_is_per_cpu(struct task_struct *p) { - struct kthread *kthread = to_kthread(k); + struct kthread *kthread = __to_kthread(p); if (!kthread) return false; --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -8505,7 +8505,7 @@ static void balance_push(struct rq *rq) * histerical raisins. */ if (rq->idle == push_task || - ((push_task->flags & PF_KTHREAD) && kthread_is_per_cpu(push_task)) || + kthread_is_per_cpu(push_task) || is_migration_disabled(push_task)) { /* --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -7619,7 +7619,7 @@ int can_migrate_task(struct task_struct return 0; /* Disregard pcpu kthreads; they are where they need to be. */ - if ((p->flags & PF_KTHREAD) && kthread_is_per_cpu(p)) + if (kthread_is_per_cpu(p)) return 0; if (!cpumask_test_cpu(env->dst_cpu, p->cpus_ptr)) { ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] WARNING in kthread_is_per_cpu 2021-04-20 8:51 ` Peter Zijlstra @ 2021-04-20 9:43 ` Valentin Schneider 2021-04-20 10:11 ` Peter Zijlstra 2021-04-22 7:36 ` [tip: sched/core] kthread: Fix PF_KTHREAD vs to_kthread() race tip-bot2 for Peter Zijlstra 1 sibling, 1 reply; 10+ messages in thread From: Valentin Schneider @ 2021-04-20 9:43 UTC (permalink / raw) To: Peter Zijlstra Cc: syzbot, bp, dwmw, hpa, linux-kernel, luto, mingo, syzkaller-bugs, tglx, x86 On 20/04/21 10:51, Peter Zijlstra wrote: > On Mon, Apr 19, 2021 at 08:58:26PM +0100, Valentin Schneider wrote: > >> Looks about right, IIUC the key being: >> >> p->flags & PF_KTHREAD + p->set_child_tid => the struct kthread is >> persistent >> >> p->flags & PF_KTHREAD => you may or may not have a struct kthread (see >> kernel/umh.c kernel_thread() uses). PF_KTHREAD isn't even guaranteed to >> persist (begin_new_exec()), which seems to be what the syzbot hit. > > Ack, that's nicely put. > >> While we're at it, does free_kthread_struct() want the __to_kthread() >> treatment as well? The other to_kthread() callsites looked like they only >> made sense with a "proper" kthread anyway. > > I think free_kthread_struct() is ok, because a task at that point in its > lifetime cannot be also doing exec(). > What if it's one of those kthreads created by directly invoking kernel_thread()? AFAICT right now it's only umh, and that one does execve() so it ends up stripped of PF_KTHREAD. It could however go through an error path, i.e. not call exec, and exit, giving us: put_task_struct(p) `\ free_task(p) `\ if (tsk->flags & PF_KTHREAD) free_kthread_struct(tsk); `\ to_kthread(p) > kthread_func() is another 'fun' trainwreck waiting to happen -- luckily > the only caller uses current, still let me go fix it. > > kthread_probe_data() relies on PF_WQ_WORKER implying PF_KTHREAD but > otherwise seems very fragile too. > > Something like so then? > Other than the above: Reviewed-by: Valentin Schneider <Valentin.Schneider@arm.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] WARNING in kthread_is_per_cpu 2021-04-20 9:43 ` Valentin Schneider @ 2021-04-20 10:11 ` Peter Zijlstra 2021-04-20 10:17 ` Valentin Schneider 0 siblings, 1 reply; 10+ messages in thread From: Peter Zijlstra @ 2021-04-20 10:11 UTC (permalink / raw) To: Valentin Schneider Cc: syzbot, bp, dwmw, hpa, linux-kernel, luto, mingo, syzkaller-bugs, tglx, x86 On Tue, Apr 20, 2021 at 10:43:43AM +0100, Valentin Schneider wrote: > On 20/04/21 10:51, Peter Zijlstra wrote: > > I think free_kthread_struct() is ok, because a task at that point in its > > lifetime cannot be also doing exec(). > > > > What if it's one of those kthreads created by directly invoking > kernel_thread()? AFAICT right now it's only umh, and that one does execve() > so it ends up stripped of PF_KTHREAD. It could however go through an error > path, i.e. not call exec, and exit, giving us: > > put_task_struct(p) > `\ > free_task(p) > `\ > if (tsk->flags & PF_KTHREAD) > free_kthread_struct(tsk); > `\ > to_kthread(p) I'm not following, at the point we hit free_task() it had better be dead and p->flags had better be stable. Either it will, or will not, have PF_KTHREAD. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] WARNING in kthread_is_per_cpu 2021-04-20 10:11 ` Peter Zijlstra @ 2021-04-20 10:17 ` Valentin Schneider 0 siblings, 0 replies; 10+ messages in thread From: Valentin Schneider @ 2021-04-20 10:17 UTC (permalink / raw) To: Peter Zijlstra Cc: syzbot, bp, dwmw, hpa, linux-kernel, luto, mingo, syzkaller-bugs, tglx, x86 On 20/04/21 12:11, Peter Zijlstra wrote: > On Tue, Apr 20, 2021 at 10:43:43AM +0100, Valentin Schneider wrote: >> On 20/04/21 10:51, Peter Zijlstra wrote: > >> > I think free_kthread_struct() is ok, because a task at that point in its >> > lifetime cannot be also doing exec(). >> > >> >> What if it's one of those kthreads created by directly invoking >> kernel_thread()? AFAICT right now it's only umh, and that one does execve() >> so it ends up stripped of PF_KTHREAD. It could however go through an error >> path, i.e. not call exec, and exit, giving us: >> >> put_task_struct(p) >> `\ >> free_task(p) >> `\ >> if (tsk->flags & PF_KTHREAD) >> free_kthread_struct(tsk); >> `\ >> to_kthread(p) > > I'm not following, at the point we hit free_task() it had better be dead > and p->flags had better be stable. Either it will, or will not, have > PF_KTHREAD. Bah, don't mind me, for some reason I was obsessed by that umh thing of having (p->flags & PF_KTHREAD) && !p->set_child_tid but that's not a problem there. Sorry about that. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [tip: sched/core] kthread: Fix PF_KTHREAD vs to_kthread() race 2021-04-20 8:51 ` Peter Zijlstra 2021-04-20 9:43 ` Valentin Schneider @ 2021-04-22 7:36 ` tip-bot2 for Peter Zijlstra 1 sibling, 0 replies; 10+ messages in thread From: tip-bot2 for Peter Zijlstra @ 2021-04-22 7:36 UTC (permalink / raw) To: linux-tip-commits Cc: Peter Zijlstra (Intel), Valentin Schneider, x86, linux-kernel The following commit has been merged into the sched/core branch of tip: Commit-ID: 3a7956e25e1d7b3c148569e78895e1f3178122a9 Gitweb: https://git.kernel.org/tip/3a7956e25e1d7b3c148569e78895e1f3178122a9 Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Tue, 20 Apr 2021 10:18:17 +02:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Wed, 21 Apr 2021 13:55:42 +02:00 kthread: Fix PF_KTHREAD vs to_kthread() race The kthread_is_per_cpu() construct relies on only being called on PF_KTHREAD tasks (per the WARN in to_kthread). This gives rise to the following usage pattern: if ((p->flags & PF_KTHREAD) && kthread_is_per_cpu(p)) However, as reported by syzcaller, this is broken. The scenario is: CPU0 CPU1 (running p) (p->flags & PF_KTHREAD) // true begin_new_exec() me->flags &= ~(PF_KTHREAD|...); kthread_is_per_cpu(p) to_kthread(p) WARN(!(p->flags & PF_KTHREAD) <-- *SPLAT* Introduce __to_kthread() that omits the WARN and is sure to check both values. Use this to remove the problematic pattern for kthread_is_per_cpu() and fix a number of other kthread_*() functions that have similar issues but are currently not used in ways that would expose the problem. Notably kthread_func() is only ever called on 'current', while kthread_probe_data() is only used for PF_WQ_WORKER, which implies the task is from kthread_create*(). Fixes: ac687e6e8c26 ("kthread: Extract KTHREAD_IS_PER_CPU") Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Reviewed-by: Valentin Schneider <Valentin.Schneider@arm.com> Link: https://lkml.kernel.org/r/YH6WJc825C4P0FCK@hirez.programming.kicks-ass.net --- kernel/kthread.c | 33 +++++++++++++++++++++++++++------ kernel/sched/core.c | 2 +- kernel/sched/fair.c | 2 +- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/kernel/kthread.c b/kernel/kthread.c index 1578973..6d3c488 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -84,6 +84,25 @@ static inline struct kthread *to_kthread(struct task_struct *k) return (__force void *)k->set_child_tid; } +/* + * Variant of to_kthread() that doesn't assume @p is a kthread. + * + * Per construction; when: + * + * (p->flags & PF_KTHREAD) && p->set_child_tid + * + * the task is both a kthread and struct kthread is persistent. However + * PF_KTHREAD on it's own is not, kernel_thread() can exec() (See umh.c and + * begin_new_exec()). + */ +static inline struct kthread *__to_kthread(struct task_struct *p) +{ + void *kthread = (__force void *)p->set_child_tid; + if (kthread && !(p->flags & PF_KTHREAD)) + kthread = NULL; + return kthread; +} + void free_kthread_struct(struct task_struct *k) { struct kthread *kthread; @@ -168,8 +187,9 @@ EXPORT_SYMBOL_GPL(kthread_freezable_should_stop); */ void *kthread_func(struct task_struct *task) { - if (task->flags & PF_KTHREAD) - return to_kthread(task)->threadfn; + struct kthread *kthread = __to_kthread(task); + if (kthread) + return kthread->threadfn; return NULL; } EXPORT_SYMBOL_GPL(kthread_func); @@ -199,10 +219,11 @@ EXPORT_SYMBOL_GPL(kthread_data); */ void *kthread_probe_data(struct task_struct *task) { - struct kthread *kthread = to_kthread(task); + struct kthread *kthread = __to_kthread(task); void *data = NULL; - copy_from_kernel_nofault(&data, &kthread->data, sizeof(data)); + if (kthread) + copy_from_kernel_nofault(&data, &kthread->data, sizeof(data)); return data; } @@ -514,9 +535,9 @@ void kthread_set_per_cpu(struct task_struct *k, int cpu) set_bit(KTHREAD_IS_PER_CPU, &kthread->flags); } -bool kthread_is_per_cpu(struct task_struct *k) +bool kthread_is_per_cpu(struct task_struct *p) { - struct kthread *kthread = to_kthread(k); + struct kthread *kthread = __to_kthread(p); if (!kthread) return false; diff --git a/kernel/sched/core.c b/kernel/sched/core.c index fcb35ae..4a0668a 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -7667,7 +7667,7 @@ static void balance_push(struct rq *rq) * histerical raisins. */ if (rq->idle == push_task || - ((push_task->flags & PF_KTHREAD) && kthread_is_per_cpu(push_task)) || + kthread_is_per_cpu(push_task) || is_migration_disabled(push_task)) { /* diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 7ea3b93..1d75af1 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -7612,7 +7612,7 @@ int can_migrate_task(struct task_struct *p, struct lb_env *env) return 0; /* Disregard pcpu kthreads; they are where they need to be. */ - if ((p->flags & PF_KTHREAD) && kthread_is_per_cpu(p)) + if (kthread_is_per_cpu(p)) return 0; if (!cpumask_test_cpu(env->dst_cpu, p->cpus_ptr)) { ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-04-22 7:36 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-19 10:36 [syzbot] WARNING in kthread_is_per_cpu syzbot 2021-04-19 11:30 ` Thomas Gleixner 2021-04-19 11:31 ` Valentin Schneider 2021-04-19 18:45 ` Peter Zijlstra 2021-04-19 19:58 ` Valentin Schneider 2021-04-20 8:51 ` Peter Zijlstra 2021-04-20 9:43 ` Valentin Schneider 2021-04-20 10:11 ` Peter Zijlstra 2021-04-20 10:17 ` Valentin Schneider 2021-04-22 7:36 ` [tip: sched/core] kthread: Fix PF_KTHREAD vs to_kthread() race tip-bot2 for Peter Zijlstra
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.