| Message ID | alpine.DEB.2.22.394.2309281616200.1996340@ubuntu-linux-20-04-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bitmap: fix n__ signess | expand |
Hi Stefano, On 29/09/2023 00:19, Stefano Stabellini wrote: > All callers of the bitmap_switch macro (which are all within bitmap.h) > pass an int as first parameter. Do not assign it to an unsigned int > potentially introducing errors. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > --- > > We could also have gone the other way and change all the callers and the > callers' callers to use an unsigned int instead, but I went for the path > of least resistance. I understand this will solve the issue right now because the callers are all passing 'int'. However, all the callers will need to switch to 'unsigned int' in order to solve violations in their callers. That unless we decide to use 'int' everywhere, but I think this is a bad idea because 'n__' is not supposed to be negative. Overall, this may be an easy win right now, but this will need to be reverted. So, I am not happy to ack it and would in fact be leaning towards Nacking it. Cheers,
> On 29 Sep 2023, at 08:24, Julien Grall <julien@xen.org> wrote: > > Hi Stefano, > > On 29/09/2023 00:19, Stefano Stabellini wrote: >> All callers of the bitmap_switch macro (which are all within bitmap.h) >> pass an int as first parameter. Do not assign it to an unsigned int >> potentially introducing errors. >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> >> --- >> We could also have gone the other way and change all the callers and the >> callers' callers to use an unsigned int instead, but I went for the path >> of least resistance. > > I understand this will solve the issue right now because the callers are all passing 'int'. However, all the callers will need to switch to 'unsigned int' in order to solve violations in their callers. I was about to send a patch like this but Stefano beat me sending this one, anyway I confirm that switching the callers type will solve this violation. Cheers, Luca
On Fri, Sep 29, 2023 at 08:24:37AM +0100, Julien Grall wrote: > Hi Stefano, > > On 29/09/2023 00:19, Stefano Stabellini wrote: > > All callers of the bitmap_switch macro (which are all within bitmap.h) > > pass an int as first parameter. Do not assign it to an unsigned int > > potentially introducing errors. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > --- > > > > We could also have gone the other way and change all the callers and the > > callers' callers to use an unsigned int instead, but I went for the path > > of least resistance. > > I understand this will solve the issue right now because the callers are > all passing 'int'. However, all the callers will need to switch to > 'unsigned int' in order to solve violations in their callers. > > That unless we decide to use 'int' everywhere, but I think this is a bad > idea because 'n__' is not supposed to be negative. > > Overall, this may be an easy win right now, but this will need to be > reverted. So, I am not happy to ack it and would in fact be leaning > towards Nacking it. I would have tended to review on the basis situations like this unsigned is always the way to go. n__ *wants* to be unsigned since such structures nearly always have some level of implicit assemption of being unsigned. Turns out though the situation is worse since as proposed this also significantly changes the logic. The value of `(int)n__ <= BITS_PER_LONG` may differ from `(unsigned int)n__ <= BITS_PER_LONG`. I wouldn't expect these functions to get negative values, but if they did a lurking issue has been added.
On Fri, 29 Sep 2023, Julien Grall wrote: > On 29/09/2023 00:19, Stefano Stabellini wrote: > > All callers of the bitmap_switch macro (which are all within bitmap.h) > > pass an int as first parameter. Do not assign it to an unsigned int > > potentially introducing errors. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > --- > > > > We could also have gone the other way and change all the callers and the > > callers' callers to use an unsigned int instead, but I went for the path > > of least resistance. > > I understand this will solve the issue right now because the callers are all > passing 'int'. However, all the callers will need to switch to 'unsigned int' > in order to solve violations in their callers. > > That unless we decide to use 'int' everywhere, but I think this is a bad idea > because 'n__' is not supposed to be negative. > > Overall, this may be an easy win right now, but this will need to be reverted. > So, I am not happy to ack it and would in fact be leaning towards Nacking it. I understand this point and I was undecided myself about the approach. The issue for me is the overwhelming amount of gcc warnings (thankfully Luca's script helps a lot with it). With so many warning, it is difficult to draw the line where to stop fixing things to generate a digestable patch and not having the feeling of unraveling an infinite ball of yarn. So, worried about having to change hundreds of lines of code, I submitted the minimal change instead. In this case though unsigned int is obviously the right type and the patch below works as well. So I think that's better. --- [PATCH v2] bitmap: fix nbits signess To avoid potentially dangerous sign conversions in bitmap_switch, all the callers of the bitmap_switch macro (which are all within bitmap.h) should pass an unsigned int as first parameter. Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- Changes in v2: - change nbits instead diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h index 3caf92c76d..657390e32e 100644 --- a/xen/include/xen/bitmap.h +++ b/xen/include/xen/bitmap.h @@ -110,7 +110,7 @@ extern int bitmap_allocate_region(unsigned long *bitmap, int pos, int order); large; \ } -static inline void bitmap_zero(unsigned long *dst, int nbits) +static inline void bitmap_zero(unsigned long *dst, unsigned int nbits) { bitmap_switch(nbits,, *dst = 0UL, @@ -134,7 +134,7 @@ static inline void bitmap_fill(unsigned long *dst, int nbits) } static inline void bitmap_copy(unsigned long *dst, const unsigned long *src, - int nbits) + unsigned int nbits) { bitmap_switch(nbits,, *dst = *src, @@ -142,7 +142,7 @@ static inline void bitmap_copy(unsigned long *dst, const unsigned long *src, } static inline void bitmap_and(unsigned long *dst, const unsigned long *src1, - const unsigned long *src2, int nbits) + const unsigned long *src2, unsigned int nbits) { bitmap_switch(nbits,, *dst = *src1 & *src2, @@ -150,7 +150,7 @@ static inline void bitmap_and(unsigned long *dst, const unsigned long *src1, } static inline void bitmap_or(unsigned long *dst, const unsigned long *src1, - const unsigned long *src2, int nbits) + const unsigned long *src2, unsigned int nbits) { bitmap_switch(nbits,, *dst = *src1 | *src2, @@ -158,7 +158,7 @@ static inline void bitmap_or(unsigned long *dst, const unsigned long *src1, } static inline void bitmap_xor(unsigned long *dst, const unsigned long *src1, - const unsigned long *src2, int nbits) + const unsigned long *src2, unsigned int nbits) { bitmap_switch(nbits,, *dst = *src1 ^ *src2, @@ -166,7 +166,7 @@ static inline void bitmap_xor(unsigned long *dst, const unsigned long *src1, } static inline void bitmap_andnot(unsigned long *dst, const unsigned long *src1, - const unsigned long *src2, int nbits) + const unsigned long *src2, unsigned int nbits) { bitmap_switch(nbits,, *dst = *src1 & ~*src2, @@ -174,7 +174,7 @@ static inline void bitmap_andnot(unsigned long *dst, const unsigned long *src1, } static inline void bitmap_complement(unsigned long *dst, const unsigned long *src, - int nbits) + unsigned int nbits) { bitmap_switch(nbits,, *dst = ~*src & BITMAP_LAST_WORD_MASK(nbits), @@ -182,7 +182,7 @@ static inline void bitmap_complement(unsigned long *dst, const unsigned long *sr } static inline int bitmap_equal(const unsigned long *src1, - const unsigned long *src2, int nbits) + const unsigned long *src2, unsigned int nbits) { bitmap_switch(nbits, return -1, @@ -191,7 +191,7 @@ static inline int bitmap_equal(const unsigned long *src1, } static inline int bitmap_intersects(const unsigned long *src1, - const unsigned long *src2, int nbits) + const unsigned long *src2, unsigned int nbits) { bitmap_switch(nbits, return -1, @@ -200,7 +200,7 @@ static inline int bitmap_intersects(const unsigned long *src1, } static inline int bitmap_subset(const unsigned long *src1, - const unsigned long *src2, int nbits) + const unsigned long *src2, unsigned int nbits) { bitmap_switch(nbits, return -1, @@ -208,7 +208,7 @@ static inline int bitmap_subset(const unsigned long *src1, return __bitmap_subset(src1, src2, nbits)); } -static inline int bitmap_empty(const unsigned long *src, int nbits) +static inline int bitmap_empty(const unsigned long *src, unsigned int nbits) { bitmap_switch(nbits, return -1, @@ -216,7 +216,7 @@ static inline int bitmap_empty(const unsigned long *src, int nbits) return __bitmap_empty(src, nbits)); } -static inline int bitmap_full(const unsigned long *src, int nbits) +static inline int bitmap_full(const unsigned long *src, unsigned int nbits) { bitmap_switch(nbits, return -1,
On Fri, Sep 29, 2023 at 02:13:59PM -0700, Stefano Stabellini wrote: > On Fri, 29 Sep 2023, Julien Grall wrote: > > On 29/09/2023 00:19, Stefano Stabellini wrote: > > > All callers of the bitmap_switch macro (which are all within bitmap.h) > > > pass an int as first parameter. Do not assign it to an unsigned int > > > potentially introducing errors. > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > > --- > > > > > > We could also have gone the other way and change all the callers and the > > > callers' callers to use an unsigned int instead, but I went for the path > > > of least resistance. > > > > I understand this will solve the issue right now because the callers are all > > passing 'int'. However, all the callers will need to switch to 'unsigned int' > > in order to solve violations in their callers. > > > > That unless we decide to use 'int' everywhere, but I think this is a bad idea > > because 'n__' is not supposed to be negative. > > > > Overall, this may be an easy win right now, but this will need to be reverted. > > So, I am not happy to ack it and would in fact be leaning towards Nacking it. > > I understand this point and I was undecided myself about the approach. > The issue for me is the overwhelming amount of gcc warnings (thankfully > Luca's script helps a lot with it). With so many warning, it is > difficult to draw the line where to stop fixing things to generate a > digestable patch and not having the feeling of unraveling an infinite > ball of yarn. So, worried about having to change hundreds of lines of > code, I submitted the minimal change instead. > > In this case though unsigned int is obviously the right type and the > patch below works as well. So I think that's better. This looks much better to me. Did changing the nbits arguments of bitmap_copy(), bitmap_fill(), and bitmap_weight(), result in the mess getting worse? If so then this is a reasonable stopping point, but if you didn't check I would be tempted to do so (ensure consistency amoung these functions).
On 29.09.2023 23:13, Stefano Stabellini wrote: > [PATCH v2] bitmap: fix nbits signess > > To avoid potentially dangerous sign conversions in bitmap_switch, all > the callers of the bitmap_switch macro (which are all within bitmap.h) > should pass an unsigned int as first parameter. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> Acked-by: Jan Beulich <jbeulich@suse.com>
diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h index 3caf92c76d..7caec098d7 100644 --- a/xen/include/xen/bitmap.h +++ b/xen/include/xen/bitmap.h @@ -101,7 +101,7 @@ extern int bitmap_allocate_region(unsigned long *bitmap, int pos, int order); #define bitmap_bytes(nbits) (BITS_TO_LONGS(nbits) * sizeof(unsigned long)) #define bitmap_switch(nbits, zero, small, large) \ - unsigned int n__ = (nbits); \ + int n__ = (nbits); \ if (__builtin_constant_p(nbits) && !n__) { \ zero; \ } else if (__builtin_constant_p(nbits) && n__ <= BITS_PER_LONG) { \
All callers of the bitmap_switch macro (which are all within bitmap.h) pass an int as first parameter. Do not assign it to an unsigned int potentially introducing errors. Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- We could also have gone the other way and change all the callers and the callers' callers to use an unsigned int instead, but I went for the path of least resistance.