Message-ID: <CAMzpN2jDsaj4-pUQhYoMxpo4Vr66fcEf03BquGzVS3U-KQbyAQ@mail.gmail.com>
Date:	Mon, 13 Jul 2015 15:46:45 -0400
From:	Brian Gerst <brgerst@...il.com>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	X86 ML <x86@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 1/5] x86/vm86: Move userspace accesses to do_sys_vm86()

On Mon, Jul 13, 2015 at 2:36 PM, Andy Lutomirski <luto@...capital.net> wrote:
> On Mon, Jul 13, 2015 at 9:45 AM, Brian Gerst <brgerst@...il.com> wrote:
>> On Sat, Jul 11, 2015 at 12:37 PM, Andy Lutomirski <luto@...capital.net> wrote:
>>> On Fri, Jul 10, 2015 at 10:09 PM, Brian Gerst <brgerst@...il.com> wrote:
>>>> Move the userspace accesses down into the common function in
>>>> preparation for the next set of patches.
>>>>
>>>
>>> One thing I don't like about the current code that makes these patches
>>> harder to review is the bizarre approach to copying.  If you changed
>>> this:
>>>
>>>> -       tmp = copy_vm86_regs_from_user(&info.regs, &v86->regs,
>>>> -                                      offsetof(struct kernel_vm86_struct, vm86plus) -
>>>> -                                      sizeof(info.regs));
>>>
>>> into a normal field-by-field get_user / copy_from_user (the latter for
>>> the big regs struct) then it would be clear what the ABI is and it
>>> would be much easier to read the patches and confirm that you aren't
>>> accidentally changing the ABI.
>>>
>>> You could also get rid of the constraint that certain fields in
>>> apparently kernel-internal structs had to be in a certain order.
>>>
>>> Other than that, patches 1-4 look good on cursory inspection.  I'll
>>> look more carefully later.  I need to think about patch 5 more.
>>>
>>> --Andy
>>
>> Any other comments before I start working on v2?
>>
>
> Nothing major.  I'm a bit nervous about leaving ds, es, fs, and gs in
> pt_regs more or less undefined until save_v86_state happens, but it's
> unlikely that there's any ABI to break there.

They are not undefined.  The CPU sets them to NULL on exit from vm86
mode, after pushing them before the normal exception frame.  The entry
code then pushes these NULL values into the normal segment slots in
pt_regs.  The NULL values would be visible to ptrace (which should be
taught to look at the alternate slots), but I don't see any other
problems.

> The results from perf
> might be a bit odd with your patches applied.  Of course, they're
> probably useless without your patch.
>
> It might also be worth renaming save_v86_state in patch 5.
>
> Do your patches pass my upgraded entry_from_vm86 test?  You're
> changing handle_vm86_trap so it always returns, which may have
> unexpected side effects (or I missed something in your patch).

Yes, it passes all the tests.  Changing the trap/fault handlers to
always return (instead of switching the stack pointer and jumping to
the exit asm) is the whole point of this patch set.  save_v86_state()
now just swaps the register state in place and sets the syscall return
value.  This allows returning from the exception handler in the normal
manner (skipping the signal if the exception was handled).  An
unhandled exception like #UD will raise a signal, which then is
handled by the change in handle_signal(), as well as external signals.

--
Brian Gerst
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists