[PATCH tip 1/2] signal, perf: Fix siginfo_t by avoiding u64 on 32-bit architectures

[PATCH tip 1/2] signal, perf: Fix siginfo_t by avoiding u64 on 32-bit architectures

[Marco Elver]

On some architectures, like Arm, the alignment of a structure is that of
its largest member.

This means that there is no portable way to add 64-bit integers to
siginfo_t on 32-bit architectures, because siginfo_t does not contain
any 64-bit integers on 32-bit architectures.

In the case of the si_perf field, word size is sufficient since there is
no exact requirement on size, given the data it contains is user-defined
via perf_event_attr::sig_data. On 32-bit architectures, any excess bits
of perf_event_attr::sig_data will therefore be truncated when copying
into si_perf.

Since this field is intended to disambiguate events (e.g. encoding
relevant information if there are more events of the same type), 32 bits
should provide enough entropy to do so on 32-bit architectures.

For 64-bit architectures, no change is intended.

Fixes: fb6cc127e0b6 ("signal: Introduce TRAP_PERF si_code and si_perf to siginfo")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Marco Elver <elver@google.com>
---

Note: I added static_assert()s to verify the siginfo_t layout to
arch/arm and arch/arm64, which caught the problem. I'll send them
separately to arm&arm64 maintainers respectively.
---
 include/linux/compat.h                                | 2 +-
 include/uapi/asm-generic/siginfo.h                    | 2 +-
 tools/testing/selftests/perf_events/sigtrap_threads.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/compat.h b/include/linux/compat.h
index c8821d966812..f0d2dd35d408 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -237,7 +237,7 @@ typedef struct compat_siginfo {
 					u32 _pkey;
 				} _addr_pkey;
 				/* used when si_code=TRAP_PERF */
-				compat_u64 _perf;
+				compat_ulong_t _perf;
 			};
 		} _sigfault;
 
diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
index d0bb9125c853..03d6f6d2c1fe 100644
--- a/include/uapi/asm-generic/siginfo.h
+++ b/include/uapi/asm-generic/siginfo.h
@@ -92,7 +92,7 @@ union __sifields {
 				__u32 _pkey;
 			} _addr_pkey;
 			/* used when si_code=TRAP_PERF */
-			__u64 _perf;
+			unsigned long _perf;
 		};
 	} _sigfault;
 
diff --git a/tools/testing/selftests/perf_events/sigtrap_threads.c b/tools/testing/selftests/perf_events/sigtrap_threads.c
index 9c0fd442da60..78ddf5e11625 100644
--- a/tools/testing/selftests/perf_events/sigtrap_threads.c
+++ b/tools/testing/selftests/perf_events/sigtrap_threads.c
@@ -44,7 +44,7 @@ static struct {
 } ctx;
 
 /* Unique value to check si_perf is correctly set from perf_event_attr::sig_data. */
-#define TEST_SIG_DATA(addr) (~(uint64_t)(addr))
+#define TEST_SIG_DATA(addr) (~(unsigned long)(addr))
 
 static struct perf_event_attr make_event_attr(bool enabled, volatile void *addr)
 {
-- 
2.31.1.498.g6c1eba8ee3d-goog

[PATCH tip 2/2] signal, perf: Add missing TRAP_PERF case in siginfo_layout()

[Marco Elver]

Add the missing TRAP_PERF case in siginfo_layout() for interpreting the
layout correctly as SIL_PERF_EVENT instead of just SIL_FAULT. This
ensures the si_perf field is copied and not just the si_addr field.

This was caught and tested by running the perf_events/sigtrap_threads
kselftest as a 32-bit binary with a 64-bit kernel.

Fixes: fb6cc127e0b6 ("signal: Introduce TRAP_PERF si_code and si_perf to siginfo")
Signed-off-by: Marco Elver <elver@google.com>
---
 kernel/signal.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/signal.c b/kernel/signal.c
index 9ed81ee4ff17..b354655a0e57 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3251,6 +3251,8 @@ enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
 			else if ((sig == SIGSEGV) && (si_code == SEGV_PKUERR))
 				layout = SIL_FAULT_PKUERR;
 #endif
+			else if ((sig == SIGTRAP) && (si_code == TRAP_PERF))
+				layout = SIL_PERF_EVENT;
 		}
 		else if (si_code <= NSIGPOLL)
 			layout = SIL_POLL;
