Skip to contentSkip to conversation

kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18)

PATCH In review Andy Lutomirski 6 participants 24 messages Jun 27, 2016 – Jun 29, 2016

Subsystems (3)
MM core Scheduler exec and binfmt
lore
Share & tools
lore

Andy Lutomirski

author
#0 lore raw

My v4 series was doing pretty well until this explosion:

Quoted 7 lines · kernel test robot

FYI, we noticed the following commit:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git x86/vmap_stack commit 26424589626d7f82d09d4e7c0569f9487b2e810a ("[DEBUG] force-enable CONFIG_VMAP_STACK")

...

Quoted 8 lines
[    4.425052] BUG: unable to handle kernel paging request at ffffc90000997f18
[    4.426645] IP: [<ffffffff81a9ace0>] _raw_spin_lock_irq+0x2c/0x3d
[    4.427869] PGD 1249e067 PUD 1249f067 PMD 11e4e067 PTE 0
[    4.429245] Oops: 0002 [#1] SMP
[    4.430086] Modules linked in:
[    4.430992] CPU: 0 PID: 1741 Comm: mount Not tainted 4.7.0-rc4-00258-g26424589 #1
[    4.432727] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Debian-1.8.2-1 04/01/2014
[    4.434646] task: ffff88000d950c80 ti: ffff88000d950c80 task.ti: ffff88000d950c80

Yeah, this line is meaningless with the thread_info cleanups, and I have it fixed for v5.

Quoted 29 lines
[    4.436406] RIP: 0010:[<ffffffff81a9ace0>]  [<ffffffff81a9ace0>] _raw_spin_lock_irq+0x2c/0x3d
[    4.438341] RSP: 0018:ffffc90000957c80  EFLAGS: 00010046
[    4.439438] RAX: 0000000000000000 RBX: 7fffffffffffffff RCX: 0000000000000a66
[    4.440735] RDX: 0000000000000001 RSI: ffff880013619bc0 RDI: ffffc90000997f18
[    4.442035] RBP: ffffc90000957c88 R08: 0000000000019bc0 R09: ffffffff81200748
[    4.443323] R10: ffffea0000474900 R11: 000000000001a2a0 R12: ffffc90000997f10
[    4.444614] R13: 0000000000000002 R14: ffffc90000997f18 R15: 00000000ffffffea
[    4.445896] FS:  00007f9ca6a32700(0000) GS:ffff880013600000(0000) knlGS:0000000000000000
[    4.447690] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    4.448819] CR2: ffffc90000997f18 CR3: 000000000d87c000 CR4: 00000000000006f0
[    4.450102] Stack:
[    4.450810]  ffffc90000997f18 ffffc90000957d00 ffffffff81a982eb 0000000000000246
[    4.452827]  0000000000000000 ffffc90000957d00 ffffffff8112584b 0000000000000000
[    4.454838]  0000000000000246 ffff88000e27f6bc 0000000000000000 ffff88000e27f080
[    4.456845] Call Trace:
[    4.457616]  [<ffffffff81a982eb>] wait_for_common+0x44/0x197
[    4.458719]  [<ffffffff8112584b>] ? try_to_wake_up+0x2dd/0x2ef
[    4.459877]  [<ffffffff81a9845b>] wait_for_completion+0x1d/0x1f
[    4.461027]  [<ffffffff8111db10>] kthread_stop+0x82/0x10a
[    4.462125]  [<ffffffff81117f08>] destroy_workqueue+0x10d/0x1cd
[    4.463347]  [<ffffffff81445236>] xfs_destroy_mount_workqueues+0x49/0x64
[    4.464620]  [<ffffffff81445c03>] xfs_fs_fill_super+0x2c0/0x49c
[    4.465807]  [<ffffffff8123547a>] mount_bdev+0x143/0x195
[    4.466937]  [<ffffffff81445943>] ? xfs_test_remount_options+0x5b/0x5b
[    4.468727]  [<ffffffff81444568>] xfs_fs_mount+0x15/0x17
[    4.469838]  [<ffffffff8123614a>] mount_fs+0x15/0x8c
[    4.470882]  [<ffffffff8124cfc4>] vfs_kern_mount+0x6a/0xfe
[    4.472005]  [<ffffffff8124fc2f>] do_mount+0x985/0xa9a
[    4.473078]  [<ffffffff811e0846>] ? strndup_user+0x3a/0x6a
Quoted 9 lines
[    4.474193]  [<ffffffff8124ff6a>] SyS_mount+0x77/0x9f
[    4.475255]  [<ffffffff81a9b081>] entry_SYSCALL_64_fastpath+0x1f/0xbd
[    4.476463] Code: 66 66 66 90 55 48 89 e5 50 48 89 7d f8 fa 66 66 90 66 66 90 e8 2d 0a 70 ff 65 ff 05 73 18 57 7e 31 c0 ba 01 00 00 00 48 8b 7d f8 <f0> 0f b1 17 85 c0 74 07 89 c6 e8 3e 20 6a ff c9 c3 66 66 66 66
[    4.484413] RIP  [<ffffffff81a9ace0>] _raw_spin_lock_irq+0x2c/0x3d
[    4.485639]  RSP <ffffc90000957c80>
[    4.486509] CR2: ffffc90000997f18
[    4.487366] ---[ end trace 79763b41869f2580 ]---
[    4.488367] Kernel panic - not syncing: Fatal exception

