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] [day] [month] [year] [list]

Message-ID: <CAJwJo6a7-p_WO4oUTC3bGMB_APZR2dEYV+TvgAcXeFjYgfLhqQ@mail.gmail.com>
Date:   Tue, 7 Aug 2018 18:56:00 +0100
From:   Dmitry Safonov <0x7f454c46@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     David Miller <davem@...emloft.net>,
        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

Hi Linus,

2018-08-05 16:52 GMT+01:00 Linus Torvalds <torvalds@...ux-foundation.org>:
> 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.

Heh, I've been stupid enough at that moment to think that
(1 << 0 == 1) and forgetting that I'm subtracting 1 for mask.

> 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.

Yes, sorry.

> 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.

I'll prepare the cleanup for linux-next.

Sorry about the stupid code,
             Dmitry