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

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

[Dave Hansen]

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.

A u64 was used for the protection key field in siginfo.  When the
containing union was aligned, this u64 unioned nicely with the
two 'void *'s in _addr_bnd.  But, on 32-bit, if the union was
unaligned, the u64 might grow the size of the union, breaking the
ABI for subsequent fields.

To fix this, we replace the u64 with an 'unsigned long'.  The long
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.  This also has the advantage that it allows existing 64-bit
userspace to keep working without modification.

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>
Cc: Stephen 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-26 08:50:47.760659292 -0800
+++ b/include/uapi/asm-generic/siginfo.h	2016-02-26 08:52:08.591330838 -0800
@@ -98,7 +98,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				unsigned long _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-26 08:51:50.357502608 -0800
+++ b/arch/mips/include/uapi/asm/siginfo.h	2016-02-26 08:51:55.206722873 -0800
@@ -93,7 +93,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				unsigned long _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-26 08:51:50.413505152 -0800
+++ b/arch/ia64/include/uapi/asm/siginfo.h	2016-02-26 08:52:00.806977252 -0800
@@ -70,7 +70,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				unsigned long _pkey;
 			};
 		} _sigfault;
 
_

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

[H. Peter Anvin]

On February 26, 2016 9:34:27 AM PST, Dave Hansen <dave@sr71.net> wrote:
>
>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.
>
>A u64 was used for the protection key field in siginfo.  When the
>containing union was aligned, this u64 unioned nicely with the
>two 'void *'s in _addr_bnd.  But, on 32-bit, if the union was
>unaligned, the u64 might grow the size of the union, breaking the
>ABI for subsequent fields.
>
>To fix this, we replace the u64 with an 'unsigned long'.  The long
>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.  This also has the advantage that it allows existing 64-bit
>userspace to keep working without modification.
>
>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>
>Cc: Stephen 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-26
>08:50:47.760659292 -0800
>+++ b/include/uapi/asm-generic/siginfo.h	2016-02-26 08:52:08.591330838
>-0800
>@@ -98,7 +98,7 @@ typedef struct siginfo {
> 					void __user *_upper;
> 				} _addr_bnd;
> 				/* used when si_code=SEGV_PKUERR */
>-				u64 _pkey;
>+				unsigned long _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-26
>08:51:50.357502608 -0800
>+++ b/arch/mips/include/uapi/asm/siginfo.h	2016-02-26
>08:51:55.206722873 -0800
>@@ -93,7 +93,7 @@ typedef struct siginfo {
> 					void __user *_upper;
> 				} _addr_bnd;
> 				/* used when si_code=SEGV_PKUERR */
>-				u64 _pkey;
>+				unsigned long _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-26
>08:51:50.413505152 -0800
>+++ b/arch/ia64/include/uapi/asm/siginfo.h	2016-02-26
>08:52:00.806977252 -0800
>@@ -70,7 +70,7 @@ typedef struct siginfo {
> 					void __user *_upper;
> 				} _addr_bnd;
> 				/* used when si_code=SEGV_PKUERR */
>-				u64 _pkey;
>+				unsigned long _pkey;
> 			};
> 		} _sigfault;
> 
>_

__u64 is okay, "unsigned long" is really messy in the presence of 32-on-64 bit ABIs...
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[Stephen Rothwell]

Hi,

On Fri, 26 Feb 2016 09:44:00 -0800 "H. Peter Anvin" <hpa@zytor.com> wrote:
>
> 
> __u64 is okay, "unsigned long" is really messy in the presence of 32-on-64 bit ABIs...

Yeah, but unfortunately, any 64 bit scalar type here will change the
alignment of the enclosing unions on (some) 32 bit platforms and thus
break the ABI.

-- 
Cheers,
Stephen Rothwell

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

