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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 4 Oct 2019 10:53:41 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Christian Brauner <christian.brauner@...ntu.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] usercopy structs for v5.4-rc2

On Fri, Oct 4, 2019 at 3:42 AM Christian Brauner
<christian.brauner@...ntu.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