On Mon, May 19, 2025 at 10:42 PM Burak Emir <b...@google.com> wrote:
> On Mon, May 19, 2025 at 8:22 PM Yury Norov <yury.no...@gmail.com> wrote:
> > On Mon, May 19, 2025 at 04:17:03PM +0000, Burak Emir wrote:
> > > + /// Set bit with index `index`.
> > > + ///
> > > + /// ATTENTION: `set_bit` is non-atomic, which differs from the naming
> > > + /// convention in C code. The corresponding C function is
> > > `__set_bit`.
> > > + ///
> > > + /// # Panics
> > > + ///
> > > + /// Panics if `index` is greater than or equal to `self.nbits`.
> > > + #[inline]
> > > + pub fn set_bit(&mut self, index: usize) {
> > > + assert!(
> > > + index < self.nbits,
> > > + "Bit `index` must be < {}, was {}",
> > > + self.nbits,
> > > + index
> > > + );
> >
> > Shouldn't this assertion be protected with hardening too? I already
> > said that: panicking on out-of-boundary access with hardening
> > disabled is a wrong way to go.
>
> I considered it, but could not convince myself that __set_bit etc are
> actually always safe.
> For the methods that have the hardening assert, I was sure, but for
> this one, not.
>
> Are all bit ops guaranteed to handle out-of-bounds gracefully?
>
> > Can you turn your bitmap_hardening_assert() to just bitmap_assert(),
> > which panics only if hardening is enabled, and otherwise just prints
> > error with pr_err()?
>
> If there is no risk of undefined behavior, then I agree that checking
> bounds is hardening.
> If a missing bounds check loses safety, we then we should not skip it.
There are no bounds checks in these C APIs, and there can't be,
because the C side does not store a length. bitmap_zalloc() just gives
you a raw array of bits (represented in C as an array of unsigned
longs), it's a very lightweight wrapper around kmalloc_array().
And if you expand __set_bit(nr, addr), you'll see that it turns into:
bitop(___set_bit, nr, addr)
which turns into:
((__builtin_constant_p(nr) &&
__builtin_constant_p((uintptr_t)(addr) != (uintptr_t)NULL) &&
(uintptr_t)(addr) != (uintptr_t)NULL &&
__builtin_constant_p(*(const unsigned long *)(addr))) ?
const___set_bit(nr, addr) : ___set_bit(nr, addr))
which (assuming a non-constant index) is:
___set_bit(nr, addr)
which is a debug-instrumented wrapper around
arch___set_bit(nr, addr)
which just leads to a raw assembly instruction (example from x86):
static __always_inline void
arch___set_bit(unsigned long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(bts) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
