On Sat, Oct 03, 2020 at 03:45:04PM +0300, Andy Shevchenko wrote: > 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. Documenting that 0 is not valid would be preferred because an additional conditional check in the code could add a significant latency in a loop. So perhaps change the documentation line to: @nbits: size of value in bits (must be between 1 and BITS_PER_LONG) > > ... > > > > > +/** > > > > + * 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) I think this is the same situation as before: we should document that nbits must be between 1 and BITS_PER_LONG. William Breathitt Gray > > > > 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
Attachment:
signature.asc
Description: PGP signature