Messages in this thread | | | From | David Laight <> | Subject | RE: [PATCH tip 1/2] signal, perf: Fix siginfo_t by avoiding u64 on 32-bit architectures | Date | Thu, 22 Apr 2021 09:48:42 +0000 |
| |
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)
| |