On Sat, Oct 3, 2020 at 2:37 PM Syed Nayyar Waris <syednwaris@xxxxxxxxx> wrote: > On Sat, Oct 3, 2020 at 2:14 PM Andy Shevchenko > <andy.shevchenko@xxxxxxxxx> wrote: > > On Sat, Oct 3, 2020 at 2:51 AM Syed Nayyar Waris <syednwaris@xxxxxxxxx> wrote: ... > > > +/** > > > + * bitmap_get_value - get a value of n-bits from the memory region > > > + * @map: address to the bitmap memory region > > > + * @start: bit offset of the n-bit value > > > + * @nbits: size of value in bits > > > + * > > > + * Returns value of nbits located at the @start bit offset within the @map > > > + * memory region. > > > + */ ... > > > + return (map[index] >> offset) & GENMASK(nbits - 1, 0); > > > > This is UB in GENMASK() when nbits == 0. > > 'nbits' actually specifies the width of clump value. Basically 'nbits' > denotes how-many-bits wide the clump value is. > 'nbits' having a value of '0' means zero-width-sized clump, meaning > nothing. 'nbits' can take valid values from '1' to BITS_PER_LONG. > The minimum value the 'nbits' can have is 1 because the smallest sized > clump can be 1-bit-wide. It can't be smaller than that. > > Let me know if I have misunderstood something? It's still possible to call with an nbits parameter be equal to 0. If code is optimized to allow it, it should be documented that 0 parameter is not valid and behaviour is undefined. ... > > > +/** > > > + * bitmap_set_value - set n-bit value within a memory region > > > + * @map: address to the bitmap memory region > > > + * @value: value of nbits > > > + * @start: bit offset of the n-bit value > > > + * @nbits: size of value in bits > > > + */ ... > > > + value &= GENMASK(nbits - 1, 0); > > > > This is UB when nbits == 0. > > Same as above. > 'nbits' actually specifies the width of clump value. Basically 'nbits' > denotes how-many-bits wide the clump value is. > 'nbits' having a value of '0' means zero-width-sized clump, meaning > nothing. 'nbits' can take valid values from '1' to BITS_PER_LONG. > The minimum value the 'nbits' can have is 1 because the smallest sized > clump can be 1-bit-wide. It can't be smaller than that. Same as above. ... > > > + map[index] &= ~BITMAP_FIRST_WORD_MASK(start); > > > + map[index] |= value << offset; Side note: I would prefer + 0 here and there, but it's up to you. > > > + map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits); > > > + map[index + 1] |= (value >> space); By the way, what about this in the case of start=0, nbits > 64? space == 64 -> UB. (And btw parentheses are redundant here) > > And another LKP finding was among these lines, but I don't remember the details. > > Yes you are right. There was sparse warning reported for this. > sparse: shift too big (64) for type unsigned long > The warning was reported in patch [4/4] referring to this patch [1/4]. > > Later it was clarified by the sparse-check maintainer that this > warning is to be ignored and no code fix is required. > > https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg2202377.html Ah, okay! -- With Best Regards, Andy Shevchenko