kthread_stop is *sick*.

    struct kthread self;

...

    current->vfork_done = &self.exited;

...

    do_exit(ret);

And then some other thread goes and waits for the completion, which is *on the stack*, which, in any sane world (e.g. with my series applied), is long gone by then.

But this is broken even without any changes: since when is gcc guaranteed to preserve the stack contents when a function ends with a sibling call, let alone with a __noreturn call?

Is there seriously no way to directly wait for a struct task_struct to exit? Could we, say, kmalloc the completion (or maybe even the whole struct kthread) and (ick!) hang it off ->vfork_done?

Linus, maybe it's time for you to carve another wax figurine.

--Andy

maintainer
#1 lore raw
Quoted 8 lines · Andy Lutomirski

kthread_stop is *sick*.

    struct kthread self;

...

    current->vfork_done = &self.exited;
Quoted 7 lines

...

    do_exit(ret);

And then some other thread goes and waits for the completion, which is *on the stack*, which, in any sane world (e.g. with my series applied), is long gone by then.

cute

Quoted 3 lines

Is there seriously no way to directly wait for a struct task_struct to exit? Could we, say, kmalloc the completion (or maybe even the whole struct kthread)

I suppose the easiest is to merge struct kthread into struct kthread_create_info, and maybe union some bits if we're really worried about size.

Quoted 1 line

and (ick!) hang it off ->vfork_done?

This... cleanup is going to be somewhat icky.

#2 lore raw
In reply to Andy Lutomirski
Quoted 9 lines · Andy Lutomirski

kthread_stop is *sick*.

    struct kthread self;

...

    current->vfork_done = &self.exited;
Quoted 7 lines

...

    do_exit(ret);

And then some other thread goes and waits for the completion, which is *on the stack*, which, in any sane world (e.g. with my series applied), is long gone by then.

Yes, I forgot this when we discussed the problems with ti->flags/etc...

Quoted 3 lines

But this is broken even without any changes: since when is gcc guaranteed to preserve the stack contents when a function ends with a sibling call, let alone with a __noreturn call?

I don't know if gcc can actually drop the stack frame in this case, but even if it can this looks fixeable.

Quoted 3 lines

Is there seriously no way to directly wait for a struct task_struct to exit? Could we, say, kmalloc the completion (or maybe even the whole struct kthread) and (ick!) hang it off ->vfork_done?

Sure we can... And yes, I think we need to alloc the whole struct kthread. Just another (unfortunate) complication, the current code is simple.

And probably kthread/kthread_stop should switch to task_work_exit().

Oleg.

#3 lore raw
Quoted 27 lines · Oleg Nesterov
Quoted 9 lines · Andy Lutomirski

kthread_stop is *sick*.

    struct kthread self;

...

    current->vfork_done = &self.exited;
Quoted 7 lines

...

    do_exit(ret);

And then some other thread goes and waits for the completion, which is *on the stack*, which, in any sane world (e.g. with my series applied), is long gone by then.

Yes, I forgot this when we discussed the problems with ti->flags/etc...

Quoted 3 lines

But this is broken even without any changes: since when is gcc guaranteed to preserve the stack contents when a function ends with a sibling call, let alone with a __noreturn call?

I don't know if gcc can actually drop the stack frame in this case, but even if it can this looks fixeable.

Quoted 8 lines
Quoted 3 lines

