[PATCH] [v4] x86, pkeys: fix siginfo ABI breakage from new field

[PATCH] [v4] x86, pkeys: fix siginfo ABI breakage from new field

[Dave Hansen]

Update changelog with better description of the issue from Ingo.

--

From: Dave Hansen <dave.hansen@linux.intel.com>

Stephen Rothwell reported:

	http://lkml.kernel.org/r/20160226164406.065a1ffc@canb.auug.org.au

that the Memory Protection Keys patches from the tip tree broke a
build-time check on an ARM build because they changed the ABI of
siginfo.

If u64 has a natural alignment of 8 bytes (this is rare, most 32-bit
platforms align it to 4 bytes), then the leadup to the _sifields union
matters:

typedef struct siginfo {
        int si_signo;
        int si_errno;
        int si_code;

        union {
	...
        } _sifields;
} __ARCH_SI_ATTRIBUTES siginfo_t;

Note how the first 3 fields give us 12 bytes, so _sifields is not 8
naturally bytes aligned.

Before the _pkey field addition the largest element of _sifields (on
32-bit platforms) was 32 bits. With the u64 added, the minimum alignment
requirement increased to 8 bytes on those (rare) 32-bit platforms. Thus
GCC padded the space after si_code with 4 extra bytes, and shifted all
_sifields offsets by 4 bytes - breaking the ABI of all of those
remaining fields.

On 64-bit platforms this problem was hidden due to _sifields already
having numerous fields with natural 8 bytes alignment (pointers).

To fix this, we replace the u64 with an '__u32'.  The __u32 is
guaranteed to union well with the pointers from _addr_bnd.  It is also
plenty large enough to store the 16-bit pkey we have today on x86.

I also shouldn't have been using a u64 in a userspace API to begin with.

Fixes: cd0ea35ff551 ("signals, pkeys: Notify userspace about protection key faults")
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Acked-by: Stehen Rothwell <sfr@canb.auug.org.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-next@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Helge Deller <deller@gmx.de>
---

 b/arch/ia64/include/uapi/asm/siginfo.h |    2 +-
 b/arch/mips/include/uapi/asm/siginfo.h |    2 +-
 b/include/uapi/asm-generic/siginfo.h   |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff -puN include/uapi/asm-generic/siginfo.h~pkeys-101-fix-siginfo include/uapi/asm-generic/siginfo.h
--- a/include/uapi/asm-generic/siginfo.h~pkeys-101-fix-siginfo	2016-02-29 09:22:45.327228965 -0800
+++ b/include/uapi/asm-generic/siginfo.h	2016-02-29 09:22:45.333229241 -0800
@@ -98,7 +98,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;
 
diff -puN arch/mips/include/uapi/asm/siginfo.h~pkeys-101-fix-siginfo arch/mips/include/uapi/asm/siginfo.h
--- a/arch/mips/include/uapi/asm/siginfo.h~pkeys-101-fix-siginfo	2016-02-29 09:22:45.330229103 -0800
+++ b/arch/mips/include/uapi/asm/siginfo.h	2016-02-29 09:22:45.333229241 -0800
@@ -93,7 +93,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;
 
diff -puN arch/ia64/include/uapi/asm/siginfo.h~pkeys-101-fix-siginfo arch/ia64/include/uapi/asm/siginfo.h
--- a/arch/ia64/include/uapi/asm/siginfo.h~pkeys-101-fix-siginfo	2016-02-29 09:22:45.331229149 -0800
+++ b/arch/ia64/include/uapi/asm/siginfo.h	2016-02-29 09:22:45.333229241 -0800
@@ -70,7 +70,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;
 
_

Re: [PATCH] [v4] x86, pkeys: fix siginfo ABI breakage from new field

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> To fix this, we replace the u64 with an '__u32'.  The __u32 is guaranteed to 
> union well with the pointers from _addr_bnd.  It is also plenty large enough to 
> store the 16-bit pkey we have today on x86.

The 'union well' sentence is really a leftover from the earlier changelog (the 
problem was never about interaction between union members) - a better explantion 
is:

