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

[ebiederm@xmission.com (Eric W. Biederman)]

Marco Elver <elver@google.com> writes:

> On Mon, 3 May 2021 at 23:04, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> "Eric W. Beiderman" <ebiederm@xmission.com> writes:
>> > From: "Eric W. Biederman" <ebiederm@xmission.com>
>> >
>> > 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.


>
>> I decided to include this change because it is not that complicated,
>> and it allows si_perf_data to have the definition that was originally
>> desired.
>
> Neat, and long-term I think this seems to be worth having. Sooner or
> later something else might want __u64, too.
>
> But right now, due to the slight increase in complexity, we need to
> take extra care ensuring nothing broke -- at a high-level I see why
> this seems to be safe.

Absolutely.

>> If this looks difficult to make in the userspace definitions,
>> or is otherwise a problem I don't mind dropping this change.  I just
>> figured since it was not too difficult and we are changing things
>> anyway I should try for everything.
>
> Languages that support inheritance might end up with the simpler
> definition here (and depending on which fields they want to access,
> they'd have to cast the base siginfo into the one they want). What
> will become annoying is trying to describe siginfo_t.
>
> I will run some tests in the morning.
>
> Thanks,
> -- Marco
>
> [...]
>> > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
>> > index e663bf117b46..1fcede623a73 100644
>> > --- a/include/uapi/asm-generic/siginfo.h
>> > +++ b/include/uapi/asm-generic/siginfo.h
>> > @@ -29,15 +29,33 @@ typedef union sigval {
>> >  #define __ARCH_SI_ATTRIBUTES
>> >  #endif
>> >
>> > +#ifndef __ARCH_HAS_SWAPPED_SIGINFO
>> > +#define ___SIGINFO_COMMON    \
>> > +     int     si_signo;       \
>> > +     int     si_errno;       \
>> > +     int     si_code
>> > +#else
>> > +#define ___SIGINFO_COMMON    \
>> > +     int     si_signo;       \
>> > +     int     si_code;        \
>> > +     int     si_errno
>> > +#endif /* __ARCH_HAS_SWAPPED_SIGINFO */
>> > +
>> > +#define __SIGINFO_COMMON     \
>> > +     ___SIGINFO_COMMON;      \
>> > +     int     _common_pad[__alignof__(void *) != __alignof(int)]
>
> Just to verify my understanding of _common_pad: this is again a legacy
> problem, right? I.e. if siginfo was designed from the start like this,
> we wouldn't need the _common_pad.

Correct.

If we are designing things carefully from the start the code could have
had a 64bit or a 128bit header that was common to all siginfo instances.
Then the trick of moving the header into each union member would not
have been necessary.

Unfortunately no one noticed until it was much too late to do anything
about it.

> While this looks quite daunting, this is effectively turning siginfo
> from a struct with a giant union, into lots of smaller structs, each
> of which share a common header a'la inheritance -- until now the
> distinction didn't matter, but it starts to matter as soon as
> alignment of one child-struct would affect the offsets of another
> child-struct (i.e. the old version). Right?

Correct.

> I was wondering if it would make things look cleaner if the above was
> encapsulated in a struct, say __sicommon? Then the outermost union
> would use 'struct __sicommon _sicommon' and we need #defines for
> si_signo, si_code, and si_errno.
>
> Or would it break alignment somewhere?
>
> I leave it to your judgement -- just an idea.

I tried it and kernel/signal.c would not compile because a few functions
have a parameter named si_code.  The kernel I could fix but there
is a chance some user space application has similar issues.

So to be safe si_signo, si_code, si_errno have to be available without
having a define.

Eric