[GIT PULL] x86: unify sys_iopl

[GIT PULL] x86: unify sys_iopl

[Jeremy Fitzhardinge]

Hi Ingo,

The x86-64 implementation of iopl was buggy because it never ended up calling
set_iopl_mask().  This had no effect on native sys_iopl (because set_iopl_mask
is normally no-op on 64-bit), but it ended up never calling the pvop, which caused
iopl to have no effect on 64-bit Xen guests.

The two functions are needlessly different anyway.  This patch just unifies them
into a single function which is mostly derived from the 32-bit version.

Thanks,
	J

The following changes since commit c44c9ec0f38b939b3200436e3aa95c1aa83c41c7:
  Jeremy Fitzhardinge (1):
        x86: split NX setup into separate file to limit unstack-protected code

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git bugfix

Jeremy Fitzhardinge (1):
      x86: unify sys_iopl

 arch/x86/include/asm/syscalls.h |    8 +++-----
 arch/x86/kernel/ioport.c        |   11 ++---------
 2 files changed, 5 insertions(+), 14 deletions(-)

>From 8dbb1d168eb8be6bf43d123fdfb3ba3b03762e62 Mon Sep 17 00:00:00 2001
From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Date: Wed, 23 Sep 2009 16:35:24 -0700
Subject: [PATCH] x86: unify sys_iopl

The 32 and 64-bit versions of sys_iopl were needlessly different:
 - The 32-bit version ignored its function argument and directly
   fetched the parameter from struct regs, presumably code dating
   from the neolithic era.
 - The 64-bit version never called set_iopl_mask().  Usually this
   is a no-op for 64-bit, but it is also a pvop, which meant that
   that iopl never worked for 64-bit guests under Xen.
 - Both versions use the archaic style of getting pt_regs passed
   to them.  We can get the current pt_regs with task_pt_regs, which
   avoids worrying about the fine details of stack layout.

We can use the body of the 32-bit function with a few small
adjustments to the function definition.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

diff --git a/arch/x86/include/asm/syscalls.h b/arch/x86/include/asm/syscalls.h
index 372b76e..5336ce2 100644
--- a/arch/x86/include/asm/syscalls.h
+++ b/arch/x86/include/asm/syscalls.h
@@ -33,11 +33,11 @@ long sys_rt_sigreturn(struct pt_regs *);
 asmlinkage int sys_set_thread_area(struct user_desc __user *);
 asmlinkage int sys_get_thread_area(struct user_desc __user *);
 
-/* X86_32 only */
-#ifdef CONFIG_X86_32
 /* kernel/ioport.c */
-long sys_iopl(struct pt_regs *);
+asmlinkage long sys_iopl(unsigned int);
 
+/* X86_32 only */
+#ifdef CONFIG_X86_32
 /* kernel/process_32.c */
 int sys_clone(struct pt_regs *);
 int sys_execve(struct pt_regs *);
@@ -70,8 +70,6 @@ int sys_vm86(struct pt_regs *);
 #else /* CONFIG_X86_32 */
 
 /* X86_64 only */
-/* kernel/ioport.c */
-asmlinkage long sys_iopl(unsigned int, struct pt_regs *);
 
 /* kernel/process_64.c */
 asmlinkage long sys_clone(unsigned long, unsigned long,
diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
index 99c4d30..bad4f22 100644
--- a/arch/x86/kernel/ioport.c
+++ b/arch/x86/kernel/ioport.c
@@ -119,11 +119,10 @@ static int do_iopl(unsigned int level, struct pt_regs *regs)
 	return 0;
 }
 
-#ifdef CONFIG_X86_32
-long sys_iopl(struct pt_regs *regs)
+asmlinkage long sys_iopl(unsigned int level)
 {
-	unsigned int level = regs->bx;
 	struct thread_struct *t = &current->thread;
+	struct pt_regs *regs = task_pt_regs(current);
 	int rc;
 
 	rc = do_iopl(level, regs);
@@ -135,9 +134,3 @@ long sys_iopl(struct pt_regs *regs)
 out:
 	return rc;
 }
-#else
-asmlinkage long sys_iopl(unsigned int level, struct pt_regs *regs)
-{
-	return do_iopl(level, regs);
-}
-#endif

Re: [GIT PULL] x86: unify sys_iopl

[Brian Gerst]

