Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1431724 > unrolled thread
| Started by | Andy Lutomirski <luto@kernel.org> |
|---|---|
| First post | 2016-06-27 07:30 +0200 |
| Last post | 2016-06-27 19:20 +0200 |
| Articles | 20 on this page of 24 — 6 participants |
Back to article view | Back to linux.kernel
kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Andy Lutomirski <luto@kernel.org> - 2016-06-27 07:30 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Peter Zijlstra <peterz@infradead.org> - 2016-06-27 10:30 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Oleg Nesterov <oleg@redhat.com> - 2016-06-27 17:00 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Andy Lutomirski <luto@amacapital.net> - 2016-06-27 17:50 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Oleg Nesterov <oleg@redhat.com> - 2016-06-27 19:00 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Oleg Nesterov <oleg@redhat.com> - 2016-06-28 21:00 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Andy Lutomirski <luto@amacapital.net> - 2016-06-28 21:20 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Oleg Nesterov <oleg@redhat.com> - 2016-06-28 22:20 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Andy Lutomirski <luto@amacapital.net> - 2016-06-28 23:00 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-28 23:20 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-28 23:20 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Andy Lutomirski <luto@amacapital.net> - 2016-06-28 23:30 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-28 23:40 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-28 23:50 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Oleg Nesterov <oleg@redhat.com> - 2016-06-29 01:00 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Oleg Nesterov <oleg@redhat.com> - 2016-06-29 01:10 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Andy Lutomirski <luto@amacapital.net> - 2016-06-29 17:40 +0200
[PATCH] kthread: to_live_kthread() needs try_get_task_stack() Oleg Nesterov <oleg@redhat.com> - 2016-06-29 20:10 +0200
Re: [PATCH] kthread: to_live_kthread() needs try_get_task_stack() kbuild test robot <lkp@intel.com> - 2016-06-29 20:40 +0200
Re: [PATCH] kthread: to_live_kthread() needs try_get_task_stack() Oleg Nesterov <oleg@redhat.com> - 2016-06-29 20:50 +0200
Re: [PATCH] kthread: to_live_kthread() needs try_get_task_stack() kbuild test robot <lkp@intel.com> - 2016-06-29 21:00 +0200
Re: [PATCH] kthread: to_live_kthread() needs try_get_task_stack() Andy Lutomirski <luto@amacapital.net> - 2016-06-30 01:10 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Andy Lutomirski <luto@amacapital.net> - 2016-06-30 01:40 +0200
Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-27 19:20 +0200
Page 1 of 2 [1] 2 Next page →
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2016-06-27 07:30 +0200 |
| Subject | kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rOxhn-4pl-5@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-06-27 10:30 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rOA5z-69P-1@gated-at.bofh.it> |
| In reply to | #1431724 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2016-06-27 17:00 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rOGb0-1Du-25@gated-at.bofh.it> |
| In reply to | #1431724 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-06-27 17:50 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rOGXn-2br-11@gated-at.bofh.it> |
| In reply to | #1432102 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2016-06-27 19:00 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rOI37-2Qk-17@gated-at.bofh.it> |
| In reply to | #1432132 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2016-06-28 21:00 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rP6oO-2h2-17@gated-at.bofh.it> |
| In reply to | #1432158 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-06-28 21:20 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rP6I9-2Di-7@gated-at.bofh.it> |
| In reply to | #1433213 |
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
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2016-06-28 22:20 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rP7Ee-3d8-45@gated-at.bofh.it> |
| In reply to | #1433227 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-06-28 23:00 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rP8gW-3r9-31@gated-at.bofh.it> |
| In reply to | #1433274 |
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
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-06-28 23:20 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rP8Ah-3MT-9@gated-at.bofh.it> |
| In reply to | #1433305 |
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
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-06-28 23:20 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rP8Ai-3MT-13@gated-at.bofh.it> |
| In reply to | #1433326 |
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
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-06-28 23:30 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rP8JY-3Qa-9@gated-at.bofh.it> |
| In reply to | #1433326 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-06-28 23:40 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rP8TD-3Tn-11@gated-at.bofh.it> |
| In reply to | #1433331 |
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
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-06-28 23:50 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rP93k-3WI-23@gated-at.bofh.it> |
| In reply to | #1433339 |
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
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2016-06-29 01:00 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rPa93-4yZ-1@gated-at.bofh.it> |
| In reply to | #1433341 |
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);
}
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2016-06-29 01:10 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rPaiJ-4Rk-9@gated-at.bofh.it> |
| In reply to | #1433305 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-06-29 17:40 +0200 |
| Subject | Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18) |
| Message-ID | <rPpKO-5V4-29@gated-at.bofh.it> |
| In reply to | #1433368 |
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
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2016-06-29 20:10 +0200 |
| Subject | [PATCH] kthread: to_live_kthread() needs try_get_task_stack() |
| Message-ID | <rPs5Z-7vv-55@gated-at.bofh.it> |
| In reply to | #1433861 |
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
[toc] | [prev] | [next] | [standalone]
| From | kbuild test robot <lkp@intel.com> |
|---|---|
| Date | 2016-06-29 20:40 +0200 |
| Subject | Re: [PATCH] kthread: to_live_kthread() needs try_get_task_stack() |
| Message-ID | <rPsz0-7FB-39@gated-at.bofh.it> |
| In reply to | #1433941 |
[Multipart message — attachments visible in raw view] — view raw
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
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2016-06-29 20:50 +0200 |
| Subject | Re: [PATCH] kthread: to_live_kthread() needs try_get_task_stack() |
| Message-ID | <rPsIG-7IY-7@gated-at.bofh.it> |
| In reply to | #1433967 |
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
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web