Is there seriously no way to directly wait for a struct task_struct to exit? Could we, say, kmalloc the completion (or maybe even the whole struct kthread) and (ick!) hang it off ->vfork_done?

Sure we can... And yes, I think we need to alloc the whole struct kthread. Just another (unfortunate) complication, the current code is simple.

And probably kthread/kthread_stop should switch to task_work_exit().

Want to send a patch? I could do it, but you understand this code much better than I do.

#4 lore raw
Quoted 7 lines · Andy Lutomirski
Quoted 5 lines · Oleg Nesterov
Quoted 4 lines

Is there seriously no way to directly wait for a struct task_struct to exit? Could we, say, kmalloc the completion (or maybe even the whole struct kthread) and (ick!) hang it off ->vfork_done?

Quoted 7 lines
Quoted 4 lines

Sure we can... And yes, I think we need to alloc the whole struct kthread. Just another (unfortunate) complication, the current code is simple.

And probably kthread/kthread_stop should switch to task_work_exit().

Want to send a patch? I could do it, but you understand this code much better than I do.

Well, I'll try to do this tomorrow unless you do it.

The problem is not the wait_for_completion(vfork_done) in kthread_stop(), we can avoid this immediately if we change it to use task_work_add().

The problem is to_live_kthread(). And damn, it seems to me that in the long term we can simpy kill "struct kthread" altogether. All we need is kthread_data() and this is just a pointer. flags,cpu,parked should go into smp_hotplug_thread.

But this needs changes, so meanwhile we will have to kmalloc() it and free in free_task().

Or perhaps you can simply move "struct kthread" into task_struct as as temporary/ugly but trivial fix, then we can think more.

Oleg.

#5 lore raw
Quoted 7 lines · Oleg Nesterov
Quoted 3 lines · Andy Lutomirski

Want to send a patch? I could do it, but you understand this code much better than I do.

Well, I'll try to do this tomorrow unless you do it.

I have cloned luto/linux.git to see if kthread_stop() can pin ->stack somehow, but it seems this is not possible, finish_task_switch() does free_thread_stack() unconditionally.

Then how (say) proc_pid_stack() can work? If it hits the task which is alreay dead we are (probably) fine, valid_stack_ptr() should fail iiuc.

But what if we race with the last schedule() ? "addr = *stack" can read the already vfree'ed memory, no?

Looks like print_context_stack/etc need probe_kernel_address or I missed something.

Oleg.

#6 lore raw
Quoted 13 lines · Oleg Nesterov
Quoted 7 lines · Oleg Nesterov
Quoted 3 lines · Andy Lutomirski

Want to send a patch? I could do it, but you understand this code much better than I do.

Well, I'll try to do this tomorrow unless you do it.

I have cloned luto/linux.git to see if kthread_stop() can pin ->stack somehow, but it seems this is not possible, finish_task_switch() does free_thread_stack() unconditionally.

Quoted 8 lines

Then how (say) proc_pid_stack() can work? If it hits the task which is alreay dead we are (probably) fine, valid_stack_ptr() should fail iiuc.

But what if we race with the last schedule() ? "addr = *stack" can read the already vfree'ed memory, no?

Looks like print_context_stack/etc need probe_kernel_address or I missed something.

Yuck. I suppose I could add a reference count to protect the stack. Would that simplify the kthread code?

It's too bad that all the kthread users use get_task_struct instead of, say get_kthread (which doesn't exist).

--Andy

Quoted 3 lines

Oleg.

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
#7 lore raw
Quoted 6 lines · Andy Lutomirski
Quoted 4 lines · Oleg Nesterov

Then how (say) proc_pid_stack() can work? If it hits the task which is alreay dead we are (probably) fine, valid_stack_ptr() should fail iiuc.

Quoted 8 lines
Quoted 5 lines

But what if we race with the last schedule() ? "addr = *stack" can read the already vfree'ed memory, no?

Looks like print_context_stack/etc need probe_kernel_address or I missed something.

Yuck. I suppose I could add a reference count to protect the stack. Would that simplify the kthread code?

Well yes, that is why I asked. So please tell me if you are going to do this...

But we can fix kthread code without this hack which we do not need in the long term anyway. Unfortunaly we need to cleanup kernel/smpboot.c first. And I was going to do this a long ago for quite different reason ;)

So please forget unless you see another reason for this change.

Oleg.

