Re: [PATCH v4 1/4] x86/signal/64: Add a comment about sigcontext->fs and gs
On Feb 16, 2016 12:42 AM, "Ingo Molnar" wrote:
>
>
> * Andy Lutomirski wrote:
>
> > These fields have a strange history. This tries to document it.
> >
> > This borrows from 9a036b93a344 ("x86/signal/64: Remove 'fs' and 'gs'
> > from sigcontext"), which was reverted by ed596cde9425 ("Revert x86
> > sigcontext cleanups").
> >
> > Signed-off-by: Andy Lutomirski
> > ---
> > arch/x86/include/uapi/asm/sigcontext.h | 19 +++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/arch/x86/include/uapi/asm/sigcontext.h
> > b/arch/x86/include/uapi/asm/sigcontext.h
> > index d485232f1e9f..47dae8150520 100644
> > --- a/arch/x86/include/uapi/asm/sigcontext.h
> > +++ b/arch/x86/include/uapi/asm/sigcontext.h
> > @@ -341,6 +341,25 @@ struct sigcontext {
> > __u64 rip;
> > __u64 eflags; /* RFLAGS */
> > __u16 cs;
> > +
> > + /*
> > + * Prior to 2.5.64 ("[PATCH] x86-64 updates for 2.5.64-bk3"),
> > + * Linux saved and restored fs and gs in these slots. This
> > + * was counterproductive, as fsbase and gsbase were never
> > + * saved, so arch_prctl was presumably unreliable.
> > + *
> > + * If these slots are ever needed for any other purpose, there
> > + * is some risk that very old 64-bit binaries could get
> > + * confused. I doubt that many such binaries still work,
> > + * though, since the same patch in 2.5.64 also removed the
> > + * 64-bit set_thread_area syscall, so it appears that there is
> > + * no TLS API beyond modify_ldt that works in both pre- and
> > + * post-2.5.64 kernels.
> > + *
> > + * There is at least one additional concern if these slots are
> > + * recycled for another purpose: some DOSEMU versions stash fs
> > + * and gs in these slots manually.
> > + */
> > __u16 gs;
> > __u16 fs;
>
> So I think this comment should be a lot more assertive: it should state that
> due
> to these old legacies that user-space learned to rely on the kernel must not
> touch
> these fields. I.e. it is an ABI - no ifs and whens.
We could still touch them to a limited extent. For example, we could
save FS and GS there (but we probably can't restore them).
I'll improve the comment.
>
> We should also rename them to __dosemu_gs_reserved/__dosemu_fs_reserved or so.
I suspect that DOSEMU won't build if we do that. In any event, I
think it should be a separate patch so that it can be trivially
reverted if needed.
--Andy
Re: [PATCH v4 1/4] x86/signal/64: Add a comment about sigcontext->fs and gs
* Andy Lutomirski wrote:
> These fields have a strange history. This tries to document it.
>
> This borrows from 9a036b93a344 ("x86/signal/64: Remove 'fs' and 'gs'
> from sigcontext"), which was reverted by ed596cde9425 ("Revert x86
> sigcontext cleanups").
>
> Signed-off-by: Andy Lutomirski
> ---
> arch/x86/include/uapi/asm/sigcontext.h | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/arch/x86/include/uapi/asm/sigcontext.h
> b/arch/x86/include/uapi/asm/sigcontext.h
> index d485232f1e9f..47dae8150520 100644
> --- a/arch/x86/include/uapi/asm/sigcontext.h
> +++ b/arch/x86/include/uapi/asm/sigcontext.h
> @@ -341,6 +341,25 @@ struct sigcontext {
> __u64 rip;
> __u64 eflags; /* RFLAGS */
> __u16 cs;
> +
> + /*
> + * Prior to 2.5.64 ("[PATCH] x86-64 updates for 2.5.64-bk3"),
> + * Linux saved and restored fs and gs in these slots. This
> + * was counterproductive, as fsbase and gsbase were never
> + * saved, so arch_prctl was presumably unreliable.
> + *
> + * If these slots are ever needed for any other purpose, there
> + * is some risk that very old 64-bit binaries could get
> + * confused. I doubt that many such binaries still work,
> + * though, since the same patch in 2.5.64 also removed the
> + * 64-bit set_thread_area syscall, so it appears that there is
> + * no TLS API beyond modify_ldt that works in both pre- and
> + * post-2.5.64 kernels.
> + *
> + * There is at least one additional concern if these slots are
> + * recycled for another purpose: some DOSEMU versions stash fs
> + * and gs in these slots manually.
> + */
> __u16 gs;
> __u16 fs;
So I think this comment should be a lot more assertive: it should state that
due
to these old legacies that user-space learned to rely on the kernel must not
touch
these fields. I.e. it is an ABI - no ifs and whens.
We should also rename them to __dosemu_gs_reserved/__dosemu_fs_reserved or so.
These are ABI legacies for DOSEMU, no need to pretend otherwise. There's very
little to be sorry about: ABI promises have consequences, we should codify that
here and move on. Also please document it precisely which syscall(s) expose
this
ABI.
If we need more space for new, cleaner functionality we'll use other fields.
(like
your later patches do.)
Thanks,
Ingo