> To fix this, we replace the u64 with an '__u32'.  The __u32 does not change the 
> minimum alignment requirements of the structure and it is also plenty large 
> enough to store the 16-bit pkey we have today on x86.

I fixed this up locally, no need to resend.

Thanks,

	Ingo

[tip:mm/pkeys] mm/pkeys: Fix siginfo ABI breakage caused by new u64 field

[tip-bot for Dave Hansen]

Commit-ID:  16bc7477807393efb5b81f875888ee9221ead3a1
Gitweb:     http://git.kernel.org/tip/16bc7477807393efb5b81f875888ee9221ead3a1
Author:     Dave Hansen <dave.hansen@linux.intel.com>
AuthorDate: Tue, 1 Mar 2016 04:54:51 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 3 Mar 2016 16:44:21 +0100

mm/pkeys: Fix siginfo ABI breakage caused by new u64 field

Stephen Rothwell reported this linux-next build failure:

	http://lkml.kernel.org/r/20160226164406.065a1ffc@canb.auug.org.au

... caused by the Memory Protection Keys patches from the tip tree triggering
a newly introduced build-time sanity check on an ARM build, because they changed
the ABI of siginfo in an unexpected way.

If u64 has a natural alignment of 8 bytes (this is rare, most 32-bit
platforms align it to 4 bytes), then the leadup to the _sifields union
matters:

typedef struct siginfo {
        int si_signo;
        int si_errno;
        int si_code;

        union {
	...
        } _sifields;
} __ARCH_SI_ATTRIBUTES siginfo_t;

Note how the first 3 fields give us 12 bytes, so _sifields is not 8
naturally bytes aligned.

Before the _pkey field addition the largest element of _sifields (on
32-bit platforms) was 32 bits. With the u64 added, the minimum alignment
requirement increased to 8 bytes on those (rare) 32-bit platforms. Thus
GCC padded the space after si_code with 4 extra bytes, and shifted all
_sifields offsets by 4 bytes - breaking the ABI of all of those
remaining fields.

On 64-bit platforms this problem was hidden due to _sifields already
having numerous fields with natural 8 bytes alignment (pointers).

To fix this, we replace the u64 with an '__u32'.  The __u32 does not
increase the minimum alignment requirement of the union, and it is
also large enough to store the 16-bit pkey we have today on x86.

Reported-by: Stehen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Acked-by: Stehen Rothwell <sfr@canb.auug.org.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dave Hansen <dave@sr71.net>
Cc: Helge Deller <deller@gmx.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-next@vger.kernel.org
Fixes: cd0ea35ff551 ("signals, pkeys: Notify userspace about protection key faults")
Link: http://lkml.kernel.org/r/20160301125451.02C7426D@viggo.jf.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/ia64/include/uapi/asm/siginfo.h | 2 +-
 arch/mips/include/uapi/asm/siginfo.h | 2 +-
 include/uapi/asm-generic/siginfo.h   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/ia64/include/uapi/asm/siginfo.h b/arch/ia64/include/uapi/asm/siginfo.h
index 0151cfa..f72bf01 100644
--- a/arch/ia64/include/uapi/asm/siginfo.h
+++ b/arch/ia64/include/uapi/asm/siginfo.h
@@ -70,7 +70,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;
 
diff --git a/arch/mips/include/uapi/asm/siginfo.h b/arch/mips/include/uapi/asm/siginfo.h
index 6f4edf0..cc49dc2 100644
--- a/arch/mips/include/uapi/asm/siginfo.h
+++ b/arch/mips/include/uapi/asm/siginfo.h
@@ -93,7 +93,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;
 
diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
index 90384d5..1abaf62 100644
--- a/include/uapi/asm-generic/siginfo.h
+++ b/include/uapi/asm-generic/siginfo.h
@@ -98,7 +98,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;
 

Re: [tip:mm/pkeys] mm/pkeys: Fix siginfo ABI breakage caused by new u64 field

[Linus Torvalds]

