linux-kernel - Re: [PATCH 1/2] IB/hfi1: Fix a parameter of find_first

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, 2 Sep 2016 10:39:11 -0400
From:   Doug Ledford <dledford@...hat.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        mike.marciniszyn@...el.com, dennis.dalessandro@...el.com,
        sean.hefty@...el.com, hal.rosenstock@...il.com,
        linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit.

On 8/28/2016 2:06 AM, Leon Romanovsky wrote:
> On Fri, Aug 26, 2016 at 03:34:48PM -0400, Doug Ledford wrote:
>> On 8/26/2016 3:29 PM, Leon Romanovsky wrote:
>>> On Fri, Aug 26, 2016 at 02:01:55PM -0400, Doug Ledford wrote:
>>>> On 8/26/2016 9:35 AM, Doug Ledford wrote:
>>>>> On 8/26/2016 12:49 AM, Christophe JAILLET wrote:
>>>>>> The 2nd parameter of 'find_first_bit' is the number of bits to search.
>>>>>> In this case, we are passing 'sizeof(unsigned long)' which is likely to
>>>>>> be 4 or 8.
>>>>>
>>>>> If the size can be 4 or 8, then using 64 universally is not correct.
>>>>> Why not use sizeof() * 8 (or << 3)?
>>>>
>>>> Better yet, why not put this patch in the kernel first:
>>>>
>>>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>>>> index d96a6118d26a..a8838c87668e 100644
>>>> --- a/include/linux/kernel.h
>>>> +++ b/include/linux/kernel.h
>>>> @@ -52,6 +52,8 @@
>>>>
>>>>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
>>>> __must_be_array(arr))
>>>>
>>>> +#define bitsizeof(x)   (sizeof((x)) << 3)
>>>> +
>>>>  #define u64_to_user_ptr(x) (           \
>>>>  {                                      \
>>>>         typecheck(u64, x);              \
>>>>
>>>> then start going around replacing all these hard coded numbers with the
>>>> use of bitsizeof().  It can be applied not just to the find_first*bit()
>>>> routines, but to a bunch of other routines too.  Just look at
>>>> include/linux/bitmap.h and any that have nbits as an argument are
>>>> candidates.
>>>
>>> There is BITS_PER_LONG define for that. There is actual use of it in mlx5 for
>>> the similar code pieces.
>>
>> BITS_PER_LONG only works if your bitfield is a single long.  It doesn't
>> work for other bitfields.  What I posted above will work for anything.
> 
> Yes, the question to ask if it is really needed.

A quick review of the usage of find_first_bit in the kernel shows that
the majority of uses that use large, complex bit arrays (things larger
than a single long, where bitsizeof(complex_object) might be helpful),
also tend to have limits to their bitmap sizes that do not directly
equal the size of the bitmap.  For example, the bitmap for an md raid
array is allocated as a chunk of memory, where the chunk of memory is
larger than the bitmap size as a general rule, so
bitsizeof(chunk_of_memory) would run off the end of the real bitmap.
Likewise for a number of other complex bitmaps (block layer tag in use
bitmap for instance).

So, is it useful?  I think so.  But it's not needed for original patch
in this thread.


-- 
Doug Ledford <dledford@...hat.com>
    GPG Key ID: 0E572FDD



Download attachment "signature.asc" of type "application/pgp-signature" (885 bytes)