Re: [PATCH 10/12] signal: Redefine signinfo so 64bit fields are possible

Marco Elver [off-list ref] writes:

On Mon, May 03, 2021 at 09:03PM -0700, Peter Collingbourne wrote:

quoted

On Mon, May 3, 2021 at 8:42 PM Eric W. Biederman [off-list ref] wrote:

quoted

Marco Elver [off-list ref] writes:

quoted

On Mon, 3 May 2021 at 23:04, Eric W. Biederman [off-list ref] wrote:

quoted

"Eric W. Beiderman" [off-list ref] writes:

quoted

From: "Eric W. Biederman" <redacted>

The si_perf code really wants to add a u64 field.  This change enables
that by reorganizing the definition of siginfo_t, so that a 64bit
field can be added without increasing the alignment of other fields.

If you can, it'd be good to have an explanation for this, because it's
not at all obvious -- some future archeologist will wonder how we ever
came up with this definition of siginfo...

(I see the trick here is that before the union would have changed
alignment, introducing padding after the 3 ints -- but now because the
3 ints are inside the union the union's padding no longer adds padding
for these ints.  Perhaps you can explain it better than I can. Also
see below.)

Yes.  The big idea is adding a 64bit field into the second union
in the _sigfault case will increase the alignment of that second
union to 64bit.

In the 64bit case the alignment is already 64bit so it is not an
issue.

In the 32bit case there are 3 ints followed by a pointer.  When the
64bit member is added the alignment of _segfault becomes 64bit.  That
64bit alignment after 3 ints changes the location of the 32bit pointer.

By moving the 3 preceding ints into _segfault that does not happen.



There remains one very subtle issue that I think isn't a problem
but I would appreciate someone else double checking me.


The old definition of siginfo_t on 32bit almost certainly had 32bit
alignment.  With the addition of a 64bit member siginfo_t gains 64bit
alignment.  This difference only matters if the 64bit field is accessed.
Accessing a 64bit field with 32bit alignment will cause unaligned access
exceptions on some (most?) architectures.

For the 64bit field to be accessed the code needs to be recompiled with
the new headers.  Which implies that when everything is recompiled
siginfo_t will become 64bit aligned.


So the change should be safe unless someone is casting something with
32bit alignment into siginfo_t.

How about if someone has a field of type siginfo_t as an element of a
struct? For example:

struct foo {
  int x;
  siginfo_t y;
};

With this change wouldn't the y field move from offset 4 to offset 8?

This is a problem if such a struct is part of the ABI -- in the kernel I
found these that might be problematic:

| arch/csky/kernel/signal.c:struct rt_sigframe {
| arch/csky/kernel/signal.c-	/*
| arch/csky/kernel/signal.c-	 * pad[3] is compatible with the same struct defined in
| arch/csky/kernel/signal.c-	 * gcc/libgcc/config/csky/linux-unwind.h
| arch/csky/kernel/signal.c-	 */
| arch/csky/kernel/signal.c-	int pad[3];
| arch/csky/kernel/signal.c-	struct siginfo info;
| arch/csky/kernel/signal.c-	struct ucontext uc;
| arch/csky/kernel/signal.c-};
| [...]
| arch/parisc/include/asm/rt_sigframe.h-#define SIGRETURN_TRAMP 4
| arch/parisc/include/asm/rt_sigframe.h-#define SIGRESTARTBLOCK_TRAMP 5 
| arch/parisc/include/asm/rt_sigframe.h-#define TRAMP_SIZE (SIGRETURN_TRAMP + SIGRESTARTBLOCK_TRAMP)
| arch/parisc/include/asm/rt_sigframe.h-
| arch/parisc/include/asm/rt_sigframe.h:struct rt_sigframe {
| arch/parisc/include/asm/rt_sigframe.h-	/* XXX: Must match trampoline size in arch/parisc/kernel/signal.c 
| arch/parisc/include/asm/rt_sigframe.h-	        Secondary to that it must protect the ERESTART_RESTARTBLOCK
| arch/parisc/include/asm/rt_sigframe.h-		trampoline we left on the stack (we were bad and didn't 
| arch/parisc/include/asm/rt_sigframe.h-		change sp so we could run really fast.) */
| arch/parisc/include/asm/rt_sigframe.h-	unsigned int tramp[TRAMP_SIZE];
| arch/parisc/include/asm/rt_sigframe.h-	struct siginfo info;
| [..]
| arch/parisc/kernel/signal32.h-#define COMPAT_SIGRETURN_TRAMP 4
| arch/parisc/kernel/signal32.h-#define COMPAT_SIGRESTARTBLOCK_TRAMP 5
| arch/parisc/kernel/signal32.h-#define COMPAT_TRAMP_SIZE (COMPAT_SIGRETURN_TRAMP + \
| arch/parisc/kernel/signal32.h-				COMPAT_SIGRESTARTBLOCK_TRAMP)
| arch/parisc/kernel/signal32.h-
| arch/parisc/kernel/signal32.h:struct compat_rt_sigframe {
| arch/parisc/kernel/signal32.h-        /* XXX: Must match trampoline size in arch/parisc/kernel/signal.c
| arch/parisc/kernel/signal32.h-                Secondary to that it must protect the ERESTART_RESTARTBLOCK
| arch/parisc/kernel/signal32.h-                trampoline we left on the stack (we were bad and didn't
| arch/parisc/kernel/signal32.h-                change sp so we could run really fast.) */
| arch/parisc/kernel/signal32.h-        compat_uint_t tramp[COMPAT_TRAMP_SIZE];
| arch/parisc/kernel/signal32.h-        compat_siginfo_t info;

Adding these static asserts to parisc shows the problem:

| diff --git a/arch/parisc/kernel/signal.c b/arch/parisc/kernel/signal.c
| index fb1e94a3982b..0be582fb81be 100644
| --- a/arch/parisc/kernel/signal.c
| +++ b/arch/parisc/kernel/signal.c
| @@ -610,3 +610,6 @@ void do_notify_resume(struct pt_regs *regs, long in_syscall)
|  	if (test_thread_flag(TIF_NOTIFY_RESUME))
|  		tracehook_notify_resume(regs);
|  }
| +
| +static_assert(sizeof(unsigned long) == 4); // 32 bit build
| +static_assert(offsetof(struct rt_sigframe, info) == 9 * 4);

This passes without the siginfo rework in this patch. With it:

| ./include/linux/build_bug.h:78:41: error: static assertion failed: "offsetof(struct rt_sigframe, info) == 9 * 4"

As sad as it is, I don't think we can have our cake and eat it, too. :-(

Unless you see why this is fine, I think we need to drop this patch and
go back to the simpler version you had.

No.  I really can't.  I think we are stuck with 32bit alignment on 32bit
architectures at this point.  Which precludes 32bit architectures from
including a 64bit field.

The variant of this that concerns me the most is siginfo_t embedded in a
structure in a library combined with code that is compiled with new
headers.  The offset of the embedded siginfo_t could very easily change
and break things.

That makes the alignment an ABI property we can't mess with.  Shame.

I will figure out some static asserts to verify this property remains
on 32bit and respin this series.

Eric