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]

Message-ID: <f9a5bc6c-347b-8243-2784-04199ef879c2@wanadoo.fr>
Date:   Sat, 4 Dec 2021 07:57:21 +0100
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        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

Le 03/12/2021 à 22:04, Boris Ostrovsky a écrit :
> 
> On 12/3/21 10:54 AM, Christophe JAILLET wrote:
>> Le 03/12/2021 à 04:03, Joe Perches a écrit :
>>
>> 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.
> 

So, maybe adding an "official" 'bitmap_size()' (which is already 
existing and duplicated in a few places) would ease things.

It would replace the 'nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;' 
and hide the implementation details of the bitmap API.

Something like:
static __always_inline size_t bitmap_size(unsigned long nr_bits)
{
	return BITS_TO_LONGS(nr_bits) * sizeof(long);
}

CJ

> 
> -boris
>