#8 lore raw
Quoted 16 lines · Oleg Nesterov
Quoted 6 lines · Andy Lutomirski
Quoted 4 lines · Oleg Nesterov

Then how (say) proc_pid_stack() can work? If it hits the task which is alreay dead we are (probably) fine, valid_stack_ptr() should fail iiuc.

Quoted 8 lines
Quoted 5 lines

But what if we race with the last schedule() ? "addr = *stack" can read the already vfree'ed memory, no?

Looks like print_context_stack/etc need probe_kernel_address or I missed something.

Yuck. I suppose I could add a reference count to protect the stack. Would that simplify the kthread code?

Quoted 9 lines

Well yes, that is why I asked. So please tell me if you are going to do this...

But we can fix kthread code without this hack which we do not need in the long term anyway. Unfortunaly we need to cleanup kernel/smpboot.c first. And I was going to do this a long ago for quite different reason ;)

So please forget unless you see another reason for this change.

But I might need to that anyway for procfs to read the the stack, right? Do you see another way to handle that case?

I'm thinking of adding:

void *try_get_task_stack(struct task_struct *tsk); void put_task_stack(struct task_struct *tsk);

where try_get_task_stack can return NULL.

--Andy

#9 lore raw
Quoted 3 lines · Andy Lutomirski

But I might need to that anyway for procfs to read the the stack, right? Do you see another way to handle that case?

I think the other way to handle the kernel stack reading would be to simply make the stack freeing be RCU-delayed, and use the RCU list itself as the stack cache.

That way reading the stack is ok in a RCU context, although you might end up reading a stack that has been re-used.

Would that work for people?

Linus

#10 lore raw
Quoted 4 lines · Linus Torvalds

I think the other way to handle the kernel stack reading would be to simply make the stack freeing be RCU-delayed, and use the RCU list itself as the stack cache.

That said, if you end up having to have a stack reference count for other reasons anyway, then I guess that creating a try_get_task_stack()/put_task_stack() model is fine.

The RCU thing would only work for pure readers that can handle stale data (which would be the stack trace thing). It wouldn't work for the kthread to_live_kthread() case.

Linus

#11 lore raw
In reply to Linus Torvalds
Quoted 5 lines · Linus Torvalds
Quoted 3 lines · Andy Lutomirski

But I might need to that anyway for procfs to read the the stack, right? Do you see another way to handle that case?

Quoted 8 lines

I think the other way to handle the kernel stack reading would be to simply make the stack freeing be RCU-delayed, and use the RCU list itself as the stack cache.

That way reading the stack is ok in a RCU context, although you might end up reading a stack that has been re-used.

Would that work for people?

I don't think I understand your proposal. We already delay freeing the stack for RCU. If we continue doing it, then, under workloads like my benchmark, the RCU list gets quite large. Or are you suggesting that we actually make a list somewhere of stacks that are nominally unused but are still around for RCU's benefit and then scavenge from that lest when we need a new stack? If so, that seems considerably more complicated than just adding a reference count.

Also, my inner security nerd says that letting /proc potentially read the wrong process's stack is bad news.

#12 lore raw
Quoted 5 lines · Andy Lutomirski

Or are you
suggesting that we actually make a list somewhere of stacks that are nominally unused but are still around for RCU's benefit and then scavenge from that lest when we need a new stack?

Yes. We'd have to make our own list (rather than just use the RCU list itself where everything goes), but..

Quoted 1 line

If so, that seems considerably more complicated than just adding a reference count.

Fair enough.

Linus

#13 lore raw
Quoted 5 lines · Linus Torvalds
Quoted 1 line · Andy Lutomirski

If so, that seems considerably more complicated than just adding a reference count.

Fair enough.

Ahh, and if you put the reference count just in the task_struct (next to the ->stack pointer), then I guess that's particularly trivial.

Then try_get_task_stack(tsk) becomes

     void *try_get_task_stack(struct task_struct *tsk)
     {
          void *stack = tsk->stack;
          if (!atomic_inc_not_zero(&tsk->stackref))
               stack = NULL;
          return stack;
     }

ok, color me convinced.

Linus

#14 lore raw
Quoted 10 lines · Linus Torvalds

Then try_get_task_stack(tsk) becomes

     void *try_get_task_stack(struct task_struct *tsk)
     {
          void *stack = tsk->stack;
          if (!atomic_inc_not_zero(&tsk->stackref))
               stack = NULL;
          return stack;
     }

