All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.