Re: [PATCH 4/8] waitid(2): leave copyout of siginfo to syscall itself

[Eric W. Biederman]

Al Viro <viro@xxxxxxxxxxxxxxxxxx> writes:

> On Mon, May 15, 2017 at 04:06:49PM -0700, Linus Torvalds wrote:
>> On Mon, May 15, 2017 at 3:37 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>> >
>> > +struct waitid_info {
>> > +       pid_t pid;
>> > +       uid_t uid;
>> > +       int status;
>> > +       int why;
>> > +};
>> 
>> Ugh. Could we please just name those with what they are actually used for?
>> 
>> Even if you hate the "si_" previx for some reason, I really don't see
>> why we'd continue call it "why", when it's written to "si_code"
>> 
>> Yes, yes, I see the historical reason, and how "si_code" is just the
>> low 16 bits of "why", and the high 16 bits is something else.
>
> __SI_CHLD, and AFAICS it only matters for copy_siginfo_to_user() and its
> relatives - basically, "how much of kernel-side struct siginfo do we have
> initialized"...
>
>> But now that there is a structure for that, could we not just make
>> that explicit in the structure instead? Those games with "why" look
>> really odd.
>
> OK...
>
>> So I can see why you'd like to keep this patch as "minimal
>> conversion", but it would be really nice to have a followup patch that
>> gets rid of the odd "why" games.
>
> The thing is, we lack convenient defines for those constants.  We could
> turn this "why" thing into u16 si_code, but then gcc will scream about
> integer constant truncation ;-/  Suggestions?

Hmm.  Given the small size of that thing I think I would just
include embed waidtid_info in wait_opts as you have done earlier with wo_stat;

Which would mean at the end of do_wait you can just do:

	wo->wo_info.si_code &= 0xffff;

Neither SI_TIMER nor SI_MESGQ are being used so sign extension
is not needed.

As for getting magic out of the upper bits of si_code I suspect
we can just switch on si_signo and then for the realtime signals
si_code to get the layout in copy_siginfo_to_user.

The compiler just uses a binary tree of jumps so I don't expect the code
generated for copy_signinfo_to_user would be much worse and I do expect
with the magic taken out the logic of the code would be easier to
understand.

Alternately we could use some kind of ksiginfo that does not take up 128
bytes (except in the case of rt_sigqueueinfo) and being kernel internal
only has a format that is easy to decode for copy_siginfo_to_user.

> BTW, I wonder if making those stores conditional is actually a win -
> sure, for put_user() it used to be, but for plain stores...  Not sure.

My guess would be that storing to a small structure on the stack would
be almost free, and with all of the fields unconditionally present in
struct wait_opts the code could compute the values directly into wo
without needing intermediate varriables.    Which ought to make
the code easier to understand and maintain if not cycle by cycle faster.

Eric