[GIT PULL] usercopy structs for v5.4-rc2

[GIT PULL] usercopy structs for v5.4-rc2

[Christian Brauner]

Hi Linus,

/* Summary */
This pull request contains the copy_struct_from_user() helper which got
split out from the openat2() patchset. It is a generic interface designed
to copy a struct from userspace.

The helper will be especially useful for structs versioned by size of which
we have quite a few. This allows for backwards compatibility, i.e. an
extended struct can be passed to an older kernel, or a legacy struct can be
passed to a newer kernel. For the first case (extended struct, older
kernel) the new fields in an extended struct can be set to zero and the
struct safely passed to an older kernel.

The most obvious benefit is that this helper lets us get rid of duplicate
code present in at least sched_setattr(), perf_event_open(), and clone3().
More importantly it will also help to ensure that users implementing
versioning-by-size end up with the same core semantics. This point is
especially crucial since we have at least one case where versioning-by-size
is used but with slighly different semantics: sched_setattr(),
perf_event_open(), and clone3() all do do similar checks to
copy_struct_from_user() while rt_sigprocmask(2) always rejects
differently-sized struct arguments.

With this pull request we also switch over sched_setattr(),
perf_event_open(), and clone3() to use the new helper.

I have carried these patches in a separate branch and taken care for them
to show up in linux-next. The only separate fix we we had to apply
was for a warning by clang when building the tests for using the result of
an assignment as a condition without parantheses. Given that this is a
reasonably sized change with proper testing it seemed ok to me to include
this in an rc2. If things break, we're around to fix them.

[1]: 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and robustify sched_read_attr() ABI logic and code")

The following changes since commit 54ecb8f7028c5eb3d740bb82b0f1d90f2df63c5c:

  Linux 5.4-rc1 (2019-09-30 10:35:40 -0700)

are available in the Git repository at:

  git@gitolite.kernel.org:pub/scm/linux/kernel/git/brauner/linux tags/copy-struct-from-user-v5.4-rc2

for you to fetch changes up to 341115822f8832f0c2d8af2f7e151c4c9a77bcd1:

  usercopy: Add parentheses around assignment in test_copy_struct_from_user (2019-10-03 21:13:27 +0200)

/* Testing */
All patches have seen exposure in linux-next and are based on v5.4-rc1. The
copy_struct_from_user() helper comes with selftests.

/* Conflicts */
At the time of creating this PR no merge conflicts were reported from
linux-next.

Please consider pulling these changes from the signed
copy-struct-from-user-v5.4-rc2 tag.

Thanks!
Christian

----------------------------------------------------------------
copy-struct-from-user-v5.4-rc2

----------------------------------------------------------------
Aleksa Sarai (4):
      lib: introduce copy_struct_from_user() helper
      clone3: switch to copy_struct_from_user()
      sched_setattr: switch to copy_struct_from_user()
      perf_event_open: switch to copy_struct_from_user()

Nathan Chancellor (1):
      usercopy: Add parentheses around assignment in test_copy_struct_from_user

 include/linux/bitops.h     |   7 +++
 include/linux/uaccess.h    |  70 +++++++++++++++++++++++
 include/uapi/linux/sched.h |   2 +
 kernel/events/core.c       |  47 +++-------------
 kernel/fork.c              |  34 +++---------
 kernel/sched/core.c        |  43 +++-----------
 lib/strnlen_user.c         |   8 +--
 lib/test_user_copy.c       | 136 +++++++++++++++++++++++++++++++++++++++++++--
 lib/usercopy.c             |  55 ++++++++++++++++++
 9 files changed, 288 insertions(+), 114 deletions(-)

Re: [GIT PULL] usercopy structs for v5.4-rc2

[Linus Torvalds]

On Fri, Oct 4, 2019 at 3:42 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
>            The only separate fix we we had to apply
> was for a warning by clang when building the tests for using the result of
> an assignment as a condition without parantheses.

Hmm. That code is ugly, both before and after the fix.

This just doesn't make sense for so many reasons:

        if ((ret |= test(umem_src == NULL, "kmalloc failed")))

where the insanity comes from

 - why "|=" when you know that "ret" was zero before (and it had to
be, for the test to make sense)

 - why do this as a single line anyway?

 - don't do the stupid "double parenthesis" to hide a warning. Make it
use an actual comparison if you add a layer of parentheses.

So

        if ((x = y))

is *wrong*. I know the compiler suggests that, but the compiler is
just being stupid, and the suggestion comes from people who don't have
any taste.

If you want to test an assignment, you should just use

        if ((x = y) != 0)

instead, at which point it's not syntactic noise mind-games any more,
but the parenthesis actually make sense.

However, you had no reason to use an assignment in the conditional in
the first place.

IOW, the code should have just been

        ret = test(umem_src == NULL, "kmalloc failed");
        if (ret) ...

instead. Which is a whole lot more legible.

The alternative, of course, is to just ignore the return value of
"test()" which is useless anyway (it's a boolean, not an error) and
just write it as

        if (test(umem_src == NULL, "kmalloc failed"))
                goto out_free;

and set ret to the error value ahead of time.

Regardless, the double parentheses are _never_ the right answer. Ugly,
broken, senseless syntax that is hard for humans to read, and doesn't
make any sense even for computers when there's a perfectly regular
alternative that isn't a random special case.

I've pulled this, since it's not in core kernel code anyway, but I
wish I had never had to see that ugly construct.

             Linus

Re: [GIT PULL] usercopy structs for v5.4-rc2

[pr-tracker-bot]

The pull request you sent on Fri,  4 Oct 2019 12:41:16 +0200:

> git@gitolite.kernel.org:pub/scm/linux/kernel/git/brauner/linux tags/copy-struct-from-user-v5.4-rc2

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e524d16e7e324039f2a9f82e302f0a39ac7d5812

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT PULL] usercopy structs for v5.4-rc2

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