-- 
2.31.1.498.g6c1eba8ee3d-goog

Re: [PATCH tip 1/2] signal, perf: Fix siginfo_t by avoiding u64 on 32-bit architectures

[Jon Hunter]

On 22/04/2021 07:44, Marco Elver wrote:
> On some architectures, like Arm, the alignment of a structure is that of
> its largest member.
> 
> This means that there is no portable way to add 64-bit integers to
> siginfo_t on 32-bit architectures, because siginfo_t does not contain
> any 64-bit integers on 32-bit architectures.
> 
> In the case of the si_perf field, word size is sufficient since there is
> no exact requirement on size, given the data it contains is user-defined
> via perf_event_attr::sig_data. On 32-bit architectures, any excess bits
> of perf_event_attr::sig_data will therefore be truncated when copying
> into si_perf.
> 
> Since this field is intended to disambiguate events (e.g. encoding
> relevant information if there are more events of the same type), 32 bits
> should provide enough entropy to do so on 32-bit architectures.
> 
> For 64-bit architectures, no change is intended.
> 
> Fixes: fb6cc127e0b6 ("signal: Introduce TRAP_PERF si_code and si_perf to siginfo")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Marco Elver <elver@google.com>


Thanks for fixing!

Tested-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

RE: [PATCH tip 1/2] signal, perf: Fix siginfo_t by avoiding u64 on 32-bit architectures

[David Laight]

From: Marco Elver
> Sent: 22 April 2021 07:45
> 
> On some architectures, like Arm, the alignment of a structure is that of
> its largest member.

That is true everywhere.
(Apart from obscure ABI where structure have at least 4 byte alignment!)

> This means that there is no portable way to add 64-bit integers to
> siginfo_t on 32-bit architectures, because siginfo_t does not contain
> any 64-bit integers on 32-bit architectures.

Uh?

The actual problem is that adding a 64-bit aligned item to the union
forces the union to be 8 byte aligned and adds a 4 byte pad before it
(and possibly another one at the end of the structure).

> In the case of the si_perf field, word size is sufficient since there is
> no exact requirement on size, given the data it contains is user-defined
> via perf_event_attr::sig_data. On 32-bit architectures, any excess bits
> of perf_event_attr::sig_data will therefore be truncated when copying
> into si_perf.

Is that right on BE architectures?

> Since this field is intended to disambiguate events (e.g. encoding
> relevant information if there are more events of the same type), 32 bits
> should provide enough entropy to do so on 32-bit architectures.

What is the size of the field used to supply the data?
The size of the returned item really ought to match.

Much as I hate __packed, you could add __packed to the
definition of the structure member _perf.
The compiler will remove the padding before it and will
assume it has the alignment of the previous item.

So it will never use byte accesses.

	David

> 
> For 64-bit architectures, no change is intended.
> 
> Fixes: fb6cc127e0b6 ("signal: Introduce TRAP_PERF si_code and si_perf to siginfo")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> 
> Note: I added static_assert()s to verify the siginfo_t layout to
> arch/arm and arch/arm64, which caught the problem. I'll send them
> separately to arm&arm64 maintainers respectively.
> ---
>  include/linux/compat.h                                | 2 +-
>  include/uapi/asm-generic/siginfo.h                    | 2 +-
>  tools/testing/selftests/perf_events/sigtrap_threads.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index c8821d966812..f0d2dd35d408 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -237,7 +237,7 @@ typedef struct compat_siginfo {
>  					u32 _pkey;
>  				} _addr_pkey;
>  				/* used when si_code=TRAP_PERF */
> -				compat_u64 _perf;
> +				compat_ulong_t _perf;
>  			};
>  		} _sigfault;
> 
> diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> index d0bb9125c853..03d6f6d2c1fe 100644
> --- a/include/uapi/asm-generic/siginfo.h
> +++ b/include/uapi/asm-generic/siginfo.h
> @@ -92,7 +92,7 @@ union __sifields {
>  				__u32 _pkey;
>  			} _addr_pkey;
>  			/* used when si_code=TRAP_PERF */
> -			__u64 _perf;
> +			unsigned long _perf;
>  		};
>  	} _sigfault;
> 
> diff --git a/tools/testing/selftests/perf_events/sigtrap_threads.c
> b/tools/testing/selftests/perf_events/sigtrap_threads.c
> index 9c0fd442da60..78ddf5e11625 100644
> --- a/tools/testing/selftests/perf_events/sigtrap_threads.c
> +++ b/tools/testing/selftests/perf_events/sigtrap_threads.c
> @@ -44,7 +44,7 @@ static struct {
>  } ctx;
> 
>  /* Unique value to check si_perf is correctly set from perf_event_attr::sig_data. */
> -#define TEST_SIG_DATA(addr) (~(uint64_t)(addr))
> +#define TEST_SIG_DATA(addr) (~(unsigned long)(addr))
> 
>  static struct perf_event_attr make_event_attr(bool enabled, volatile void *addr)
>  {
> --
> 2.31.1.498.g6c1eba8ee3d-goog

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH tip 1/2] signal, perf: Fix siginfo_t by avoiding u64 on 32-bit architectures