On Thu, Mar 3, 2016 at 8:53 AM, tip-bot for Dave Hansen
<tipbot@zytor.com> wrote:
>
> If u64 has a natural alignment of 8 bytes (this is rare, most 32-bit
> platforms align it to 4 bytes), then the leadup to the _sifields union
> matters:

Side note: I'm not sure that "this is rare" comment is necessarily correct.

I think natural alignment is pretty common, even for 32-bit targets.
x86-32 is I think the exception rather than the rule.

There is some real odd case iirc - embedded m68k, which has some
ridiculous alignment rules. I think it only ever aligns to 16-bit
boundaries.

I do keep coming back to the fact that we should *probably* just do
something like

    typedef unsigned long long __attribute__((aligned(8))) __u64;

and then introduce a separate "u64_unaligned" type for all the legacy
cases that depended on 32-bit alignment.

It's horrendously nasty to test, though.

                  Linus

Re: [tip:mm/pkeys] mm/pkeys: Fix siginfo ABI breakage caused by new u64 field

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Mar 3, 2016 at 8:53 AM, tip-bot for Dave Hansen
> <tipbot@zytor.com> wrote:
> >
> > If u64 has a natural alignment of 8 bytes (this is rare, most 32-bit
> > platforms align it to 4 bytes), then the leadup to the _sifields union
> > matters:
> 
> Side note: I'm not sure that "this is rare" comment is necessarily correct.
> 
> I think natural alignment is pretty common, even for 32-bit targets.
> x86-32 is I think the exception rather than the rule.
> 
> There is some real odd case iirc - embedded m68k, which has some
> ridiculous alignment rules. I think it only ever aligns to 16-bit
> boundaries.

So I got curious about this, but couldn't find any good online documentation about 
the alignment defaults of various architectures that GCC supports. So I reverted 
the fix and added the new check from linux-next:

    Revert "mm/pkeys: Fix siginfo ABI breakage caused by new u64 field"
    kernel/signal.c: add compile-time check for __ARCH_SI_PREAMBLE_SIZE