On 2019-10-04, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Fri, Oct 4, 2019 at 3:42 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> >            The only separate fix we we had to apply
> > was for a warning by clang when building the tests for using the result of
> > an assignment as a condition without parantheses.
> 
> Hmm. That code is ugly, both before and after the fix.
> 
> This just doesn't make sense for so many reasons:
> 
>         if ((ret |= test(umem_src == NULL, "kmalloc failed")))
> 
> where the insanity comes from
> 
>  - why "|=" when you know that "ret" was zero before (and it had to
> be, for the test to make sense)
> 
>  - why do this as a single line anyway?
> 
>  - don't do the stupid "double parenthesis" to hide a warning. Make it
> use an actual comparison if you add a layer of parentheses.

You're quite right -- I was mindlessly copying the "ret |=" logic the
rest of test_user_copy.c does without thinking about it. I'll include a
cleanup for it in the openat2(2) series.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [GIT PULL] usercopy structs for v5.4-rc2

[Nathan Chancellor]

On Fri, Oct 04, 2019 at 10:53:41AM -0700, Linus Torvalds wrote:
> On Fri, Oct 4, 2019 at 3:42 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> >            The only separate fix we we had to apply
> > was for a warning by clang when building the tests for using the result of
> > an assignment as a condition without parantheses.
> 
> Hmm. That code is ugly, both before and after the fix.
> 
> This just doesn't make sense for so many reasons:
> 
>         if ((ret |= test(umem_src == NULL, "kmalloc failed")))
> 
> where the insanity comes from
> 
>  - why "|=" when you know that "ret" was zero before (and it had to
> be, for the test to make sense)
> 
>  - why do this as a single line anyway?
> 
>  - don't do the stupid "double parenthesis" to hide a warning. Make it
> use an actual comparison if you add a layer of parentheses.
> 
> So
> 
>         if ((x = y))
> 
> is *wrong*. I know the compiler suggests that, but the compiler is
> just being stupid, and the suggestion comes from people who don't have
> any taste.
> 
> If you want to test an assignment, you should just use
> 
>         if ((x = y) != 0)
> 
> instead, at which point it's not syntactic noise mind-games any more,
> but the parenthesis actually make sense.
> 
> However, you had no reason to use an assignment in the conditional in
> the first place.
> 
> IOW, the code should have just been
> 
>         ret = test(umem_src == NULL, "kmalloc failed");
>         if (ret) ...

Yes, I had this as the original fix but I tried to keep the same
intention as the original author. I should have gone with my gut. Sorry
for the ugliness, I'll try to be better in the future.

Cheers,
Nathan

Re: [GIT PULL] usercopy structs for v5.4-rc2

[Christian Brauner]

On Fri, Oct 04, 2019 at 10:53:41AM -0700, Linus Torvalds wrote:
> On Fri, Oct 4, 2019 at 3:42 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> >            The only separate fix we we had to apply
> > was for a warning by clang when building the tests for using the result of
> > an assignment as a condition without parantheses.

<snip>

> 
> I've pulled this, since it's not in core kernel code anyway, but I
> wish I had never had to see that ugly construct.

I admit that I'm _less_ anal about other people's code in _tests_ as for
example in this case.
For core kernel code I obviously would not have taken this into the
tree.
Honestly, my main concern currently is to get people to add tests at all
- at least if they want to go through my tree - and if they adhere to
that request I'm more lenient.

Christian

RE: [GIT PULL] usercopy structs for v5.4-rc2

[David Laight]

From: Nathan Chancellor
> Sent: 04 October 2019 20:44
...
> > IOW, the code should have just been
> >
> >         ret = test(umem_src == NULL, "kmalloc failed");
> >         if (ret) ...
> 
> Yes, I had this as the original fix but I tried to keep the same
> intention as the original author. I should have gone with my gut. Sorry
> for the ugliness, I'll try to be better in the future.

This rather begs the question about why 'usercopy' is ever calling kmalloc() at all.
Never mind some perverted style for reporting errors.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [GIT PULL] usercopy structs for v5.4-rc2

[Al Viro]

On Mon, Oct 07, 2019 at 01:11:46PM +0000, David Laight wrote:
> From: Nathan Chancellor
> > Sent: 04 October 2019 20:44
> ...
> > > IOW, the code should have just been
> > >
> > >         ret = test(umem_src == NULL, "kmalloc failed");
> > >         if (ret) ...
> > 
> > Yes, I had this as the original fix but I tried to keep the same
> > intention as the original author. I should have gone with my gut. Sorry
> > for the ugliness, I'll try to be better in the future.
> 
> This rather begs the question about why 'usercopy' is ever calling kmalloc() at all.

Do you even bother to read what you are commenting upon, or is it simply the
irresistable pleasure of being seen[*]?

When a function called 'test_copy_struct_from_user' starts with a couple of
allocations, one called 'umem_src' and another - 'expected', what could that
possibly be about?  Something to do with testing copy_struct_from_user(),
perhaps?  And, taking a wild guess, maybe allocating a buffer or two to
be somehow used in setting the test up?

Or you could just go and read the damn function, you twit.

[*] sensu Monty Python, if we are lucky enough