[Ingo Molnar]

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi,
> 
> On Fri, 26 Feb 2016 09:44:00 -0800 "H. Peter Anvin" <hpa@zytor.com> wrote:
> >
> > 
> > __u64 is okay, "unsigned long" is really messy in the presence of 32-on-64 bit ABIs...
> 
> Yeah, but unfortunately, any 64 bit scalar type here will change the
> alignment of the enclosing unions on (some) 32 bit platforms and thus
> break the ABI.

Then a different solution has to be found.

Thanks,

	Ingo

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

[Dave Hansen]

On 02/27/2016 03:41 AM, Ingo Molnar wrote:
> * Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>> > On Fri, 26 Feb 2016 09:44:00 -0800 "H. Peter Anvin" <hpa@zytor.com> wrote:
>>> > > __u64 is okay, "unsigned long" is really messy in the presence of 32-on-64 bit ABIs...
>> > 
>> > Yeah, but unfortunately, any 64 bit scalar type here will change the
>> > alignment of the enclosing unions on (some) 32 bit platforms and thus
>> > break the ABI.
> Then a different solution has to be found.

I've acked Stephen's initial patch changing the 'u64' to an 'int'.  x86
only needs 4 bits, and in the remote chance that a future implementation
needed more space, we could easily add a second 32-bit field "_pkey_hi"
or something that wouldn't have the alignment issues of a true 64-bit type.

How should we get Stephen's patch in to the tip tree?

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

[H. Peter Anvin]

On February 27, 2016 11:16:44 AM PST, Dave Hansen <dave@sr71.net> wrote:
>On 02/27/2016 03:41 AM, Ingo Molnar wrote:
>> * Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>>> > On Fri, 26 Feb 2016 09:44:00 -0800 "H. Peter Anvin"
><hpa@zytor.com> wrote:
>>>> > > __u64 is okay, "unsigned long" is really messy in the presence
>of 32-on-64 bit ABIs...
>>> > 
>>> > Yeah, but unfortunately, any 64 bit scalar type here will change
>the
>>> > alignment of the enclosing unions on (some) 32 bit platforms and
>thus
>>> > break the ABI.
>> Then a different solution has to be found.
>
>I've acked Stephen's initial patch changing the 'u64' to an 'int'.  x86
>only needs 4 bits, and in the remote chance that a future
>implementation
>needed more space, we could easily add a second 32-bit field "_pkey_hi"
>or something that wouldn't have the alignment issues of a true 64-bit
>type.
>
>How should we get Stephen's patch in to the tip tree?

u32?
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[Stephen Rothwell]

Hi H.,

On Sat, 27 Feb 2016 11:35:08 -0800 "H. Peter Anvin" <hpa@zytor.com> wrote:
>
> On February 27, 2016 11:16:44 AM PST, Dave Hansen <dave@sr71.net> wrote:
> >On 02/27/2016 03:41 AM, Ingo Molnar wrote:  
> >> * Stephen Rothwell <sfr@canb.auug.org.au> wrote:  
> >>> > On Fri, 26 Feb 2016 09:44:00 -0800 "H. Peter Anvin"  
> ><hpa@zytor.com> wrote:  
> >>>> > > __u64 is okay, "unsigned long" is really messy in the presence  
> >of 32-on-64 bit ABIs...  
> >>> > 
> >>> > Yeah, but unfortunately, any 64 bit scalar type here will change  
> >the  
> >>> > alignment of the enclosing unions on (some) 32 bit platforms and  
> >thus  
> >>> > break the ABI.  
> >> Then a different solution has to be found.  
> >
> >I've acked Stephen's initial patch changing the 'u64' to an 'int'.  x86
> >only needs 4 bits, and in the remote chance that a future
> >implementation
> >needed more space, we could easily add a second 32-bit field "_pkey_hi"
> >or something that wouldn't have the alignment issues of a true 64-bit
> >type.
> >
> >How should we get Stephen's patch in to the tip tree?  
> 
> u32?

It would have to be __u32, but we already use int and unsigned int
extensively in the siginfo structure (which are both always assumed to
be 32 bits).  So "unsigned int" probably makes most sense.

I will submit that patch - with Dave's Ack.
-- 
Cheers,
Stephen Rothwell

