On Wed, Sep 21, 2016 at 8:43 AM, Roman Pen
<[email protected]> wrote:
> kthread uses stack and keeps completion structure on it to be woken up
> on vfork_done completion.
>
> In commit 2deb4be28 Andy Lutomirski rewinds the stack unconditionally
> and further completion of task->vfork_done for any kthread leads to stack
> corruption (or infinite spin on attempt to spin lock on garbage memory).
This is sort of okay, but it will blow up pretty badly if a kthread
overflows its stack. Would it make more sense to change
rewind_stack_do_exit() to leave a big enough gap at the top of the
stack to avoid clobbering the completion?
--Andy
On Thu, Oct 20, 2016 at 04:07:28PM -0700, Andy Lutomirski wrote:
> On Wed, Sep 21, 2016 at 8:43 AM, Roman Pen
> <[email protected]> wrote:
> > kthread uses stack and keeps completion structure on it to be woken up
> > on vfork_done completion.
> >
> > In commit 2deb4be28 Andy Lutomirski rewinds the stack unconditionally
> > and further completion of task->vfork_done for any kthread leads to stack
> > corruption (or infinite spin on attempt to spin lock on garbage memory).
>
> This is sort of okay, but it will blow up pretty badly if a kthread
> overflows its stack. Would it make more sense to change
> rewind_stack_do_exit() to leave a big enough gap at the top of the
> stack to avoid clobbering the completion?
We need to preserve the entire struct kthread on the stack, kthread just
abuses that pointer to stash an on-stack kthread descriptor. See
kthread():
current->vfork_done = &self.exited;
Its a horrible horrible thing kthread does. I suppose there might have
been some intent by keeping that exited completion last in the
structure, but *shudder*.
But yes, leaving enough stack to not clobber that might keep this horror
show working.
ISTR talk about alternative schemes for this a long time ago, but I
cannot recall :-(
On Fri, 21 Oct 2016, Peter Zijlstra wrote:
> We need to preserve the entire struct kthread on the stack, kthread just
> abuses that pointer to stash an on-stack kthread descriptor. See
> kthread():
>
> current->vfork_done = &self.exited;
>
> Its a horrible horrible thing kthread does. I suppose there might have
> been some intent by keeping that exited completion last in the
> structure, but *shudder*.
>
> But yes, leaving enough stack to not clobber that might keep this horror
> show working.
>
> ISTR talk about alternative schemes for this a long time ago, but I
> cannot recall :-(
The simplest solution would be to stick struct kthread into task_struct,
but that's bloat.
But we can allocate it seperately along with kthread_create_info. That's
pretty straight forward.
Thanks,
tglx