Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752422AbbGMTqr (ORCPT ); Mon, 13 Jul 2015 15:46:47 -0400 Received: from mail-oi0-f53.google.com ([209.85.218.53]:33550 "EHLO mail-oi0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751117AbbGMTqq (ORCPT ); Mon, 13 Jul 2015 15:46:46 -0400 MIME-Version: 1.0 In-Reply-To: References: <1436591360-16210-1-git-send-email-brgerst@gmail.com> <1436591360-16210-2-git-send-email-brgerst@gmail.com> Date: Mon, 13 Jul 2015 15:46:45 -0400 Message-ID: Subject: Re: [PATCH 1/5] x86/vm86: Move userspace accesses to do_sys_vm86() From: Brian Gerst To: Andy Lutomirski Cc: X86 ML , "linux-kernel@vger.kernel.org" , Ingo Molnar , "H. Peter Anvin" , Denys Vlasenko , Linus Torvalds Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3183 Lines: 69 On Mon, Jul 13, 2015 at 2:36 PM, Andy Lutomirski wrote: > On Mon, Jul 13, 2015 at 9:45 AM, Brian Gerst wrote: >> On Sat, Jul 11, 2015 at 12:37 PM, Andy Lutomirski wrote: >>> On Fri, Jul 10, 2015 at 10:09 PM, Brian Gerst 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/