Update changelog with better description of the issue from Ingo.
--
From: Dave Hansen <[email protected]>
Stephen Rothwell reported:
http://lkml.kernel.org/r/[email protected]
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 <[email protected]>
Acked-by: Stehen Rothwell <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Helge Deller <[email protected]>
---
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;
_
* Dave Hansen <[email protected]> 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
Commit-ID: 16bc7477807393efb5b81f875888ee9221ead3a1
Gitweb: http://git.kernel.org/tip/16bc7477807393efb5b81f875888ee9221ead3a1
Author: Dave Hansen <[email protected]>
AuthorDate: Tue, 1 Mar 2016 04:54:51 -0800
Committer: Ingo Molnar <[email protected]>
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/[email protected]
... 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 <[email protected]>
Signed-off-by: Dave Hansen <[email protected]>
Acked-by: Stehen Rothwell <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Fixes: cd0ea35ff551 ("signals, pkeys: Notify userspace about protection key faults")
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
On Thu, Mar 3, 2016 at 8:53 AM, tip-bot for Dave Hansen
<[email protected]> 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
* Linus Torvalds <[email protected]> wrote:
> On Thu, Mar 3, 2016 at 8:53 AM, tip-bot for Dave Hansen
> <[email protected]> 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
Commit-ID: 49cd53bf14aeb471c4a2682300dfc05ef2fd09f2
Gitweb: http://git.kernel.org/tip/49cd53bf14aeb471c4a2682300dfc05ef2fd09f2
Author: Dave Hansen <[email protected]>
AuthorDate: Tue, 1 Mar 2016 04:54:51 -0800
Committer: Ingo Molnar <[email protected]>
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/[email protected]
... 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 <[email protected]>
Signed-off-by: Dave Hansen <[email protected]>
Acked-by: Stehen Rothwell <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Fixes: cd0ea35ff551 ("signals, pkeys: Notify userspace about protection key faults")
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
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?
On Sat, Mar 5, 2016 at 8:52 AM, Peter Zijlstra <[email protected]> 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
* Linus Torvalds <[email protected]> wrote:
> On Sat, Mar 5, 2016 at 8:52 AM, Peter Zijlstra <[email protected]> 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