Yes, and then we can trivilly fix the users of to_live_kthread(). So I'll wait for this change and send the simple fix on top of it.

Otherwise I'll send another ugly hack (see below).

Oleg.
---

include/linux/kthread.h +1 -0
index e691b6a..7667bc62 100644
@@ -37,6 +37,7 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
 	__k;								   \
 })
 
+void set_kthread_struct(void *kthread);
 void kthread_bind(struct task_struct *k, unsigned int cpu);
 void kthread_bind_mask(struct task_struct *k, const struct cpumask *mask);
 int kthread_stop(struct task_struct *k);
kernel/fork.c +18 -0
index 97122f9..8643248 100644
@@ -326,6 +326,23 @@ void release_task_stack(struct task_struct *tsk)
 #endif
 }
 
+void set_kthread_struct(void *kthread)
+{
+	/*
+	 * This is the ugly but simple hack we will hopefully remove soon.
+	 * We abuse ->set_child_tid to avoid the new member and because it
+	 * can't be wrongly copied by copy_process(). We also rely on fact
+	 * that the caller can't exec, so PF_KTHREAD can't be cleared.
+	 */
+	current->set_child_tid = (__force void __user *)kthread;
+}
+
+static void free_kthread_struct(struct task_struct *tsk)
+{
+	if (tsk->flags & PF_KTHREAD)
+		kfree((__force void *)tsk->set_child_tid); /* can be NULL */
+}
+
 void free_task(struct task_struct *tsk)
 {
 #ifndef CONFIG_THREAD_INFO_IN_TASK

@@ -345,6 +362,7 @@ void free_task(struct task_struct *tsk)
 	ftrace_graph_exit_task(tsk);
 	put_seccomp_filter(tsk);
 	arch_release_task_struct(tsk);
+	free_kthread_struct(tsk);
 	free_task_struct(tsk);
 }
 EXPORT_SYMBOL(free_task);
kernel/kthread.c +18 -10
index 9ff173d..3a4921f 100644
@@ -181,14 +181,11 @@ static int kthread(void *_create)
 	int (*threadfn)(void *data) = create->threadfn;
 	void *data = create->data;
 	struct completion *done;
-	struct kthread self;
+	struct kthread *self;
 	int ret;
 
-	self.flags = 0;
-	self.data = data;
-	init_completion(&self.exited);
-	init_completion(&self.parked);
-	current->vfork_done = &self.exited;
+	self = kmalloc(sizeof(*self), GFP_KERNEL);
+	set_kthread_struct(self);
 
 	/* If user was SIGKILLed, I release the structure. */
 	done = xchg(&create->done, NULL);

@@ -196,6 +193,19 @@ static int kthread(void *_create)
 		kfree(create);
 		do_exit(-EINTR);
 	}
+
+	if (!self) {
+		create->result = ERR_PTR(-ENOMEM);
+		complete(done);
+		do_exit(-ENOMEM);
+	}
+
+	self->flags = 0;
+	self->data = data;
+	init_completion(&self->exited);
+	init_completion(&self->parked);
+	current->vfork_done = &self->exited;
+
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	create->result = current;

@@ -203,12 +213,10 @@ static int kthread(void *_create)
 	schedule();
 
 	ret = -EINTR;
-
-	if (!test_bit(KTHREAD_SHOULD_STOP, &self.flags)) {
-		__kthread_parkme(&self);
+	if (!test_bit(KTHREAD_SHOULD_STOP, &self->flags)) {
+		__kthread_parkme(self);
 		ret = threadfn(data);
 	}
-	/* we can't just return, we must preserve "self" on stack */
 	do_exit(ret);
 }
#15 lore raw
In reply to Andy Lutomirski
Quoted 8 lines · Andy Lutomirski
Quoted 3 lines · Oleg Nesterov

So please forget unless you see another reason for this change.

But I might need to that anyway for procfs to read the the stack, right? Do you see another way to handle that case?

Well, we could use probe_kernel_text() and recheck tsk->stack != NULL after this.

But,

Quoted 4 lines

I'm thinking of adding:

void *try_get_task_stack(struct task_struct *tsk); void put_task_stack(struct task_struct *tsk);

Yes, agreed, this looks better.

Oleg.

#16 lore raw
Quoted 15 lines · Oleg Nesterov
Quoted 8 lines · Andy Lutomirski
Quoted 3 lines · Oleg Nesterov