The correct fix is to move set_iopl_mask into do_iopl.  sys_iopl
should only be a wrapper around do_iopl to manage the different
calling convention used by 32-bit and 64-bit.

On Fri, Sep 25, 2009 at 6:05 PM, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> Hi Ingo,
>
> The x86-64 implementation of iopl was buggy because it never ended up calling
> set_iopl_mask().  This had no effect on native sys_iopl (because set_iopl_mask
> is normally no-op on 64-bit), but it ended up never calling the pvop, which caused
> iopl to have no effect on 64-bit Xen guests.
>
> The two functions are needlessly different anyway.  This patch just unifies them
> into a single function which is mostly derived from the 32-bit version.
>
> Thanks,
>        J
>
> The following changes since commit c44c9ec0f38b939b3200436e3aa95c1aa83c41c7:
>  Jeremy Fitzhardinge (1):
>        x86: split NX setup into separate file to limit unstack-protected code
>
> are available in the git repository at:
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git bugfix
>
> Jeremy Fitzhardinge (1):
>      x86: unify sys_iopl
>
>  arch/x86/include/asm/syscalls.h |    8 +++-----
>  arch/x86/kernel/ioport.c        |   11 ++---------
>  2 files changed, 5 insertions(+), 14 deletions(-)
>
> From 8dbb1d168eb8be6bf43d123fdfb3ba3b03762e62 Mon Sep 17 00:00:00 2001
> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> Date: Wed, 23 Sep 2009 16:35:24 -0700
> Subject: [PATCH] x86: unify sys_iopl
>
> The 32 and 64-bit versions of sys_iopl were needlessly different:
>  - The 32-bit version ignored its function argument and directly
>   fetched the parameter from struct regs, presumably code dating
>   from the neolithic era.
>  - The 64-bit version never called set_iopl_mask().  Usually this
>   is a no-op for 64-bit, but it is also a pvop, which meant that
>   that iopl never worked for 64-bit guests under Xen.
>  - Both versions use the archaic style of getting pt_regs passed
>   to them.  We can get the current pt_regs with task_pt_regs, which
>   avoids worrying about the fine details of stack layout.
>
> We can use the body of the 32-bit function with a few small
> adjustments to the function definition.
>
> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>
> diff --git a/arch/x86/include/asm/syscalls.h b/arch/x86/include/asm/syscalls.h
> index 372b76e..5336ce2 100644
> --- a/arch/x86/include/asm/syscalls.h
> +++ b/arch/x86/include/asm/syscalls.h
> @@ -33,11 +33,11 @@ long sys_rt_sigreturn(struct pt_regs *);
>  asmlinkage int sys_set_thread_area(struct user_desc __user *);
>  asmlinkage int sys_get_thread_area(struct user_desc __user *);
>
> -/* X86_32 only */
> -#ifdef CONFIG_X86_32
>  /* kernel/ioport.c */
> -long sys_iopl(struct pt_regs *);
> +asmlinkage long sys_iopl(unsigned int);
>
> +/* X86_32 only */
> +#ifdef CONFIG_X86_32
>  /* kernel/process_32.c */
>  int sys_clone(struct pt_regs *);
>  int sys_execve(struct pt_regs *);
> @@ -70,8 +70,6 @@ int sys_vm86(struct pt_regs *);
>  #else /* CONFIG_X86_32 */
>
>  /* X86_64 only */
> -/* kernel/ioport.c */
> -asmlinkage long sys_iopl(unsigned int, struct pt_regs *);
>
>  /* kernel/process_64.c */
>  asmlinkage long sys_clone(unsigned long, unsigned long,
> diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
> index 99c4d30..bad4f22 100644
> --- a/arch/x86/kernel/ioport.c
> +++ b/arch/x86/kernel/ioport.c
> @@ -119,11 +119,10 @@ static int do_iopl(unsigned int level, struct pt_regs *regs)
>        return 0;
>  }
>
> -#ifdef CONFIG_X86_32
> -long sys_iopl(struct pt_regs *regs)
> +asmlinkage long sys_iopl(unsigned int level)
>  {
> -       unsigned int level = regs->bx;
>        struct thread_struct *t = &current->thread;
> +       struct pt_regs *regs = task_pt_regs(current);
>        int rc;
>
>        rc = do_iopl(level, regs);
> @@ -135,9 +134,3 @@ long sys_iopl(struct pt_regs *regs)
>  out:
>        return rc;
>  }
> -#else
> -asmlinkage long sys_iopl(unsigned int level, struct pt_regs *regs)
> -{
> -       return do_iopl(level, regs);
> -}
> -#endif
>
>
> --
> 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/
>

