linux-kernel - Re: [PATCH v6 3/4] rust: add bitmap API.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z-rJ0vMFsFIOP84B@thinkpad>
Date: Mon, 31 Mar 2025 12:58:58 -0400
From: Yury Norov <yury.norov@...il.com>
To: Burak Emir <bqe@...gle.com>
Cc: Rasmus Villemoes <linux@...musvillemoes.dk>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 3/4] rust: add bitmap API.

On Fri, Mar 28, 2025 at 11:36:51AM +0100, Burak Emir wrote:
> On Thu, Mar 27, 2025 at 5:16 PM Burak Emir <bqe@...gle.com> wrote:
> >
> > +    /// Set bit with index `index`.
> 
> I missed this, will change to /// Set `index` 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
> > +        );
> > +        // SAFETY: Bit `index` is within bounds.
> > +        unsafe { bindings::__set_bit(index as u32, self.as_mut_ptr()) };
> > +    }
> > +
> > +    /// Set bit with index `index`, atomically.
> 
> dto, will change to /// Set `index` bit, atomically.
> 
> > +    ///
> > +    /// WARNING: this is a relaxed atomic operation (no implied memory barriers).
> 
> Is this the kind of warning you had in mind?

The __set_bit() in C and set_bit() in rust is a non-atomic function.
Relaxed atomic API has a different meaning. Please add something like
the following on top of 'pub fn set_bit()' implementation:

    /// ATTENTION: Contrary to C, the rust set_bit() method is non-atomic.
    /// This mismatches kernel naming convention and corresponds to the C
    /// function __set_bit(). For atomicity, use the set_bit_atomic() method.
 
> > +    ///
> > +    /// # Panics
> > +    ///
> > +    /// Panics if `index` is greater than or equal to `self.nbits`.
> > +    #[inline]
> > +    pub fn set_bit_atomic(&self, index: usize) {
> > +        assert!(
> > +            index < self.nbits,
> > +            "Bit `index` must be < {}, was {}",
> > +            self.nbits,
> > +            index
> > +        );
> > +        // SAFETY: `index` is within bounds and there cannot be any data races
> > +        // because all non-atomic operations require exclusive access through
> > +        // a &mut reference.
> 
> I have considered marking set_bit_atomic as unsafe, but then come
> around to think that it is actually safe.
> 
> I'd appreciate a review of the reasoning by my fellow Rust-for-Linux folks.
> 
> What must be ensured is absence of data race, e.g. that an atomic op
> does not happen concurrently with a conflicting non-synchronized,
> non-atomic op.
> Do I need to worry about non-atomic accesses in the same thread
> (temporarily reborrowing a &mut to & in the same thread is a
> possibility)?

To me - no. Atomicity only works if everyone follow the same rules.
If someone accessed some data without grabbing a lock on it, and
ended up corrupting the kernel, it's not a problem of spinlock API.

Thanks,
Yury