So please forget unless you see another reason for this change.

But I might need to that anyway for procfs to read the the stack, right? Do you see another way to handle that case?

Well, we could use probe_kernel_text() and recheck tsk->stack != NULL after this.

But,

Quoted 9 lines
Quoted 4 lines

I'm thinking of adding:

void *try_get_task_stack(struct task_struct *tsk); void put_task_stack(struct task_struct *tsk);

Yes, agreed, this looks better.

Oleg.

I pushed that change to my tree (seems to work well enough to boot without warnings as long as I don't unmount XFS, but not particularly well tested). Want to refresh your patch on top?

I'll probably have to split the patch to introduce no-op try_get_task_stack / put_task_stack first so I can avoid breaking bisection, but the interface shouldn't change unless something's wrong with it.

--Andy

#17 lore raw

PATCH 1/1 kthread: to_live_kthread() needs try_get_task_stack()

Quoted 4 lines · Andy Lutomirski

I pushed that change to my tree (seems to work well enough to boot without warnings as long as I don't unmount XFS, but not particularly well tested). Want to refresh your patch on top?

Please see the trivial fix below. Compile tested, but looks obvious.

Btw, why free_thread_stack() calls vfree() with irqs disabled? Doesn't look good and perhaps even wrong; at least vmap_debug_free_range() does flush_tlb_kernel_range() and smp_call_function() can deadlock?

-------------------------------------------------------------------------------
Subject: [PATCH] kthread: to_live_kthread() needs try_get_task_stack()

get_task_struct(tsk) no longer pins tsk->stack so all users of to_live_kthread() should do try_get_task_stack/put_task_stack to protect "struct kthread" which lives on kthread's stack.

TODO: Kill to_live_kthread(), perhaps we can even kill "struct kthread" too, and rework kthread_stop(), it can use task_work_add() to sync with the exiting kernel thread.

Signed-off-by Oleg Nesterov <[email protected]>
---
 kernel/kthread.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
kernel/kthread.c +6 -2
index 9ff173d..4ab4c37 100644
@@ -64,7 +64,7 @@ static inline struct kthread *to_kthread(struct task_struct *k)
 static struct kthread *to_live_kthread(struct task_struct *k)
 {
 	struct completion *vfork = ACCESS_ONCE(k->vfork_done);
-	if (likely(vfork))
+	if (likely(vfork) && try_get_task_stack(k))
 		return __to_kthread(vfork);
 	return NULL;
 }

@@ -425,8 +425,10 @@ void kthread_unpark(struct task_struct *k)
 {
 	struct kthread *kthread = to_live_kthread(k);
 
-	if (kthread)
+	if (kthread) {
 		__kthread_unpark(k, kthread);
+		put_task_stack(k);
+	}
 }
 EXPORT_SYMBOL_GPL(kthread_unpark);
 

@@ -455,6 +457,7 @@ int kthread_park(struct task_struct *k)
 				wait_for_completion(&kthread->parked);
 			}
 		}
+		put_task_stack(k);
 		ret = 0;
 	}
 	return ret;

@@ -490,6 +493,7 @@ int kthread_stop(struct task_struct *k)
 		__kthread_unpark(k, kthread);
 		wake_up_process(k);
 		wait_for_completion(&kthread->exited);
+		put_task_stack(k);
 	}
 	ret = k->exit_code;
 	put_task_struct(k);
Signature · 1 line
2.5.0
build bot
#18 lore raw

Re: [PATCH] kthread: to_live_kthread() needs try_get_task_stack()

url: https://github.com/0day-ci/linux/commits/Oleg-Nesterov/kthread-to_live_kthread-needs-try_get_task_stack/20160630-020824 config: i386-tinyconfig (attached as .config) compiler: gcc-6 (Debian…

Hi,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.7-rc5 next-20160629]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Oleg-Nesterov/kthread-to_live_kthread-needs-try_get_task_stack/20160630-020824
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430 reproduce:

        # save the attached .config to linux build tree
        make ARCH=i386

All errors (new ones prefixed by >>):

kernel/kthread.c: In function 'to_live_kthread':

>> kernel/kthread.c:67:23: error: implicit declaration of function 'try_get_task_stack' [-Werror=implicit-function-declaration]
     if (likely(vfork) && try_get_task_stack(k))
                          ^~~~~~~~~~~~~~~~~~
   kernel/kthread.c: In function 'kthread_unpark':