[PATCH v2] signals, pkeys: make si_pkey 32 bits

[Stephen Rothwell]

In order to prevent a change of alignment of the _sifields union in the
siginfo structure on (some) 32 bit platforms and an ABI breakage, we
change the type of _pkey to unsigned int.  If more bits are needed in
the future, a second unsigned int could be added.

Fixes: cd0ea35ff551 ("signals, pkeys: Notify userspace about protection key faults")
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 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 0151cfab929d..19e7db0c9453 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;
+				unsigned int _pkey;
 			};
 		} _sigfault;
 
diff --git a/arch/mips/include/uapi/asm/siginfo.h b/arch/mips/include/uapi/asm/siginfo.h
index 6f4edf0d794c..3cc14f4a5936 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;
+				unsigned int _pkey;
 			};
 		} _sigfault;
 
diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
index 90384d55225b..f4459dc3d31b 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;
+				unsigned int _pkey;
 			};
 		} _sigfault;
 
-- 
2.7.0

-- 
Cheers,
Stephen Rothwell

Re: [PATCH v2] signals, pkeys: make si_pkey 32 bits

[Ingo Molnar]

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> In order to prevent a change of alignment of the _sifields union in the
> siginfo structure on (some) 32 bit platforms and an ABI breakage, we
> change the type of _pkey to unsigned int.  If more bits are needed in
> the future, a second unsigned int could be added.
> 
> Fixes: cd0ea35ff551 ("signals, pkeys: Notify userspace about protection key faults")
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
>  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 0151cfab929d..19e7db0c9453 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;
> +				unsigned int _pkey;
>  			};
>  		} _sigfault;
>  
> diff --git a/arch/mips/include/uapi/asm/siginfo.h b/arch/mips/include/uapi/asm/siginfo.h
> index 6f4edf0d794c..3cc14f4a5936 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;
> +				unsigned int _pkey;
>  			};
>  		} _sigfault;
>  
> diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> index 90384d55225b..f4459dc3d31b 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;
> +				unsigned int _pkey;
>  			};
>  		} _sigfault;
>  

Please use the standard ABI integer type pattern: __u32.

The advantage of only using __[su][8|16|32|64] integer types is that it's 
"obvious" at a glance that an ABI is bitness-invariant.

For example include/uapi/linux/perf_event.h only uses such ABI-safe types, and 
arch/x86/include/uapi is using these types 95%+ of the time.

( The various struct siginfo definitions should probably be harmonized as well, 
  but in a separate patch. )

Thanks,

	Ingo

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

[Ingo Molnar]

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> > u32?
> 
> It would have to be __u32, but we already use int and unsigned int
> extensively in the siginfo structure (which are both always assumed to
> be 32 bits).  So "unsigned int" probably makes most sense.

No. This whole mishap is an object lesson in why it's a bad idea to ever use ABI 
types outside of the __[us][8|16|32|64] space: some of them are 'fine', some of 
them (like longs) are not.

And we have to start somewhere, so we might as well start with new code that adds 
new ABI details: if a patch only uses __[us][8|16|32|64] types then it's easier to 
tell whether it's a safe ABI extension.

Thanks,

	Ingo

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

[Stephen Rothwell]

Hi Ingo,

On Mon, 29 Feb 2016 09:01:43 +0100 Ingo Molnar <mingo@kernel.org> wrote:
>
> * Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> 
> > > u32?  
> > 
> > It would have to be __u32, but we already use int and unsigned int
> > extensively in the siginfo structure (which are both always assumed to
> > be 32 bits).  So "unsigned int" probably makes most sense.  
> 
> No. This whole mishap is an object lesson in why it's a bad idea to ever use ABI 
> types outside of the __[us][8|16|32|64] space: some of them are 'fine', some of 
> them (like longs) are not.

Absolutely. I was just trying to be consistent with the rest of the structure.

Dave has submitted a follow up version which I have Acked.

-- 
Cheers,
Stephen Rothwell