Re: [GIT PULL] x86: unify sys_iopl

[Jeremy Fitzhardinge]

On 09/25/09 15:56, Brian Gerst wrote:
> The correct fix is to move set_iopl_mask into do_iopl.  sys_iopl
> should only be a wrapper around do_iopl to manage the different
> calling convention used by 32-bit and 64-bit.
>   

The patch eliminates that distinction, so I guess do_iopl should really
be folded into sys_iopl (though the compiler effectively does that
anyway by inlining it).

    J

Re: [GIT PULL] x86: unify sys_iopl

[Brian Gerst]

On Fri, Sep 25, 2009 at 7:22 PM, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> On 09/25/09 15:56, Brian Gerst wrote:
>> The correct fix is to move set_iopl_mask into do_iopl.  sys_iopl
>> should only be a wrapper around do_iopl to manage the different
>> calling convention used by 32-bit and 64-bit.
>>
>
> The patch eliminates that distinction, so I guess do_iopl should really
> be folded into sys_iopl (though the compiler effectively does that
> anyway by inlining it).

Using task_pt_regs is less effecient than getting the pointer directly
from the syscall entry code.  On 32-bit it probably doesn't matter too
much, but on 64-bit, you still need the special stub code in
entry_64.S to put the full pt_regs struct on the stack.  Changing the
calling conventions should be a seperate patch, and should be done for
all pt_regs-using syscalls.

Re: [GIT PULL] x86: unify sys_iopl

[Jeremy Fitzhardinge]

On 09/25/09 16:54, Brian Gerst wrote:
> Using task_pt_regs is less effecient than getting the pointer directly
> from the syscall entry code.

(really not an issue in this case)

>   On 32-bit it probably doesn't matter too
> much, but on 64-bit, you still need the special stub code in
> entry_64.S to put the full pt_regs struct on the stack.  Changing the
> calling conventions should be a seperate patch, and should be done for
> all pt_regs-using syscalls.
>   

iopl only really cares about [er]flags, so it probably doesn't need a
full ptregs structure anyway.  But I'm not sure what changes you'd
consider for the rest of the ptregs calls; sigaltstack probably doesn't
need full ptregs either (it just updates [er]sp I think), but fork/clone
definitely do.

    J

Re: [GIT PULL] x86: unify sys_iopl

[Ingo Molnar]

* Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Hi Ingo,
> 
> The x86-64 implementation of iopl was buggy because it never ended up 
> calling set_iopl_mask().  This had no effect on native sys_iopl 
> (because set_iopl_mask is normally no-op on 64-bit), but it ended up 
> never calling the pvop, which caused iopl to have no effect on 64-bit 
> Xen guests.
> 
> The two functions are needlessly different anyway.  This patch just 
> unifies them into a single function which is mostly derived from the 
> 32-bit version.
> 
> Thanks,
> 	J
> 
> The following changes since commit c44c9ec0f38b939b3200436e3aa95c1aa83c41c7:
>   Jeremy Fitzhardinge (1):
>         x86: split NX setup into separate file to limit unstack-protected code
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git bugfix
> 
> Jeremy Fitzhardinge (1):
>       x86: unify sys_iopl
> 
>  arch/x86/include/asm/syscalls.h |    8 +++-----
>  arch/x86/kernel/ioport.c        |   11 ++---------
>  2 files changed, 5 insertions(+), 14 deletions(-)

Pulled, thanks a lot Jeremy!

It's in tip:x86/paravirt right now. I'm uneasy about pushing this into 
.32 - we had this status quo forever. Peter, Thomas, what do you think?

	Ingo

Re: [GIT PULL] x86: unify sys_iopl

[H. Peter Anvin]

On 10/13/2009 03:30 AM, Ingo Molnar wrote:
> 
> Pulled, thanks a lot Jeremy!
> 
> It's in tip:x86/paravirt right now. I'm uneasy about pushing this into 
> .32 - we had this status quo forever. Peter, Thomas, what do you think?
> 

First of all, the unification looks good.

