linux-kernel - Re: [PATCH v8 3/5] 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: <CACQBu=UNAFjKw_m8oE5Mst_eThEf36zqgUWZ3a0u1m4zr6MoJw@mail.gmail.com>
Date: Mon, 19 May 2025 22:07:59 +0200
From: Burak Emir <bqe@...gle.com>
To: Jann Horn <jannh@...gle.com>
Cc: Yury Norov <yury.norov@...il.com>, Kees Cook <kees@...nel.org>, 
	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>, 
	"Gustavo A . R . Silva" <gustavoars@...nel.org>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v8 3/5] rust: add bitmap API.

On Mon, May 19, 2025 at 9:01 PM Jann Horn <jannh@...gle.com> wrote:
>
> On Mon, May 19, 2025 at 6:24 PM Burak Emir <bqe@...gle.com> wrote:
> > +    /// Set bit with index `index`, atomically.
> > +    ///
> > +    /// ATTENTION: The naming convention differs from C, where the corresponding
> > +    /// function is called `set_bit`.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// This is a relaxed atomic operation (no implied memory barriers, no
> > +    /// ordering guarantees). The caller must ensure that this is safe, as
> > +    /// the compiler cannot prevent code with an exclusive reference from
> > +    /// calling atomic operations.
>
> How can atomic operations through an exclusive reference be unsafe?
> You can't have a data race between two atomic operations, and an
> exclusive reference should anyway prevent any concurrency, right?

The atomic operations take a &self (shared reference).

The patch is missing the implementation of Sync for now. With that,
one would get concurrent write access through shared references.

The "unsafe" here should serve as reminder to argue why it is ok to
not have any ordering guarantees.

The last sentence is supposed to say: when you have a &mut bitmap, you
can reborrow it as &bitmap, and then happily call this atomic op.
Even though it is unnecessary.

It is unsafe because if one has shared refs on multiple threads, one
needs to be ready to observe updates in any order.

Does it make more sense now? I am open to ideas how to describe this better.


> > +    ///
> > +    /// # Panics
> > +    ///
> > +    /// Panics if `index` is greater than or equal to `self.nbits`.
> > +    #[inline]
> > +    pub unsafe 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 the caller has ensured that
> > +        // there is no mix of non-atomic and atomic operations.
>
> Aren't non-atomic operations only possible through a mutable
> reference? And doesn't the rust compiler enforce that, if someone
> holds a mutable reference, nobody else can hold any reference at all?
>
> You wrote yourself above: "all (non-atomic) mutating operations
> require a &mut reference which amounts to exclusive access."

As noted above, an exclusive ref can always be reborrowed as a shared ref.

> I don't understand what's going on here, unless you're saying that
> Rust does not enforce that an object ownership transfer between
> threads has proper RELEASE/ACQUIRE (or RELEASE/CONSUME) memory
> ordering or something like that?

Indeed without the Sync implementation, it does not make sense to have
atomic ops that take &self.
Sorry for the confusion, I should have added the Sync implementation.

The normal scenario is that concurrent access would be accompanied by
synchronization through a lock.
For things where the order does not matter, something like counters,
using the atomic ops would be ok.

Ownership transfer between threads (sending) is not possible right
now, unless we implement also `Send`.

> > +        unsafe { bindings::set_bit(index as u32, self.as_ptr() as *mut usize) };
> > +    }