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 <[email protected]>
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 <[email protected]>
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 = ¤t->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
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 <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
>
> 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 = ¤t->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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
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
On Fri, Sep 25, 2009 at 7:22 PM, Jeremy Fitzhardinge <[email protected]> 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.
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
* Jeremy Fitzhardinge <[email protected]> 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
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.
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
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.
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.
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.