>> kernel/kthread.c:430:3: error: implicit declaration of function 'put_task_stack' [-Werror=implicit-function-declaration]
      put_task_stack(k);
      ^~~~~~~~~~~~~~

cc1: some warnings being treated as errors

vim +/try_get_task_stack +67 kernel/kthread.c

    61		return __to_kthread(k->vfork_done);
    62	}
    63	
    64	static struct kthread *to_live_kthread(struct task_struct *k)
    65	{
    66		struct completion *vfork = ACCESS_ONCE(k->vfork_done);
  > 67		if (likely(vfork) && try_get_task_stack(k))
    68			return __to_kthread(vfork);
    69		return NULL;
    70	}

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

#19 lore raw

Re: [PATCH] kthread: to_live_kthread() needs try_get_task_stack()

Quoted 6 lines · kbuild test robot

Hi,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.7-rc5 next-20160629]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

Yes, please ignore this, the patch is based on Andy's x86/vmap_stack tree.

Quoted 35 lines

url: https://github.com/0day-ci/linux/commits/Oleg-Nesterov/kthread-to_live_kthread-needs-try_get_task_stack/20160630-020824
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430 reproduce:

        # save the attached .config to linux build tree
        make ARCH=i386

All errors (new ones prefixed by >>):

kernel/kthread.c: In function 'to_live_kthread':

Quoted 1 line
Quoted 1 line

kernel/kthread.c:67:23: error: implicit declaration of function 'try_get_task_stack' [-Werror=implicit-function-declaration]

     if (likely(vfork) && try_get_task_stack(k))
                          ^~~~~~~~~~~~~~~~~~

kernel/kthread.c: In function 'kthread_unpark':

Quoted 1 line
Quoted 1 line

kernel/kthread.c:430:3: error: implicit declaration of function 'put_task_stack' [-Werror=implicit-function-declaration]

      put_task_stack(k);
      ^~~~~~~~~~~~~~

cc1: some warnings being treated as errors

vim +/try_get_task_stack +67 kernel/kthread.c

    61		return __to_kthread(k->vfork_done);
    62	}
    63	
    64	static struct kthread *to_live_kthread(struct task_struct *k)
    65	{
    66		struct completion *vfork = ACCESS_ONCE(k->vfork_done);
Quoted 1 line

67 if (likely(vfork) && try_get_task_stack(k))

    68			return __to_kthread(vfork);
    69		return NULL;
    70	}

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

build bot
#20 lore raw

Re: [PATCH] kthread: to_live_kthread() needs try_get_task_stack()

In reply to Oleg Nesterov
url: https://github.com/0day-ci/linux/commits/Oleg-Nesterov/kthread-to_live_kthread-needs-try_get_task_stack/20160630-020824 config: x86_64-randconfig-s3-06300221 (attached as .config) compiler…

Hi,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.7-rc5 next-20160629]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Oleg-Nesterov/kthread-to_live_kthread-needs-try_get_task_stack/20160630-020824
config: x86_64-randconfig-s3-06300221 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430 reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All warnings (new ones prefixed by >>):

In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/uapi/linux/capability.h:16,
from include/linux/capability.h:15,
from include/linux/sched.h:15,
from kernel/kthread.c:8:
kernel/kthread.c: In function 'to_live_kthread':
kernel/kthread.c:67:23: error: implicit declaration of function 'try_get_task_stack' [-Werror=implicit-function-declaration]

     if (likely(vfork) && try_get_task_stack(k))
                          ^
   include/linux/compiler.h:151:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> kernel/kthread.c:67:2: note: in expansion of macro 'if'
     if (likely(vfork) && try_get_task_stack(k))
     ^~
   kernel/kthread.c: In function 'kthread_unpark':
   kernel/kthread.c:430:3: error: implicit declaration of function 'put_task_stack' [-Werror=implicit-function-declaration]
      put_task_stack(k);
      ^~~~~~~~~~~~~~

cc1: some warnings being treated as errors

vim +/if +67 kernel/kthread.c