[Marco Elver]

On Thu, 22 Apr 2021 at 11:48, David Laight <David.Laight@aculab.com> wrote:
>
> From: Marco Elver
> > Sent: 22 April 2021 07:45
> >
> > On some architectures, like Arm, the alignment of a structure is that of
> > its largest member.
>
> That is true everywhere.
> (Apart from obscure ABI where structure have at least 4 byte alignment!)

For instance, x86 didn't complain, nor did m68k. Both of them have
compile-time checks for the layout (I'm adding those for Arm
elsewhere).

> > This means that there is no portable way to add 64-bit integers to
> > siginfo_t on 32-bit architectures, because siginfo_t does not contain
> > any 64-bit integers on 32-bit architectures.
>
> Uh?
>
> The actual problem is that adding a 64-bit aligned item to the union
> forces the union to be 8 byte aligned and adds a 4 byte pad before it
> (and possibly another one at the end of the structure).

Yes, which means there's no portable way (without starting to add
attributes that are outside the C std) to add 64-bit integers to
siginfo_t without breaking the ABI on 32-bit architectures.

> > In the case of the si_perf field, word size is sufficient since there is
> > no exact requirement on size, given the data it contains is user-defined
> > via perf_event_attr::sig_data. On 32-bit architectures, any excess bits
> > of perf_event_attr::sig_data will therefore be truncated when copying
> > into si_perf.
>
> Is that right on BE architectures?

We effectively do

  u64 sig_data = ...;
  unsigned long si_perf = sig_data;

Since the user decides what to place into perf_event_attr::sig_data,
whatever ends up in si_perf is fully controllable by the user, who
knows which arch they're on. So I do not think this is a problem.

> > Since this field is intended to disambiguate events (e.g. encoding
> > relevant information if there are more events of the same type), 32 bits
> > should provide enough entropy to do so on 32-bit architectures.
>
> What is the size of the field used to supply the data?
> The size of the returned item really ought to match.

It's u64, but because perf_event_attr wants fixed size fields, this
can't change.

> Much as I hate __packed, you could add __packed to the
> definition of the structure member _perf.
> The compiler will remove the padding before it and will
> assume it has the alignment of the previous item.
>
> So it will never use byte accesses.

Sure __packed works for Arm. But I think there's no precedent using
this on siginfo_t, possibly for good reasons? I simply can't find
evidence that this is portable on *all* architectures and for *all*
possible definitions of siginfo_t, including those that live in things
like glibc.

