Re: [PATCH v3 04/12] signal: Verify the alignment and size of siginfo_t
From: Marco Elver <elver@google.com>
Date: 2021-05-05 17:53:40
Also in:
linux-api, lkml, sparclinux
On Wed, 5 May 2021 at 16:11, Eric W. Beiderman [off-list ref] wrote:
From: "Eric W. Biederman" <redacted> Update the static assertions about siginfo_t to also describe it's alignment and size. While investigating if it was possible to add a 64bit field into siginfo_t[1] it became apparent that the alignment of siginfo_t is as much a part of the ABI as the size of the structure. If the alignment changes siginfo_t when embedded in another structure can move to a different offset. Which is not acceptable from an ABI structure. So document that fact and add static assertions to notify developers if they change change the alignment by accident. [1] https://lkml.kernel.org/r/YJEZdhe6JGFNYlum@elver.google.com Signed-off-by: "Eric W. Biederman" <redacted>
Acked-by: Marco Elver <elver@google.com>
quoted hunk ↗ jump to hunk
--- arch/arm/kernel/signal.c | 2 ++ arch/arm64/kernel/signal.c | 2 ++ arch/arm64/kernel/signal32.c | 2 ++ arch/sparc/kernel/signal32.c | 2 ++ arch/sparc/kernel/signal_64.c | 2 ++ arch/x86/kernel/signal_compat.c | 6 ++++++ include/uapi/asm-generic/siginfo.h | 5 +++++ 7 files changed, 21 insertions(+)diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c index 2dac5d2c5cf6..643bcb0f091b 100644 --- a/arch/arm/kernel/signal.c +++ b/arch/arm/kernel/signal.c@@ -737,6 +737,8 @@ static_assert(NSIGBUS == 5); static_assert(NSIGTRAP == 6); static_assert(NSIGCHLD == 6); static_assert(NSIGSYS == 2); +static_assert(sizeof(siginfo_t) == 128); +static_assert(__alignof__(siginfo_t) == 4); static_assert(offsetof(siginfo_t, si_signo) == 0x00); static_assert(offsetof(siginfo_t, si_errno) == 0x04); static_assert(offsetof(siginfo_t, si_code) == 0x08);diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index af8bd2af1298..ad4bd27fc044 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c@@ -985,6 +985,8 @@ static_assert(NSIGBUS == 5); static_assert(NSIGTRAP == 6); static_assert(NSIGCHLD == 6); static_assert(NSIGSYS == 2); +static_assert(sizeof(siginfo_t) == 128);
Would using SI_MAX_SIZE be appropriate? Perhaps not.. in case somebody changes it, given these static asserts are meant to double-check. I leave it to you to decide what makes more sense.
quoted hunk ↗ jump to hunk
+static_assert(__alignof__(siginfo_t) == 8); static_assert(offsetof(siginfo_t, si_signo) == 0x00); static_assert(offsetof(siginfo_t, si_errno) == 0x04); static_assert(offsetof(siginfo_t, si_code) == 0x08);diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c index b6afb646515f..ee6c7484e130 100644 --- a/arch/arm64/kernel/signal32.c +++ b/arch/arm64/kernel/signal32.c@@ -469,6 +469,8 @@ static_assert(NSIGBUS == 5); static_assert(NSIGTRAP == 6); static_assert(NSIGCHLD == 6); static_assert(NSIGSYS == 2); +static_assert(sizeof(compat_siginfo_t) == 128); +static_assert(__alignof__(compat_siginfo_t) == 4); static_assert(offsetof(compat_siginfo_t, si_signo) == 0x00); static_assert(offsetof(compat_siginfo_t, si_errno) == 0x04); static_assert(offsetof(compat_siginfo_t, si_code) == 0x08);diff --git a/arch/sparc/kernel/signal32.c b/arch/sparc/kernel/signal32.c index 778ed5c26d4a..32b977f253e3 100644 --- a/arch/sparc/kernel/signal32.c +++ b/arch/sparc/kernel/signal32.c@@ -757,6 +757,8 @@ static_assert(NSIGBUS == 5); static_assert(NSIGTRAP == 6); static_assert(NSIGCHLD == 6); static_assert(NSIGSYS == 2); +static_assert(sizeof(compat_siginfo_t) == 128); +static_assert(__alignof__(compat_siginfo_t) == 4); static_assert(offsetof(compat_siginfo_t, si_signo) == 0x00); static_assert(offsetof(compat_siginfo_t, si_errno) == 0x04); static_assert(offsetof(compat_siginfo_t, si_code) == 0x08);diff --git a/arch/sparc/kernel/signal_64.c b/arch/sparc/kernel/signal_64.c index c9bbf5f29078..e9dda9db156c 100644 --- a/arch/sparc/kernel/signal_64.c +++ b/arch/sparc/kernel/signal_64.c@@ -567,6 +567,8 @@ static_assert(NSIGBUS == 5); static_assert(NSIGTRAP == 6); static_assert(NSIGCHLD == 6); static_assert(NSIGSYS == 2); +static_assert(sizeof(siginfo_t) == 128); +static_assert(__alignof__(siginfo_t) == 8); static_assert(offsetof(siginfo_t, si_signo) == 0x00); static_assert(offsetof(siginfo_t, si_errno) == 0x04); static_assert(offsetof(siginfo_t, si_code) == 0x08);diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c index 0e5d0a7e203b..e735bc129331 100644 --- a/arch/x86/kernel/signal_compat.c +++ b/arch/x86/kernel/signal_compat.c@@ -34,7 +34,13 @@ static inline void signal_compat_build_tests(void) BUILD_BUG_ON(NSIGSYS != 2); /* This is part of the ABI and can never change in size: */ + BUILD_BUG_ON(sizeof(siginfo_t) != 128); BUILD_BUG_ON(sizeof(compat_siginfo_t) != 128); + + /* This is a part of the ABI and can never change in alignment */ + BUILD_BUG_ON(__alignof__(siginfo_t) != 8); + BUILD_BUG_ON(__alignof__(compat_siginfo_t) != 4); + /* * The offsets of all the (unioned) si_fields are fixed * in the ABI, of course. Make sure none of them everdiff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h index 03d6f6d2c1fe..91c80d0c10c5 100644 --- a/include/uapi/asm-generic/siginfo.h +++ b/include/uapi/asm-generic/siginfo.h@@ -29,6 +29,11 @@ typedef union sigval { #define __ARCH_SI_ATTRIBUTES #endif +/* + * Be careful when extending this union. On 32bit siginfo_t is 32bit + * aligned. Which means that a 64bit field or any other field that + * would increase the alignment of siginfo_t will break the ABI. + */ union __sifields { /* kill() */ struct { --2.30.1