x86/bitmap: Compile with -Wsign-conversion

[PATCH] x86/bitmap: Compile with -Wsign-conversion

Posted by Andrew Cooper 2 months, 3 weeks ago

Use pragmas to able the warning in this file only.  All supported versions of
Clang understand this, while older GCCs simply ignore it.

bitmap_find_free_region() is the only function which isn't sign-convert
clean.  This highlights a latent bug in that it can't return successfully for
a bitmap larger than 2G.

Add an extra check, and explicit cast to silence the warning.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>

Slightly RFC.  This is our first use of pragmas like this.
---
 xen/common/bitmap.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c
index 18e23e2f0e18..b14f8a3b3030 100644
--- a/xen/common/bitmap.c
+++ b/xen/common/bitmap.c
@@ -14,6 +14,9 @@
 #include <xen/lib.h>
 #include <asm/byteorder.h>
 
+#pragma GCC diagnostic warning "-Wsign-conversion"
+#pragma clang diagnostic warning "-Wsign-conversion"
+
 /*
  * bitmaps provide an array of bits, implemented using an an
  * array of unsigned longs.  The number of valid bits in a
@@ -263,7 +266,7 @@ int bitmap_find_free_region(unsigned long *bitmap, unsigned int bits, unsigned i
 	unsigned int pages = 1 << order;
 	unsigned int i;
 
-	if(pages > BITS_PER_LONG)
+	if (pages > BITS_PER_LONG || bits >= INT_MAX)
 		return -EINVAL;
 
 	/* make a mask of the order */
@@ -278,7 +281,7 @@ int bitmap_find_free_region(unsigned long *bitmap, unsigned int bits, unsigned i
 		if((bitmap[index] & (mask << offset)) == 0) {
 			/* set region in bimap */
 			bitmap[index] |= (mask << offset);
-			return i;
+			return (int)i;
 		}
 	}
 	return -ENOMEM;
-- 
2.30.2

Re: [PATCH] x86/bitmap: Compile with -Wsign-conversion

Posted by Jan Beulich 2 months, 3 weeks ago

On 05.02.2024 16:14, Andrew Cooper wrote:
> @@ -263,7 +266,7 @@ int bitmap_find_free_region(unsigned long *bitmap, unsigned int bits, unsigned i
>  	unsigned int pages = 1 << order;

If we mean to keep this function (see my reply to the other patch),
shouldn't we also check "order" along with

>  	unsigned int i;
>  
> -	if(pages > BITS_PER_LONG)
> +	if (pages > BITS_PER_LONG || bits >= INT_MAX)
>  		return -EINVAL;

... these checks?

> @@ -278,7 +281,7 @@ int bitmap_find_free_region(unsigned long *bitmap, unsigned int bits, unsigned i
>  		if((bitmap[index] & (mask << offset)) == 0) {
>  			/* set region in bimap */
>  			bitmap[index] |= (mask << offset);
> -			return i;
> +			return (int)i;

I agree with Julien that it looks like this change actually belongs in
the other patch.

Jan

Re: [PATCH] x86/bitmap: Compile with -Wsign-conversion

Posted by Julien Grall 2 months, 3 weeks ago

Hi Andrew,

Title: You seem to change common code. So s/x86//

On 05/02/2024 15:14, Andrew Cooper wrote:
> Use pragmas to able the warning in this file only.  All supported versions of
> Clang understand this, while older GCCs simply ignore it.
> 
> bitmap_find_free_region() is the only function which isn't sign-convert
> clean.  This highlights a latent bug in that it can't return successfully for
> a bitmap larger than 2G.
> 
> Add an extra check, and explicit cast to silence the warning.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> 
> Slightly RFC.  This is our first use of pragmas like this.

The only other approach I can think of is specifying the CFLAGS per file 
like Linux did. I don't know if our build system supports that though.

AFAICT, the only advantage would be to avoid duplicating the pragmas. So 
this is not a strong preference.

> ---
>   xen/common/bitmap.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c
> index 18e23e2f0e18..b14f8a3b3030 100644
> --- a/xen/common/bitmap.c
> +++ b/xen/common/bitmap.c
> @@ -14,6 +14,9 @@
>   #include <xen/lib.h>
>   #include <asm/byteorder.h>
>   
> +#pragma GCC diagnostic warning "-Wsign-conversion"
> +#pragma clang diagnostic warning "-Wsign-conversion"
> +

OOI, any reason why wasn't added at the right at the top of the file?

>   /*
>    * bitmaps provide an array of bits, implemented using an an
>    * array of unsigned longs.  The number of valid bits in a
> @@ -263,7 +266,7 @@ int bitmap_find_free_region(unsigned long *bitmap, unsigned int bits, unsigned i
>   	unsigned int pages = 1 << order;
>   	unsigned int i;
>   
> -	if(pages > BITS_PER_LONG)
> +	if (pages > BITS_PER_LONG || bits >= INT_MAX)
>   		return -EINVAL;
>   
>   	/* make a mask of the order */
> @@ -278,7 +281,7 @@ int bitmap_find_free_region(unsigned long *bitmap, unsigned int bits, unsigned i
>   		if((bitmap[index] & (mask << offset)) == 0) {
>   			/* set region in bimap */
>   			bitmap[index] |= (mask << offset);
> -			return i;
> +			return (int)i;

It took me a while to realize that this patch should be reviewed after 
"x86/bitmap: Even more signed-ness fixes".

Before hand, 'i' is a signed int and we would return -ENOMEM if 'bits' 
is negative. So I wonder whether the change in common/bitmap.c should 
belong to the other patch?

Cheers,

-- 
Julien Grall

Re: [PATCH] x86/bitmap: Compile with -Wsign-conversion

Posted by Jan Beulich 2 months, 3 weeks ago

On 05.02.2024 16:55, Julien Grall wrote:
> On 05/02/2024 15:14, Andrew Cooper wrote:
>> Use pragmas to able the warning in this file only.  All supported versions of
>> Clang understand this, while older GCCs simply ignore it.
>>
>> bitmap_find_free_region() is the only function which isn't sign-convert
>> clean.  This highlights a latent bug in that it can't return successfully for
>> a bitmap larger than 2G.
>>
>> Add an extra check, and explicit cast to silence the warning.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: George Dunlap <George.Dunlap@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Wei Liu <wl@xen.org>
>> CC: Julien Grall <julien@xen.org>
>>
>> Slightly RFC.  This is our first use of pragmas like this.
> 
> The only other approach I can think of is specifying the CFLAGS per file 
> like Linux did. I don't know if our build system supports that though.

It does, see e.g.

# Allows usercopy.c to include itself
$(obj)/usercopy.o: CFLAGS-y += -iquote .

in arch/x86/Makefile.

> AFAICT, the only advantage would be to avoid duplicating the pragmas. So 
> this is not a strong preference.

My other concern there are old gcc versions we still support. I haven't
checked (yet) when support for these pragma-s was introduced; I only
know they haven't been there forever. However, ...

>> --- a/xen/common/bitmap.c
>> +++ b/xen/common/bitmap.c
>> @@ -14,6 +14,9 @@
>>   #include <xen/lib.h>
>>   #include <asm/byteorder.h>
>>   
>> +#pragma GCC diagnostic warning "-Wsign-conversion"
>> +#pragma clang diagnostic warning "-Wsign-conversion"
>> +
> 
> OOI, any reason why wasn't added at the right at the top of the file?

... this may be relevant: Inline functions may have an issue with being
processed with the warning enabled. Otoh it may also be a problem if
the warning isn't enabled for them.

Jan