linux-kernel - Re: [PATCH] linux/bitmap.h: fix BITMAP_LAST_WORD

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:   Thu, 26 Jul 2018 18:15:59 +0800
From:   Wei Wang <wei.w.wang@...el.com>
To:     Yury Norov <ynorov@...iumnetworks.com>
CC:     linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
        corbet@....net, linux@...musvillemoes.dk, dgilbert@...hat.com,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH] linux/bitmap.h: fix BITMAP_LAST_WORD_MASK

On 07/26/2018 05:37 PM, Yury Norov wrote:
> On Thu, Jul 26, 2018 at 04:07:51PM +0800, Wei Wang wrote:
>> The existing BITMAP_LAST_WORD_MASK macro returns 0xffffffff if nbits is
>> 0. This patch changes the macro to return 0 when there is no bit needs to
>> be masked.
> I think this is intentional behavour. Previous version did return ~0UL
> explicitly in this case. See patch 89c1e79eb3023 (linux/bitmap.h: improve
> BITMAP_{LAST,FIRST}_WORD_MASK) from Rasmus.

Yes, I saw that. But it seems confusing for the corner case that nbits=0 
(no bits to mask), the macro returns with all the bits set.


>
> Introducing conditional branch would affect performance. All existing
> code checks nbits for 0 before handling last word where needed
> explicitly. So I think we'd better change nothing here.

I think that didn't save the conditional branch essentially, because 
it's just moved from inside this macro to the caller as you mentioned. 
If callers missed the check for some reason and passed 0 to the macro, 
they will get something unexpected.

Current callers like __bitmap_weight, __bitmap_equal, and others, they have

if (bits % BITS_PER_LONG)
     w += hweight_long(bitmap[k] & BITMAP_LAST_WORD_MASK(bits));

we could remove the "if" check by "w += hweight_long(bitmap[k] & 
BITMAP_LAST_WORD_MASK(bits % BITS_PER_LONG));" the branch is the same.


Best,
Wei