2 * Copyright (C) 2004 IBM Corporation, Rusty Russell.
3 *
4 * Creation is done via kthreadd, so that we get a clean environment
5 * even if we're invoked from userspace (think modprobe, hotplug cpu,
6 * etc.).
7 */

   > 8	#include <linux/sched.h>
     9	#include <linux/kthread.h>
    10	#include <linux/completion.h>
    11	#include <linux/err.h>
    12	#include <linux/cpuset.h>
    13	#include <linux/unistd.h>
    14	#include <linux/file.h>
    15	#include <linux/export.h>
    16	#include <linux/mutex.h>
    17	#include <linux/slab.h>
    18	#include <linux/freezer.h>
    19	#include <linux/ptrace.h>
    20	#include <linux/uaccess.h>
    21	#include <trace/events/sched.h>
    22	
    23	static DEFINE_SPINLOCK(kthread_create_lock);
    24	static LIST_HEAD(kthread_create_list);
    25	struct task_struct *kthreadd_task;
    26	
    27	struct kthread_create_info
    28	{
    29		/* Information passed to kthread() from kthreadd. */
    30		int (*threadfn)(void *data);
    31		void *data;
    32		int node;
    33	
    34		/* Result passed back to kthread_create() from kthreadd. */
    35		struct task_struct *result;
    36		struct completion *done;
    37	
    38		struct list_head list;
    39	};
    40	
    41	struct kthread {
    42		unsigned long flags;
    43		unsigned int cpu;
    44		void *data;
    45		struct completion parked;
    46		struct completion exited;
    47	};
    48	
    49	enum KTHREAD_BITS {
    50		KTHREAD_IS_PER_CPU = 0,
    51		KTHREAD_SHOULD_STOP,
    52		KTHREAD_SHOULD_PARK,
    53		KTHREAD_IS_PARKED,
    54	};
    55	
    56	#define __to_kthread(vfork)	\
    57		container_of(vfork, struct kthread, exited)
    58	
    59	static inline struct kthread *to_kthread(struct task_struct *k)
    60	{
    61		return __to_kthread(k->vfork_done);
    62	}
    63	
    64	static struct kthread *to_live_kthread(struct task_struct *k)
    65	{
    66		struct completion *vfork = ACCESS_ONCE(k->vfork_done);
  > 67		if (likely(vfork) && try_get_task_stack(k))
    68			return __to_kthread(vfork);
    69		return NULL;
    70	}

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

#21 lore raw

Re: [PATCH] kthread: to_live_kthread() needs try_get_task_stack()

In reply to Oleg Nesterov
Quoted 7 lines · Oleg Nesterov
Quoted 4 lines · Andy Lutomirski

I pushed that change to my tree (seems to work well enough to boot without warnings as long as I don't unmount XFS, but not particularly well tested). Want to refresh your patch on top?

Please see the trivial fix below. Compile tested, but looks obvious.

I stuck it in my tree in the right place. Thanks!

Quoted 4 lines

Btw, why free_thread_stack() calls vfree() with irqs disabled? Doesn't look good and perhaps even wrong; at least vmap_debug_free_range() does flush_tlb_kernel_range() and smp_call_function() can deadlock?

Whoops, that's a bug. Fixed now.

--Andy

#22 lore raw
In reply to Oleg Nesterov
Quoted 7 lines · Oleg Nesterov
Quoted 6 lines · Oleg Nesterov
Quoted 3 lines · Andy Lutomirski

Want to send a patch? I could do it, but you understand this code much better than I do.

Quoted 9 lines
Quoted 1 line

Well, I'll try to do this tomorrow unless you do it.

I have cloned luto/linux.git to see if kthread_stop() can pin ->stack somehow, but it seems this is not possible, finish_task_switch() does free_thread_stack() unconditionally.

Then how (say) proc_pid_stack() can work? If it hits the task which is alreay dead we are (probably) fine, valid_stack_ptr() should fail iiuc.

I changed save_stack_trace_tsk() to use try_get_task_stack(). I think that's sufficient to fix this.

--Andy

#23 lore raw
In reply to Andy Lutomirski
Quoted 11 lines · Andy Lutomirski

kthread_stop is *sick*.

    struct kthread self;


    current->vfork_done = &self.exited;


    do_exit(ret);
Quoted 3 lines

And then some other thread goes and waits for the completion, which is *on the stack*, which, in any sane world (e.g. with my series applied), is long gone by then.

Yeah. To be fair, that used to work. And the waiter does actually get a reference to the task struct, and with the lifetime of the stack historically being the same as the task struct, it was even being fairly careful about it.

But yes, it's disgusting, and doesn't work in the new world order, and I think it should be fairly easy to fix. Although getting the lifetime right for a separately allocated "struct kthread_struct" might be a bit exciting.

Linus