Messages in this thread | | | From | Andy Shevchenko <> | Date | Sat, 3 Oct 2020 15:45:04 +0300 | Subject | Re: [PATCH v10 1/4] bitops: Introduce the for_each_set_clump macro |
| |
On Sat, Oct 3, 2020 at 2:37 PM Syed Nayyar Waris <syednwaris@gmail.com> wrote: > On Sat, Oct 3, 2020 at 2:14 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Sat, Oct 3, 2020 at 2:51 AM Syed Nayyar Waris <syednwaris@gmail.com> 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@vger.kernel.org/msg2202377.html
Ah, okay! -- With Best Regards, Andy Shevchenko
| |