Can we confirm that __packed is fine to add to siginfo_t on *all*
architectures for *all* possible definitions of siginfo_t? I currently
can't. And given it's outside the scope of the C standard (as of C11
we got _Alignas, but that doesn't help I think), I'd vote to not
venture too far for code that should be portable especially things as
important as siginfo_t, and has definitions *outside* the kernel (I
know we do lots of non-standard things, but others might not).

Thanks,
-- Marco

>         David
>
> >
> > For 64-bit architectures, no change is intended.
> >
> > Fixes: fb6cc127e0b6 ("signal: Introduce TRAP_PERF si_code and si_perf to siginfo")
> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Reported-by: Jon Hunter <jonathanh@nvidia.com>
> > Signed-off-by: Marco Elver <elver@google.com>
> > ---
> >
> > Note: I added static_assert()s to verify the siginfo_t layout to
> > arch/arm and arch/arm64, which caught the problem. I'll send them
> > separately to arm&arm64 maintainers respectively.
> > ---
> >  include/linux/compat.h                                | 2 +-
> >  include/uapi/asm-generic/siginfo.h                    | 2 +-
> >  tools/testing/selftests/perf_events/sigtrap_threads.c | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/compat.h b/include/linux/compat.h
> > index c8821d966812..f0d2dd35d408 100644
> > --- a/include/linux/compat.h
> > +++ b/include/linux/compat.h
> > @@ -237,7 +237,7 @@ typedef struct compat_siginfo {
> >                                       u32 _pkey;
> >                               } _addr_pkey;
> >                               /* used when si_code=TRAP_PERF */
> > -                             compat_u64 _perf;
> > +                             compat_ulong_t _perf;
> >                       };
> >               } _sigfault;
> >
> > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> > index d0bb9125c853..03d6f6d2c1fe 100644
> > --- a/include/uapi/asm-generic/siginfo.h
> > +++ b/include/uapi/asm-generic/siginfo.h
> > @@ -92,7 +92,7 @@ union __sifields {
> >                               __u32 _pkey;
> >                       } _addr_pkey;
> >                       /* used when si_code=TRAP_PERF */
> > -                     __u64 _perf;
> > +                     unsigned long _perf;
> >               };
> >       } _sigfault;
> >
> > diff --git a/tools/testing/selftests/perf_events/sigtrap_threads.c
> > b/tools/testing/selftests/perf_events/sigtrap_threads.c
> > index 9c0fd442da60..78ddf5e11625 100644
> > --- a/tools/testing/selftests/perf_events/sigtrap_threads.c
> > +++ b/tools/testing/selftests/perf_events/sigtrap_threads.c
> > @@ -44,7 +44,7 @@ static struct {
> >  } ctx;
> >
> >  /* Unique value to check si_perf is correctly set from perf_event_attr::sig_data. */
> > -#define TEST_SIG_DATA(addr) (~(uint64_t)(addr))
> > +#define TEST_SIG_DATA(addr) (~(unsigned long)(addr))
> >
> >  static struct perf_event_attr make_event_attr(bool enabled, volatile void *addr)
> >  {
> > --
> > 2.31.1.498.g6c1eba8ee3d-goog
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Re: [PATCH tip 1/2] signal, perf: Fix siginfo_t by avoiding u64 on 32-bit architectures

[Marco Elver]

On Thu, 22 Apr 2021 at 12:17, Marco Elver <elver@google.com> wrote:
> On Thu, 22 Apr 2021 at 11:48, David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Marco Elver
> > > Sent: 22 April 2021 07:45
> > >
> > > On some architectures, like Arm, the alignment of a structure is that of
> > > its largest member.
> >
> > That is true everywhere.
> > (Apart from obscure ABI where structure have at least 4 byte alignment!)
>
> For instance, x86 didn't complain, nor did m68k. Both of them have
> compile-time checks for the layout (I'm adding those for Arm
> elsewhere).
[...]
> > Much as I hate __packed, you could add __packed to the
> > definition of the structure member _perf.
> > The compiler will remove the padding before it and will
> > assume it has the alignment of the previous item.
> >
> > So it will never use byte accesses.
>
> Sure __packed works for Arm. But I think there's no precedent using
> this on siginfo_t, possibly for good reasons? I simply can't find
> evidence that this is portable on *all* architectures and for *all*
> possible definitions of siginfo_t, including those that live in things
> like glibc.
>
> Can we confirm that __packed is fine to add to siginfo_t on *all*
> architectures for *all* possible definitions of siginfo_t? I currently
> can't. And given it's outside the scope of the C standard (as of C11
> we got _Alignas, but that doesn't help I think), I'd vote to not
> venture too far for code that should be portable especially things as
> important as siginfo_t, and has definitions *outside* the kernel (I
> know we do lots of non-standard things, but others might not).

After thinking about this all afternoon, you convinced me that the
commit message wasn't great, and this should be in the commit message,
too: https://lkml.kernel.org/r/20210422191823.79012-1-elver@google.com

Thanks,
-- Marco