... which does:

 void __init signals_init(void)
 {
+       /* If this check fails, the __ARCH_SI_PREAMBLE_SIZE value is wrong! */
+       BUILD_BUG_ON(__ARCH_SI_PREAMBLE_SIZE
+               != offsetof(struct siginfo, _sifields._pad));
+

and tested it on the -tip cross-arch build testing suite, which gave the following 
result (only 32-bit architectures listed):

                                 (warns)               (warns)
testing     x86-32:  -git:  pass (    0),  -tip:  pass (    0)
testing        arm:  -git:  pass (    1),  -tip:  FAIL  .....
testing   blackfin:  -git:  pass (    0),  -tip:  pass (    0)
testing       cris:  -git:  pass (   32),  -tip:  pass (   32)
testing        frv:  -git:  pass (    1),  -tip:  FAIL  .....
testing       m32r:  -git:  pass (    6),  -tip:  pass (    6)
testing       m68k:  -git:  pass (    1),  -tip:  pass (    1)
testing microblaze:  -git:  pass (    0),  -tip:  pass (    0)
testing       mips:  -git:  pass (    1),  -tip:  FAIL  .....
testing   openrisc:  -git:  pass (    2),  -tip:  pass (    2)
testing     parisc:  -git:  pass (    0),  -tip:  FAIL  .....
testing         sh:  -git:  pass (   36),  -tip:  pass (   36)
testing      sparc:  -git:  pass (    0),  -tip:  FAIL  .....
testing       tile:  -git:  pass (    5),  -tip:  pass (    5)
testing     xtensa:  -git:  pass (    0),  -tip:  FAIL  .....
testing  powerpc32:  -git:  pass (    0),  -tip:  FAIL  .....

so if my test is correct then it's 9 architectures that align u64 to 4 bytes, vs. 
7 that align it to 8 bytes.

So naturally aligned u64 is definitely not 'rare' (so the characterisation in my 
changelog is wrong), but it's not dominant either.

FWIIW: if we only list 'major' architectures then x86-32 is indeed the odd one 
out...

> I do keep coming back to the fact that we should *probably* just do
> something like
> 
>     typedef unsigned long long __attribute__((aligned(8))) __u64;
> 
> and then introduce a separate "u64_unaligned" type for all the legacy
> cases that depended on 32-bit alignment.
> 
> It's horrendously nasty to test, though.

So in theory we could test most of it by comparing the disassembly of allyesconfig 
builds, but comparing disassemblies is a pretty hard to use method in practice.

A more workable method would be to have a test .c file that includes all UAPI 
structures in existence and defines a variable out of every single one, and then 
generates a list of sizeof() values or so. But even that isn't perfect: a 
structure might shift some fields forward, into a pre-existing hole, without 
changing the sizeof? We'd need a list of all field offsets in all structures to be 
really sure, and that's nasty.

Thanks,

	Ingo

Re: [tip:mm/pkeys] mm/pkeys: Fix siginfo ABI breakage caused by new u64 field

[Peter Zijlstra]

On Sat, Mar 05, 2016 at 02:50:06PM +0100, Ingo Molnar wrote:
> A more workable method would be to have a test .c file that includes all UAPI 
> structures in existence and defines a variable out of every single one, and then 
> generates a list of sizeof() values or so. But even that isn't perfect: a 
> structure might shift some fields forward, into a pre-existing hole, without 
> changing the sizeof? We'd need a list of all field offsets in all structures to be 
> really sure, and that's nasty.

pahole has such logic, right?

Re: [tip:mm/pkeys] mm/pkeys: Fix siginfo ABI breakage caused by new u64 field

[Linus Torvalds]

On Sat, Mar 5, 2016 at 8:52 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Sat, Mar 05, 2016 at 02:50:06PM +0100, Ingo Molnar wrote:
>> A more workable method would be to have a test .c file that includes all UAPI
>> structures in existence and defines a variable out of every single one, and then
>> generates a list of sizeof() values or so. But even that isn't perfect: a
>> structure might shift some fields forward, into a pre-existing hole, without
>> changing the sizeof? We'd need a list of all field offsets in all structures to be
>> really sure, and that's nasty.
>
> pahole has such logic, right?

sparse could be taught to warn about unaligned u64's, but there are
still config issues and issues across other architectures, and if some
case gets missed it can be really quite painful.

          Linus

Re: [tip:mm/pkeys] mm/pkeys: Fix siginfo ABI breakage caused by new u64 field

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, Mar 5, 2016 at 8:52 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Sat, Mar 05, 2016 at 02:50:06PM +0100, Ingo Molnar wrote:
> >> A more workable method would be to have a test .c file that includes all UAPI
> >> structures in existence and defines a variable out of every single one, and then
> >> generates a list of sizeof() values or so. But even that isn't perfect: a
> >> structure might shift some fields forward, into a pre-existing hole, without
> >> changing the sizeof? We'd need a list of all field offsets in all structures to be
> >> really sure, and that's nasty.
> >
> > pahole has such logic, right?
> 
> sparse could be taught to warn about unaligned u64's, but there are
> still config issues and issues across other architectures, and if some
> case gets missed it can be really quite painful.

So I think what might work is to define bitness and alignment-independent ABI 
structures, and add (tooling) infrastructure to enforce that invariance.

For example every ABI detail in include/uapi/linux/perf_event.h is supposed to be 
alignment-independent: it should be the same structure and same fields on all 
32-bit and 64-bit platforms, regardless of the natural alignment of u64.

It's as close to an 'architecture independent' ABI as we can get IMHO. (With the 
notable exception of endianness: making endianness an invariant via dynamic 
endianness flags or so would be a mistake IMHO - structures that can be 
transmitted between different machines via network or via disk should pick one 
particular endianness statically and stick with that.)

If we can build tooling that checks _that_ kind of struct/ABI invariance, it would 
be a lot easier to ensure that future changes don't break the ABI.

New structures could be annotated to be arch-invariant, via something like:

	struct foo __arch_invariant_ABI {
		...
	};

