[PATCH v10 2/7] arch: move SA_* definitions to generic headers

[Dave Martin]

On Fri, Oct 02, 2020 at 06:14:01PM -0700, Peter Collingbourne wrote:
> On Tue, Sep 8, 2020 at 8:12 AM Dave Martin <Dave.Martin at arm.com> wrote:
> >
> > On Fri, Aug 21, 2020 at 10:10:12PM -0700, Peter Collingbourne wrote:
> > > Most architectures with the exception of alpha, mips, parisc and
> > > sparc use the same values for these flags. Move their definitions into
> > > asm-generic/signal-defs.h and allow the architectures with non-standard
> > > values to override them. Also, document the non-standard flag values
> > > in order to make it easier to add new generic flags in the future.
> > >
> > > Signed-off-by: Peter Collingbourne <pcc at google.com>
> >
> > While this looks reasonable, I've just realised that you strip the "U"
> > from some arches' definitions here.
> >
> > So, on powerpc and x86, this changes the type of flags other than
> > SA_RESETHAND from unsigned int to int.
> >
> > While I can't see this breaking any sensible use of these flags, there's
> > a chance that there is software relying on this distinction by
> > accident.
> 
> While it's true that it's technically possible that making these
> signed could change semantics, I'm having trouble seeing a realistic
> way in which software could be relying on this. Can you see one? I can
> think of cases like if the code does something like left shifts one of
> the flag bits into the sign bit (technically undefined behavior) and
> then right shifts it back (in C this would need to all be done in a
> single expression without storing to a variable; in C++ I suppose you
> could use auto to preserve the signedness in a variable's type). For
> example:
> 
> int x = (SA_NODEFER << 1) >> 1;
> 
> would give a different value to x if we made SA_NODEFER signed. But I
> wouldn't really expect software to be doing this sort of thing even
> accidentally, or much more than or'ing the flags together and
> assigning them to a variable, or passing them as a parameter, or some
> other operation which would fix the type.

I couldn't come up with a very good, non-contrived example I admit.

> I believe that the kernel's uapi guarantee applies at the binary
> level, not at the source level. If that were not the case, I think we
> would not be allowed to add any new declaration to an existing .h file
> for fear of conflicting with a user program's identically spelled
> declaration. And that seems more likely to me than software that would
> do this sort of thing.

The behaviour of source code making legitimate use of the kernel headers
still shouldn't be changed without good reason, especially if the change
could lead to subtle bugs that the compiler won't detect.

However this doesn't mean that decrufting unintentional inconsistencies
in the headers is a bad idea ... and see my response below.

> > I wonder whether it's worth doing something like
> >
> >         #ifdef ARCH_WANT_STRICTLY_UNSIGNED_SA_FLAGS
> >         #define __SA_FLAG_VAL(x) x ## U
> >         #else
> >         #define __SA_FLAG_VAL(x) x
> >         #endif
> >
> >         #ifndef SA_NOCLDSTOP
> >         #define SA_NOCLDSTOP __SA_FLAG_VAL(0x00000001)
> >         #endif
> >
> >         /* ... */
> 
> If we do this I would mildly prefer to keep the existing #defines in
> the arch-specific headers as if the arch had different flag values, as
> this would leave the arch-specific legacy cruft in the arch-specific
> headers where it belongs.
> 
> > Mind you, the historical situation also has issues, e.g. because
> > sa_flags in struct sigaction is an int, assigning
> >
> >         struct sigaction sa;
> >
> >         sa.sa_flags = SA_RESETHAND;
> >
> > implies an overflow and so isn't portably safe (at least in theory).  I
> > guess we are getting away with it today.  Preserving the situation by
> > keeping the "U"s where appropriate would at least avoid making the
> > situation worse.
> 
> I believe that the result of this assignment (involving an unsigned to
> signed conversion) is implementation defined and not undefined (which
> would be problematic). And in all the implementations that matter, as
> well as the C++ standard starting with C++20, this is a no-op cast
> assuming two's complement. I'm not sure what this has to do with
> making the constants signed because, as you pointed out, SA_RESETHAND
> would remain unsigned despite the absence of 'U' because its value
> does not fit in an int.


I generally agree with your analysis here, and yes, the de facto langue
does define the behaviour for signed integer overflow in practice, even
if the C standards demur on this issue.


I actually got my argument a little backwards: the problem here is not
that SA_RESETHAND unexpectedly becomes a (negative) int, but that the
other values become ints: but those nonetheless remain positive because
they aren't big enough to alias the sign bit.

So the main way for things to go wrong seems to be if the values are
shifted up, as you observe.  But (a) that is a strange thing to do, and
(b) there is an obvious need for a cast if doing things like:

#define ENCODE_SIGNAL(int sig, int flags) ((u64)(flags) << 8 | (sig))

Code lacking the cast would already be wrong, since SA_RESETHAND etc.
would simply get shifted off.


I think it's worth drawing attention to the issue in the commit message,
but if nobody else objects then I guess I am not too concerned about the
change.

Cheers
---Dave