netdev - Re: [GIT] Networking

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]

Message-ID: <CA+55aFx1vbQWnQUVoieFgKrNECLKJs-RQi8z8cpe29g0aAiq3A@mail.gmail.com>
Date:   Sun, 5 Aug 2018 08:52:50 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     David Miller <davem@...emloft.net>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Network Development <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT] Networking

On Sun, Aug 5, 2018 at 12:47 AM David Miller <davem@...emloft.net> wrote:
>
> 4) Fix regression in netlink bind handling of multicast
>    gruops, from Dmitry Safonov.

This one is written kind of stupidly.

The code went from the original

        groups &= (1UL << nlk->ngroups) - 1;

(which is incorrect for large values of nlk->ngroups) to the fixed

        if (nlk->ngroups == 0)
                groups = 0;
        else if (nlk->ngroups < 8*sizeof(groups))
                groups &= (1UL << nlk->ngroups) - 1;

which isn't technically incorrect...

But it is stupid.

Why stupid? Because the test for 0 is pointless.

Just doing

        if (nlk->ngroups < 8*sizeof(groups))
                groups &= (1UL << nlk->ngroups) - 1;

would have been fine and more understandable, since the "mask by shift
count" already does the right thing for a ngroups value of 0. Now that
test for zero makes me go "what's special about zero?". It turns out
that the answer to that is "nothing".

I certainly didn't care enough to fix it up, and maybe the compiler is
even smart enough to remove the unnecessary test for zero, but it's
kind of sad to see this kind of "people didn't think the code through"
patch this late in the rc.

I'll be making an rc8 today anyway, but I did want to just point to it
and say "hey guys, the code is unnecessarily stupid and overly
complicated".

The type of "groups" is kind of silly too.

Yeah, "long unsigned int" isn't _technically_ wrong. But we normally
call that type "unsigned long".

And comparing against "8*sizeof(groups)" is misleading too, when the
actual masking expression works and is correct in "unsigned long"
because that's the type of the actual mask we're computing (due to the
"1UL").

So _regardless_ of the type of "groups" itself, the mask is actually
correct in unsigned long. I personally think it would have been more
legible as just

        unsigned long groups;
        ...
        if (nlk->ngroups < BITS_PER_LONG)
                groups &= (1UL << nlk->ngroups) - 1;

but by now I'm just nitpicking.

                  Linus