and tooling could pick it up from there.

We do have a healthy body of 'messy' ABIs, which make liberal use of longs, 
unaligned u64's and other non-invariant constructs - those would have to be 
cleaned up and in the worst case they would have to be split into several explicit 
variants.

I tried to do something like that with a particularly messy x86 header lately, see 
arch/x86/include/uapi/asm/sigcontext.h, but it's a pretty slow and painful process 
altogether...

Thanks,

	Ingo

[tip:mm/pkeys] mm/pkeys: Fix siginfo ABI breakage caused by new u64 field

[tip-bot for Dave Hansen]

Commit-ID:  49cd53bf14aeb471c4a2682300dfc05ef2fd09f2
Gitweb:     http://git.kernel.org/tip/49cd53bf14aeb471c4a2682300dfc05ef2fd09f2
Author:     Dave Hansen <dave.hansen@linux.intel.com>
AuthorDate: Tue, 1 Mar 2016 04:54:51 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 5 Mar 2016 15:00:06 +0100

mm/pkeys: Fix siginfo ABI breakage caused by new u64 field

Stephen Rothwell reported this linux-next build failure:

	http://lkml.kernel.org/r/20160226164406.065a1ffc@canb.auug.org.au

... caused by the Memory Protection Keys patches from the tip tree triggering
a newly introduced build-time sanity check on an ARM build, because they changed
the ABI of siginfo in an unexpected way.

If u64 has a natural alignment of 8 bytes (which is the case on most mainstream
platforms, with the notable exception of x86-32), then the leadup to the
_sifields union matters:

typedef struct siginfo {
        int si_signo;
        int si_errno;
        int si_code;

        union {
	...
        } _sifields;
} __ARCH_SI_ATTRIBUTES siginfo_t;

Note how the first 3 fields give us 12 bytes, so _sifields is not 8
naturally bytes aligned.

Before the _pkey field addition the largest element of _sifields (on
32-bit platforms) was 32 bits. With the u64 added, the minimum alignment
requirement increased to 8 bytes on those (rare) 32-bit platforms. Thus
GCC padded the space after si_code with 4 extra bytes, and shifted all
_sifields offsets by 4 bytes - breaking the ABI of all of those
remaining fields.

On 64-bit platforms this problem was hidden due to _sifields already
having numerous fields with natural 8 bytes alignment (pointers).

To fix this, we replace the u64 with an '__u32'.  The __u32 does not
increase the minimum alignment requirement of the union, and it is
also large enough to store the 16-bit pkey we have today on x86.

Reported-by: Stehen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Acked-by: Stehen Rothwell <sfr@canb.auug.org.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dave Hansen <dave@sr71.net>
Cc: Helge Deller <deller@gmx.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-next@vger.kernel.org
Fixes: cd0ea35ff551 ("signals, pkeys: Notify userspace about protection key faults")
Link: http://lkml.kernel.org/r/20160301125451.02C7426D@viggo.jf.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/ia64/include/uapi/asm/siginfo.h | 2 +-
 arch/mips/include/uapi/asm/siginfo.h | 2 +-
 include/uapi/asm-generic/siginfo.h   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/ia64/include/uapi/asm/siginfo.h b/arch/ia64/include/uapi/asm/siginfo.h
index 0151cfa..f72bf01 100644
--- a/arch/ia64/include/uapi/asm/siginfo.h
+++ b/arch/ia64/include/uapi/asm/siginfo.h
@@ -70,7 +70,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;
 
diff --git a/arch/mips/include/uapi/asm/siginfo.h b/arch/mips/include/uapi/asm/siginfo.h
index 6f4edf0..cc49dc2 100644
--- a/arch/mips/include/uapi/asm/siginfo.h
+++ b/arch/mips/include/uapi/asm/siginfo.h
@@ -93,7 +93,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;
 
diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
index 90384d5..1abaf62 100644
--- a/include/uapi/asm-generic/siginfo.h
+++ b/include/uapi/asm-generic/siginfo.h
@@ -98,7 +98,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;