| Message ID | 37ac7589ff0ea147e8a21cda5eb84d3af1f6cd60.1466974736.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jun 26, 2016 at 02:55:32PM -0700, Andy Lutomirski wrote: > It's not going to work, because the scheduler will explode if we try > to schedule when running on an IST stack or similar. > > This will matter when we let kernel stack overflows (which are #DF) > call die(). > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > arch/x86/kernel/dumpstack.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c > index ef8017ca5ba9..352f022cfd5b 100644 > --- a/arch/x86/kernel/dumpstack.c > +++ b/arch/x86/kernel/dumpstack.c > @@ -245,6 +245,9 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr) > return; > if (in_interrupt()) > panic("Fatal exception in interrupt"); > + if (((current_stack_pointer() ^ (current_top_of_stack() - 1)) > + & ~(THREAD_SIZE - 1)) != 0) Ugh, that's hard to parse. You could remove the "!= 0" at least to shorten it a bit and have one less braces level. Or maybe even do something like that to make it a bit more readable: if ((current_stack_pointer() ^ (current_top_of_stack() - 1)) & ~(THREAD_SIZE - 1)) panic("Fatal exception on non-default stack"); Meh. > + panic("Fatal exception on special stack"); "Fatal exception on non-default stack" maybe?
On Sat, Jul 02, 2016 at 07:24:41PM +0200, Borislav Petkov wrote: > On Sun, Jun 26, 2016 at 02:55:32PM -0700, Andy Lutomirski wrote: > > It's not going to work, because the scheduler will explode if we try > > to schedule when running on an IST stack or similar. > > > > This will matter when we let kernel stack overflows (which are #DF) > > call die(). > > > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > > --- > > arch/x86/kernel/dumpstack.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c > > index ef8017ca5ba9..352f022cfd5b 100644 > > --- a/arch/x86/kernel/dumpstack.c > > +++ b/arch/x86/kernel/dumpstack.c > > @@ -245,6 +245,9 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr) > > return; > > if (in_interrupt()) > > panic("Fatal exception in interrupt"); > > + if (((current_stack_pointer() ^ (current_top_of_stack() - 1)) > > + & ~(THREAD_SIZE - 1)) != 0) > > Ugh, that's hard to parse. You could remove the "!= 0" at least to > shorten it a bit and have one less braces level. > > Or maybe even do something like that to make it a bit more readable: > > if ((current_stack_pointer() ^ (current_top_of_stack() - 1)) > & > ~(THREAD_SIZE - 1)) > panic("Fatal exception on non-default stack"); > > Meh. A helper function would be even better. The existing 'object_is_on_stack()' can probably be used: if (!object_is_on_stack(current_top_of_stack())) panic("..."); Though that function isn't quite accurately named. It should really have 'task_stack' in its name, like 'object_is_on_task_stack()'. Or even better, something more concise like 'on_task_stack()'.
On Sat, Jul 02, 2016 at 01:34:51PM -0500, Josh Poimboeuf wrote: > The existing 'object_is_on_stack()' can probably be used: > > if (!object_is_on_stack(current_top_of_stack())) > panic("..."); > > Though that function isn't quite accurately named. It should really > have 'task_stack' in its name, like 'object_is_on_task_stack()'. Or > even better, something more concise like 'on_task_stack()'. So I'm obviously missing something here: object_is_on_stack() uses task_stack_page(current) -> task_struct.stack while current_stack_pointer() reads %rsp directly. I'm guessing %rsp and task_struct.stack are in sync?
On Sat, Jul 2, 2016 at 11:34 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Sat, Jul 02, 2016 at 07:24:41PM +0200, Borislav Petkov wrote: >> On Sun, Jun 26, 2016 at 02:55:32PM -0700, Andy Lutomirski wrote: >> > It's not going to work, because the scheduler will explode if we try >> > to schedule when running on an IST stack or similar. >> > >> > This will matter when we let kernel stack overflows (which are #DF) >> > call die(). >> > >> > Signed-off-by: Andy Lutomirski <luto@kernel.org> >> > --- >> > arch/x86/kernel/dumpstack.c | 3 +++ >> > 1 file changed, 3 insertions(+) >> > >> > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c >> > index ef8017ca5ba9..352f022cfd5b 100644 >> > --- a/arch/x86/kernel/dumpstack.c >> > +++ b/arch/x86/kernel/dumpstack.c >> > @@ -245,6 +245,9 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr) >> > return; >> > if (in_interrupt()) >> > panic("Fatal exception in interrupt"); >> > + if (((current_stack_pointer() ^ (current_top_of_stack() - 1)) >> > + & ~(THREAD_SIZE - 1)) != 0) >> >> Ugh, that's hard to parse. You could remove the "!= 0" at least to >> shorten it a bit and have one less braces level. >> >> Or maybe even do something like that to make it a bit more readable: >> >> if ((current_stack_pointer() ^ (current_top_of_stack() - 1)) >> & >> ~(THREAD_SIZE - 1)) >> panic("Fatal exception on non-default stack"); >> >> Meh. > > A helper function would be even better. > > The existing 'object_is_on_stack()' can probably be used: > > if (!object_is_on_stack(current_top_of_stack())) > panic("..."); > > Though that function isn't quite accurately named. It should really > have 'task_stack' in its name, like 'object_is_on_task_stack()'. Or > even better, something more concise like 'on_task_stack()'. > Given that the very next patch deletes this code, I vote for leaving it alone. Or I could fold the patches together. --Andy
On Sun, Jul 03, 2016 at 07:25:05AM -0700, Andy Lutomirski wrote: > Given that the very next patch deletes this code, I vote for leaving > it alone. Or I could fold the patches together. Ah, true. Yes, please fold them together.
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c index ef8017ca5ba9..352f022cfd5b 100644 --- a/arch/x86/kernel/dumpstack.c +++ b/arch/x86/kernel/dumpstack.c @@ -245,6 +245,9 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr) return; if (in_interrupt()) panic("Fatal exception in interrupt"); + if (((current_stack_pointer() ^ (current_top_of_stack() - 1)) + & ~(THREAD_SIZE - 1)) != 0) + panic("Fatal exception on special stack"); if (panic_on_oops) panic("Fatal exception"); do_exit(signr);
It's not going to work, because the scheduler will explode if we try to schedule when running on an IST stack or similar. This will matter when we let kernel stack overflows (which are #DF) call die(). Signed-off-by: Andy Lutomirski <luto@kernel.org> --- arch/x86/kernel/dumpstack.c | 3 +++ 1 file changed, 3 insertions(+)