Mailing List Archive: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

[PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

Mar 10, 2015, 7:03 AM

Post #1 of 7 (351 views)

As far as I can tell, these fields have been set to zero on save and
ignored on restore since Linux was imported into git. Rename them
'__pad1' and '__pad2' to avoid confusion and to allow them to be
recycled some day.

I'm intentionally avoiding calling either of them __pad0: the field
formerly known as __pad0 is now ss.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/include/asm/sigcontext.h | 4 ++--
arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
arch/x86/kernel/signal.c | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
index f910cdcb71fd..5f0ef11719e1 100644
--- a/arch/x86/include/asm/sigcontext.h
+++ b/arch/x86/include/asm/sigcontext.h
@@ -57,8 +57,8 @@ struct sigcontext {
unsigned long ip;
unsigned long flags;
unsigned short cs;
- unsigned short gs;
- unsigned short fs;
+ unsigned short __pad2; /* Was called gs, but was always zero. */
+ unsigned short __pad1; /* Was called gs, but was always zero. */
unsigned short ss;
unsigned long err;
unsigned long trapno;
diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
index 076b11fd6fa1..df9908b1aa95 100644
--- a/arch/x86/include/uapi/asm/sigcontext.h
+++ b/arch/x86/include/uapi/asm/sigcontext.h
@@ -177,8 +177,8 @@ struct sigcontext {
__u64 rip;
__u64 eflags; /* RFLAGS */
__u16 cs;
- __u16 gs;
- __u16 fs;
+ __u16 __pad2; /* Was called gs, but was always zero. */
+ __u16 __pad1; /* Was called fs, but was always zero. */
__u16 ss;
__u64 err;
__u64 trapno;
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 40f34574fb36..01a53767823c 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -155,8 +155,8 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
#else /* !CONFIG_X86_32 */
put_user_ex(regs->flags, &sc->flags);
put_user_ex(regs->cs, &sc->cs);
- put_user_ex(0, &sc->gs);
- put_user_ex(0, &sc->fs);
+ put_user_ex(0, &sc->__pad2);
+ put_user_ex(0, &sc->__pad1);
put_user_ex(regs->ss, &sc->ss);
#endif /* CONFIG_X86_32 */

--
2.3.0

--
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: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext [ In reply to ]

luto at amacapital

Mar 10, 2015, 7:22 AM

Post #2 of 7 (347 views)

Permalink

[cc: Oleg, Borislav]

On Tue, Mar 10, 2015 at 7:03 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> As far as I can tell, these fields have been set to zero on save and
> ignored on restore since Linux was imported into git. Rename them
> '__pad1' and '__pad2' to avoid confusion and to allow them to be
> recycled some day.
>
> I'm intentionally avoiding calling either of them __pad0: the field
> formerly known as __pad0 is now ss.
>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
> arch/x86/include/asm/sigcontext.h | 4 ++--
> arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
> arch/x86/kernel/signal.c | 4 ++--
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
> index f910cdcb71fd..5f0ef11719e1 100644
> --- a/arch/x86/include/asm/sigcontext.h
> +++ b/arch/x86/include/asm/sigcontext.h
> @@ -57,8 +57,8 @@ struct sigcontext {
> unsigned long ip;
> unsigned long flags;
> unsigned short cs;
> - unsigned short gs;
> - unsigned short fs;
> + unsigned short __pad2; /* Was called gs, but was always zero. */
> + unsigned short __pad1; /* Was called gs, but was always zero. */
> unsigned short ss;
> unsigned long err;
> unsigned long trapno;
> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
> index 076b11fd6fa1..df9908b1aa95 100644
> --- a/arch/x86/include/uapi/asm/sigcontext.h
> +++ b/arch/x86/include/uapi/asm/sigcontext.h
> @@ -177,8 +177,8 @@ struct sigcontext {
> __u64 rip;
> __u64 eflags; /* RFLAGS */
> __u16 cs;
> - __u16 gs;
> - __u16 fs;
> + __u16 __pad2; /* Was called gs, but was always zero. */
> + __u16 __pad1; /* Was called fs, but was always zero. */
> __u16 ss;
> __u64 err;
> __u64 trapno;
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index 40f34574fb36..01a53767823c 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -155,8 +155,8 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
> #else /* !CONFIG_X86_32 */
> put_user_ex(regs->flags, &sc->flags);
> put_user_ex(regs->cs, &sc->cs);
> - put_user_ex(0, &sc->gs);
> - put_user_ex(0, &sc->fs);
> + put_user_ex(0, &sc->__pad2);
> + put_user_ex(0, &sc->__pad1);
> put_user_ex(regs->ss, &sc->ss);
> #endif /* CONFIG_X86_32 */
>
> --
> 2.3.0
>

--
Andy Lutomirski
AMA Capital Management, LLC
--
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: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext [ In reply to ]

john at stoffel

Mar 10, 2015, 9:16 AM

Post #3 of 7 (348 views)

Permalink

>>>>> "Andy" == Andy Lutomirski <luto@amacapital.net> writes:

Andy> As far as I can tell, these fields have been set to zero on save and
Andy> ignored on restore since Linux was imported into git. Rename them
Andy> '__pad1' and '__pad2' to avoid confusion and to allow them to be
Andy> recycled some day.

Andy> I'm intentionally avoiding calling either of them __pad0: the field
Andy> formerly known as __pad0 is now ss.

Andy> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Andy> ---
Andy> arch/x86/include/asm/sigcontext.h | 4 ++--
Andy> arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
Andy> arch/x86/kernel/signal.c | 4 ++--
Andy> 3 files changed, 6 insertions(+), 6 deletions(-)

Andy> diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
Andy> index f910cdcb71fd..5f0ef11719e1 100644
Andy> --- a/arch/x86/include/asm/sigcontext.h
Andy> +++ b/arch/x86/include/asm/sigcontext.h
Andy> @@ -57,8 +57,8 @@ struct sigcontext {
Andy> unsigned long ip;
Andy> unsigned long flags;
Andy> unsigned short cs;
Andy> - unsigned short gs;
Andy> - unsigned short fs;
Andy> + unsigned short __pad2; /* Was called gs, but was always zero. */
Andy> + unsigned short __pad1; /* Was called gs, but was always zero. */

Shouldn't this comment read:

/* Was called fs, but was always zero. */

for the second one?
--
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: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext [ In reply to ]

luto at amacapital

Mar 10, 2015, 11:12 AM

Post #4 of 7 (345 views)

Permalink

On Tue, Mar 10, 2015 at 9:16 AM, John Stoffel <john@stoffel.org> wrote:
>>>>>> "Andy" == Andy Lutomirski <luto@amacapital.net> writes:
>
> Andy> As far as I can tell, these fields have been set to zero on save and
> Andy> ignored on restore since Linux was imported into git. Rename them
> Andy> '__pad1' and '__pad2' to avoid confusion and to allow them to be
> Andy> recycled some day.
>
> Andy> I'm intentionally avoiding calling either of them __pad0: the field
> Andy> formerly known as __pad0 is now ss.
>
> Andy> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> Andy> ---
> Andy> arch/x86/include/asm/sigcontext.h | 4 ++--
> Andy> arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
> Andy> arch/x86/kernel/signal.c | 4 ++--
> Andy> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> Andy> diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
> Andy> index f910cdcb71fd..5f0ef11719e1 100644
> Andy> --- a/arch/x86/include/asm/sigcontext.h
> Andy> +++ b/arch/x86/include/asm/sigcontext.h
> Andy> @@ -57,8 +57,8 @@ struct sigcontext {
> Andy> unsigned long ip;
> Andy> unsigned long flags;
> Andy> unsigned short cs;
> Andy> - unsigned short gs;
> Andy> - unsigned short fs;
> Andy> + unsigned short __pad2; /* Was called gs, but was always zero. */
> Andy> + unsigned short __pad1; /* Was called gs, but was always zero. */
>
> Shouldn't this comment read:
>
> /* Was called fs, but was always zero. */
>
> for the second one?

Will fix for v2. Thanks.

--
Andy Lutomirski
AMA Capital Management, LLC
--
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: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext [ In reply to ]

bp at alien8

Mar 11, 2015, 2:28 AM

Post #5 of 7 (342 views)

Permalink

On Tue, Mar 10, 2015 at 07:03:25AM -0700, Andy Lutomirski wrote:
> As far as I can tell, these fields have been set to zero on save and
> ignored on restore since Linux was imported into git. Rename them

Actually from the very beginning:

commit 47f16da277d10ef9494f3e9da2a9113bb22bcd75
Author: Andi Kleen <ak@muc.de>
Date: Tue Feb 12 20:17:35 2002 -0800

[PATCH] x86_64 merge: arch + asm

...

+static int
+setup_sigcontext(struct sigcontext *sc, struct _fpstate *fpstate,
+ struct pt_regs *regs, unsigned long mask)
+{
+ int tmp, err = 0;
+
+ tmp = 0;
+ __asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp));
+ err |= __put_user(tmp, (unsigned int *)&sc->gs);
+ __asm__("movl %%fs,%0" : "=r"(tmp): "0"(tmp));
+ err |= __put_user(tmp, (unsigned int *)&sc->fs);

> '__pad1' and '__pad2' to avoid confusion and to allow them to be
> recycled some day.
>
> I'm intentionally avoiding calling either of them __pad0: the field
> formerly known as __pad0 is now ss.
>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>

Acked-by: Borislav Petkov <bp@suse.de>

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext [ In reply to ]

luto at amacapital

Mar 11, 2015, 5:22 AM

Post #6 of 7 (330 views)

Permalink

On Mar 11, 2015 5:29 AM, "Borislav Petkov" <bp@alien8.de> wrote:
>
> On Tue, Mar 10, 2015 at 07:03:25AM -0700, Andy Lutomirski wrote:
> > As far as I can tell, these fields have been set to zero on save and
> > ignored on restore since Linux was imported into git. Rename them
>
> Actually from the very beginning:
>
> commit 47f16da277d10ef9494f3e9da2a9113bb22bcd75
> Author: Andi Kleen <ak@muc.de>
> Date: Tue Feb 12 20:17:35 2002 -0800
>
> [PATCH] x86_64 merge: arch + asm
>
> ...
>
> +static int
> +setup_sigcontext(struct sigcontext *sc, struct _fpstate *fpstate,
> + struct pt_regs *regs, unsigned long mask)
> +{
> + int tmp, err = 0;
> +
> + tmp = 0;
> + __asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp));
> + err |= __put_user(tmp, (unsigned int *)&sc->gs);
> + __asm__("movl %%fs,%0" : "=r"(tmp): "0"(tmp));
> + err |= __put_user(tmp, (unsigned int *)&sc->fs);
>

Wait... It looks like it really was saving them. Of course, this is
mostly useless unless the code also does something very clever with
fsbase and gsbase.

After wrestling with the historic git a bit:

commit a104ba57b3b3697fd77778918b6a77ed16bb2ff8
Author: Andi Kleen <ak@muc.de>
Date: Mon Mar 10 01:41:56 2003 -0800

[PATCH] x86-64 updates for 2.5.64-bk3

...

static inline int
setup_sigcontext(struct sigcontext *sc, struct pt_regs *regs,
unsigned long mask, struct task_struct *me)
{
- int tmp, err = 0;
+ int err = 0;

- tmp = 0;
- __asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp));
- err |= __put_user(tmp, (unsigned int *)&sc->gs);
- __asm__("movl %%fs,%0" : "=r"(tmp): "0"(tmp));
- err |= __put_user(tmp, (unsigned int *)&sc->fs);
+ err |= __put_user(0, &sc->gs);
+ err |= __put_user(0, &sc->fs);

Prior to that, we actually saved and restored the thing. In restore_context:

{
unsigned int seg;
err |= __get_user(seg, &sc->gs);
load_gs_index(seg);
err |= __get_user(seg, &sc->fs);
loadsegment(fs,seg);
}

We did not, however, save and restore fsbase and gsbase.

Interestingly, in the same BK change, we also remove set_thread_area
as a 64-bit syscall entirely (take that, ABI compatibility!), but
arch_prctl existed before that change.

IOW, in 2.5.63 and earlier, we tried to save and restore TLS state
across signals, but we did it wrong and would corrupt it for any
program that used arch_prctl. At that time, programs could switch
userspace threads using signals and their TLS pointers would switch.
In 2.5.64 and later, we broke set_thread_area users, fixed arch_prctl
users, and made signals effectively *not* switch TLS pointers.

It seems unlikely to me that many pre-2.5.64 programs still run due to
the aforementioned removal of a syscall, but I think that we should at
least document this in the header so that anyone who tries to recycle
those padding bits is appropriately careful.

--Andy
--
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: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext [ In reply to ]

bp at alien8

Mar 11, 2015, 5:32 AM

Post #7 of 7 (332 views)

Permalink

On Wed, Mar 11, 2015 at 05:22:48AM -0700, Andy Lutomirski wrote:
> Wait... It looks like it really was saving them. Of course, this is

Bah, of course. Blind me.

> IOW, in 2.5.63 and earlier, we tried to save and restore TLS state
> across signals, but we did it wrong and would corrupt it for any
> program that used arch_prctl. At that time, programs could switch
> userspace threads using signals and their TLS pointers would switch.
> In 2.5.64 and later, we broke set_thread_area users, fixed arch_prctl

That is probably non-issue as 2.5 was the devel branch anyway in the old
days. I'm not sure what the statement on breaking ABI was though then...

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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/