linux-kernel - Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

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, 3 Dec 2021 16:04:11 -0500
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Joe Perches <joe@...ches.com>, Juergen Gross <jgross@...e.com>,
        sstabellini@...nel.org, roger.pau@...rix.com, axboe@...nel.dk
Cc:     xen-devel@...ts.xenproject.org, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] xen-blkfront: Use the bitmap API when applicable


On 12/3/21 10:54 AM, Christophe JAILLET wrote:
> Le 03/12/2021 à 04:03, Joe Perches a écrit :
>> On Thu, 2021-12-02 at 20:07 +0100, Christophe JAILLET wrote:
>>> Le 02/12/2021 à 19:16, Joe Perches a écrit :
>>>> On Thu, 2021-12-02 at 19:12 +0100, Christophe JAILLET wrote:
>>>>> Le 02/12/2021 à 07:12, Juergen Gross a écrit :
>>>>>> On 01.12.21 22:10, Christophe JAILLET wrote:
>>>>>>> Use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid
>>>>>>> some open-coded arithmetic in allocator arguments.
>>>>>>>
>>>>>>> Also change the corresponding 'kfree()' into 'bitmap_free()' to keep
>>>>>>> consistency.
>>>>>>>
>>>>>>> Use 'bitmap_copy()' to avoid an explicit 'memcpy()'
>>>> []
>>>>>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>>> []
>>>>>>> @@ -442,16 +442,14 @@ static int xlbd_reserve_minors(unsigned int
>>>>>>> minor, unsigned int nr)
>>>>>>>         if (end > nr_minors) {
>>>>>>>             unsigned long *bitmap, *old;
>>>>>>> -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
>>>>>>> -                 GFP_KERNEL);
>>>>>>> +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
>>>>>>>             if (bitmap == NULL)
>>>>>>>                 return -ENOMEM;
>>>>>>>             spin_lock(&minor_lock);
>>>>>>>             if (end > nr_minors) {
>>>>>>>                 old = minors;
>>>>>>> -            memcpy(bitmap, minors,
>>>>>>> -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
>>>>>>> +            bitmap_copy(bitmap, minors, nr_minors);
>>>>>>>                 minors = bitmap;
>>>>>>>                 nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
>>>>
>>>>         nr_minors = end;
>>>> ?
>>>>
>>>
>>> No,
>>> My understanding of the code is that if we lack space (end > nr_minors),
>>> we need to allocate more. In such a case, we want to keep track of what
>>> we have allocated, not what we needed.
>>> The "padding" bits in the "long align" allocation, can be used later.
>>
>>>
>>> first call
>>> ----------
>>> end = 65
>>> nr_minors = 63
>>>
>>> --> we need some space
>>> --> we allocate 2 longs = 128 bits
>>> --> we now use 65 bits of these 128 bits
>>
>> or 96, 32 or 64 bit longs remember.
>
> 32 and 64 for sure, but I was not aware of 96. On which arch?
>
>>
>>>
>>> new call
>>> --------
>>> end = 68
>>> nr_minors = 128 (from previous call)
>>
>> The initial allocation is now bitmap_zalloc which
>> specifies only bits and the nr_minors is then in
>> BITS_TO_LONGS(bits) * BITS_PER_LONG
>>
>> Perhaps that assumes too much about the internal
>> implementation of bitmap_alloc
>>
>>
>
> I get your point now, and I agree with you.
>
> Maybe something as what is done in mc-entity.c?
> Explicitly require more bits (which will be allocated anyway), instead of taking advantage (read "hoping") that it will be done.
>
> Could be:
>
> @@ -440,26 +440,25 @@ static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
>      int rc;
>
>      if (end > nr_minors) {
>          unsigned long *bitmap, *old;
>
> -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
> -                 GFP_KERNEL);
> +        end = ALIGN(end, BITS_PER_LONG);
> +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
>          if (bitmap == NULL)
>              return -ENOMEM;
>
>          spin_lock(&minor_lock);
>          if (end > nr_minors) {
>              old = minors;
> -            memcpy(bitmap, minors,
> -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
> +            bitmap_copy(bitmap, minors, nr_minors);
>              minors = bitmap;
> -            nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
> +            nr_minors = end;
>          } else
>              old = bitmap;
>          spin_unlock(&minor_lock);
> -        kfree(old);
> +        bitmap_free(old);
>      }
>
>      spin_lock(&minor_lock);
>      if (find_next_bit(minors, end, minor) >= end) {


I don't think this will work anymore, we may now fail if another thread gets a minor above the original (i.e. no aligned) @end.


-boris


> bitmap_set(minors, minor, nr);
> @@ -2608,11 +2607,11 @@ static void __exit xlblk_exit(void)
>  {
>      cancel_delayed_work_sync(&blkfront_work);
>
>      xenbus_unregister_driver(&blkfront_driver);
>      unregister_blkdev(XENVBD_MAJOR, DEV_NAME);
> -    kfree(minors);
> +    bitmap_free(minors);
>  }
>  module_exit(xlblk_exit);
>
>  MODULE_DESCRIPTION("Xen virtual block device frontend");
>  MODULE_LICENSE("GPL");
>
>