As far as .32 is concerned... this *is* a bug even if this is only for
paravirt, and given the small amount of code I am personally OK with
taking the whole patch for .32.

However, the patch is not complete!  The patch incidentally eliminates
the need to have assembly stubs for sys_iopl, and those assembly stubs
should be removed.  I have a patch for that currently test building.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [GIT PULL] x86: unify sys_iopl

[Jeremy Fitzhardinge]

On 10/13/09 09:24, H. Peter Anvin wrote:
> First of all, the unification looks good.
>
> As far as .32 is concerned... this *is* a bug even if this is only for
> paravirt, and given the small amount of code I am personally OK with
> taking the whole patch for .32.
>
> However, the patch is not complete!  The patch incidentally eliminates
> the need to have assembly stubs for sys_iopl, and those assembly stubs
> should be removed.  I have a patch for that currently test building.
>   

I wasn't sure whether task_pt_regs() needed the full register set to be
saved to correctly return rflags (that is, does PTREGSCALL change the
shape of the stack, or just the contents?).

    J

Re: [GIT PULL] x86: unify sys_iopl

[H. Peter Anvin]

On 10/13/2009 09:53 AM, Jeremy Fitzhardinge wrote:
> On 10/13/09 09:24, H. Peter Anvin wrote:
>> First of all, the unification looks good.
>>
>> As far as .32 is concerned... this *is* a bug even if this is only for
>> paravirt, and given the small amount of code I am personally OK with
>> taking the whole patch for .32.
>>
>> However, the patch is not complete!  The patch incidentally eliminates
>> the need to have assembly stubs for sys_iopl, and those assembly stubs
>> should be removed.  I have a patch for that currently test building.
>>   
> 
> I wasn't sure whether task_pt_regs() needed the full register set to be
> saved to correctly return rflags (that is, does PTREGSCALL change the
> shape of the stack, or just the contents?).
> 

PTREGSCALL does change the shape of the stack on 64 bits, but not on 32
bits.  However, I believe it only matters for the additional fields,
since the structure is anchored at the top.  I will double-check.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [GIT PULL] x86: unify sys_iopl

[H. Peter Anvin]

On 10/13/2009 09:53 AM, Jeremy Fitzhardinge wrote:
> On 10/13/09 09:24, H. Peter Anvin wrote:
>> First of all, the unification looks good.
>>
>> As far as .32 is concerned... this *is* a bug even if this is only for
>> paravirt, and given the small amount of code I am personally OK with
>> taking the whole patch for .32.
>>
>> However, the patch is not complete!  The patch incidentally eliminates
>> the need to have assembly stubs for sys_iopl, and those assembly stubs
>> should be removed.  I have a patch for that currently test building.
>>   
> 
> I wasn't sure whether task_pt_regs() needed the full register set to be
> saved to correctly return rflags (that is, does PTREGSCALL change the
> shape of the stack, or just the contents?).
> 

A double-check confirmed my previous statement: on 64 bits, PTREGSCALL
does populate the fields at the beginning of struct pt_regs, but
task_pt_regs(current) is valid even without those fields populated,
since it is anchored at the top "hanging down" from the kernel entry
stack pointer address.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [GIT PULL] x86: unify sys_iopl

[H. Peter Anvin]

On 10/13/2009 09:53 AM, Jeremy Fitzhardinge wrote:
> On 10/13/09 09:24, H. Peter Anvin wrote:
>> First of all, the unification looks good.
>>
>> As far as .32 is concerned... this *is* a bug even if this is only for
>> paravirt, and given the small amount of code I am personally OK with
>> taking the whole patch for .32.
>>
>> However, the patch is not complete!  The patch incidentally eliminates
>> the need to have assembly stubs for sys_iopl, and those assembly stubs
>> should be removed.  I have a patch for that currently test building.
>>   
> 
> I wasn't sure whether task_pt_regs() needed the full register set to be
> saved to correctly return rflags (that is, does PTREGSCALL change the
> shape of the stack, or just the contents?).
> 

Erk, I was right but still wrong... it isn't safe to modify the partial
pt_regs because of the magic sync stuff that goes on in entry_64.S.

I have to say the entry_64.S stuff gives me a headache in the extreme.
It really can't be the sanest way to do this stuff.

/me makes a mental note to try to work through this code at some point.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.