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

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

[Andy Lutomirski]

My v4 series was doing pretty well until this explosion:

On Sun, Jun 26, 2016 at 9:41 PM, kernel test robot
<xiaolong.ye@intel.com> wrote:
>
>
> 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")
>

...

> [    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.

> [    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
> [    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

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

[Peter Zijlstra]

On Sun, Jun 26, 2016 at 10:22:32PM -0700, Andy Lutomirski wrote:
> 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.

cute

> 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.

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

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

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

[Oleg Nesterov]

On 06/26, Andy Lutomirski wrote:
>
> 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.

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

> 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.

> 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.

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

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 7:54 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/26, Andy Lutomirski wrote:
>>
>> 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.
>
> Yes, I forgot this when we discussed the problems with ti->flags/etc...
>
>> 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.
>
>> 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.

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

[Oleg Nesterov]

On 06/27, Andy Lutomirski wrote:
>
> On Mon, Jun 27, 2016 at 7:54 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> >> 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.

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.

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

[Oleg Nesterov]

On 06/27, Oleg Nesterov wrote:
>
> On 06/27, Andy Lutomirski wrote:
> >
> > 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.

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

[Andy Lutomirski]

On Tue, Jun 28, 2016 at 11:58 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/27, Oleg Nesterov wrote:
>>
>> On 06/27, Andy Lutomirski wrote:
>> >
>> > 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.

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

>
> Oleg.
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

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

[Oleg Nesterov]

On 06/28, Andy Lutomirski wrote:
>
> On Tue, Jun 28, 2016 at 11:58 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > 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?

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.

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

[Andy Lutomirski]

On Tue, Jun 28, 2016 at 1:12 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/28, Andy Lutomirski wrote:
>>
>> On Tue, Jun 28, 2016 at 11:58 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> > 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?
>
> 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

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

[Linus Torvalds]

On Tue, Jun 28, 2016 at 1:54 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> 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

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

[Linus Torvalds]

On Tue, Jun 28, 2016 at 2:14 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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

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

[Andy Lutomirski]

On Tue, Jun 28, 2016 at 2:14 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Jun 28, 2016 at 1:54 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> 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?

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.

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

[Linus Torvalds]

On Tue, Jun 28, 2016 at 2:21 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
>  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..

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

Fair enough.

             Linus

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

[Linus Torvalds]

On Tue, Jun 28, 2016 at 2:35 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Jun 28, 2016 at 2:21 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
>> 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

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

[Oleg Nesterov]

On 06/28, Linus Torvalds wrote:
>
> 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.
---

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index e691b6a..7667bc62 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -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);
diff --git a/kernel/fork.c b/kernel/fork.c
index 97122f9..8643248 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -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);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 9ff173d..3a4921f 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -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);
 }
 

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

[Oleg Nesterov]

On 06/28, Andy Lutomirski wrote:
>
> On Tue, Jun 28, 2016 at 1:12 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > 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,

> 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.

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

[Andy Lutomirski]

On Tue, Jun 28, 2016 at 3:59 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/28, Andy Lutomirski wrote:
>>
>> On Tue, Jun 28, 2016 at 1:12 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> > 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,
>
>> 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

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

[Oleg Nesterov]

On 06/29, Andy Lutomirski wrote:
>
> 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 <oleg@redhat.com>
---
 kernel/kthread.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 9ff173d..4ab4c37 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -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);
-- 
2.5.0

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1619 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6350 bytes --]

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

[Oleg Nesterov]

On 06/30, kbuild test robot wrote:
>
> 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.


> 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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4167 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23018 bytes --]

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

[Andy Lutomirski]

On Wed, Jun 29, 2016 at 11:03 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/29, Andy Lutomirski wrote:
>>
>> 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!

>
> 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

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

[Andy Lutomirski]

On Tue, Jun 28, 2016 at 11:58 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/27, Oleg Nesterov wrote:
>>
>> On 06/27, Andy Lutomirski wrote:
>> >
>> > 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.
>

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

--Andy

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

[Linus Torvalds]

On Sun, Jun 26, 2016 at 10:22 PM, Andy Lutomirski <luto@kernel.org> wrote:
>
> 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.

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