Message-ID: <CA+55aFw1gmfFH1o+oWMo4TnU5hEczAOiY1g=eMxqYZmy9JqDYw@mail.gmail.com>
Date:   Sun, 15 Apr 2018 11:16:04 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Dave Martin <Dave.Martin@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Dmitry V. Levin" <ldv@...linux.org>,
        sparclinux <sparclinux@...r.kernel.org>,
        ppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        linux-arch <linux-arch@...r.kernel.org>
Subject: Re: [RFC PATCH 0/3] Dealing with the aliases of SI_USER

(

On Sun, Apr 15, 2018 at 8:56 AM, Eric W. Biederman
<ebiederm@...ssion.com> wrote:
>
> Would you consider the patchset below for -rc2?

Ugh.

I have an irrational dislike of "clear_siginfo()". It's a nasty broken
interface, which just mis-spells "memset()", and makes it non-obvious
that you could clear it other ways too (eg "kzalloc()" or whatever).

And this series brings in a lot of new users.

Honestly, both "clear_siginfo()" and "copy_siginfo()" are crazy
interfaces. They may have made sense when converting code, but not so
much now.

If we want to have a function that initializes a siginfo, I think it
should _actually_ initialize it, and at least take the two fields that
a siginfo has to have to be valid: si_signo and si_code.

So I'd rather see a patch that does something like this:

  -             clear_siginfo(info);
  -             info->si_signo = sig;
  -             info->si_errno = 0;
  -             info->si_code = SI_USER;
  -             info->si_pid = 0;
  -             info->si_uid = 0;
  +             init_siginfo(info, sig, SI_USER);

which at least makes that function be *worth* something, not just a
bad spelling of memset. It's not just the removal of pointless "set to
zero", it's the whole concept of "this function actually makes a valid
siginfo", which is lacking in the existing function.

(Yeah, yeah, si_errno is "generic" too and always part of a siginfo,
but nobody cares. It's pretty much always set to zero, it would be
stupid to add that interface to the "init_siginfo()" function. So just
clearing it is fine, the one or two places that want to set it to some
silly value can do it themselves).

Then your series would incidentally also:

 (a) make for fewer lines overall, rather than add lines

 (b) make it clear that we always initialize si_code, which now *must*
be a valid value with all the recent siginfo changes.

Hmm?

The other thing we should do is to get rid of the stupid padding.
Right now "struct siginfo" is pointlessly padded to 128 bytes. That is
completely insane, when it's always just zero in the kernel.

So put that _pad[] thing inside #ifndef __KERNEL__, and make
copy_siginfo_to_user() write the padding zeroes when copying to user
space. The reason for the padding is "future expansion", so we do want
to tell the user space that it's maybe up to 128 bytes in size, but if
we don't fill it all, we shouldn't waste time and memory on clearing
the padding internally.

I'm certainly *hoping* nobody depends on the whole 128 bytes in
rt_sigqueueinfo(). In theory you can fill it all (as long as si_code
is negative), but the man-page only says "si_value", and the compat
function doesn't copy any more than that either, so any user that
tries to fill in more than si_value is already broken. In fact, it
might even be worth enforcing that in rt_sigqueueinfo(), just to see
if anybody plays any games..

On x86-64, without the pointless padding, the size of 'struct siginfo'
inside the kernel would be 48 bytes. That's quite a big difference for
something that is often allocated on the kernel stack.

So I'm certainly willing to make those kinds of changes, but let's
make them real *improvements* now, ok? Wasn't that the point of all
the cleanups in the end?

                       Linus

Powered by blists - more mailing lists