| 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
| ||
|
Message-ID: <CAJ-ks9kECSobk0NX6SXn1US7My028POc=nLmw0AHZGiRUstP2g@mail.gmail.com>
Date: Mon, 11 Aug 2025 14:02:59 -0400
From: Tamir Duberstein <tamird@...il.com>
To: Beata Michalska <beata.michalska@....com>
Cc: Andreas Hindborg <a.hindborg@...nel.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 <lossin@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>,
Matthew Wilcox <willy@...radead.org>, Andrew Morton <akpm@...ux-foundation.org>,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
Daniel Almeida <daniel.almeida@...labora.com>, Janne Grunau <j@...nau.net>
Subject: Re: [PATCH v2 3/3] rust: xarray: add `insert` and `reserve`
On Mon, Aug 11, 2025 at 10:35 AM Beata Michalska
<beata.michalska@....com> wrote:
>
> On Mon, Aug 11, 2025 at 09:09:56AM -0400, Tamir Duberstein wrote:
> > On Mon, Aug 11, 2025 at 8:57 AM Beata Michalska <beata.michalska@....com> wrote:
> > >
> > > Hi Tamir,
> > >
> > > Apologies for such a late drop.
> >
> > Hi Beata, no worries, thanks for your review.
> >
> > >
> > > On Sun, Jul 13, 2025 at 08:05:49AM -0400, Tamir Duberstein wrote:
> [snip] ...
> > > > +/// A reserved slot in an array.
> > > > +///
> > > > +/// The slot is released when the reservation goes out of scope.
> > > > +///
> > > > +/// Note that the array lock *must not* be held when the reservation is filled or dropped as this
> > > > +/// will lead to deadlock. [`Reservation::fill_locked`] and [`Reservation::release_locked`] can be
> > > > +/// used in context where the array lock is held.
> > > > +#[must_use = "the reservation is released immediately when the reservation is unused"]
> > > > +pub struct Reservation<'a, T: ForeignOwnable> {
> > > > + xa: &'a XArray<T>,
> > > > + index: usize,
> > > > +}
> > > > +
> [snip] ...
> > > > +
> > > > +impl<T: ForeignOwnable> Drop for Reservation<'_, T> {
> > > > + fn drop(&mut self) {
> > > > + // NB: Errors here are possible since `Guard::store` does not honor reservations.
> > > > + let _: Result = self.release_inner(None);
> > > This seems bit risky as one can drop the reservation while still holding the
> > > lock?
> >
> > Yes, that's true. The only way to avoid it would be to make the
> > reservation borrowed from the guard, but that would exclude usage
> > patterns where the caller wants to reserve and fulfill in different
> > critical sections.
> >
> > Do you have a specific suggestion?
> I guess we could try with locked vs unlocked `Reservation' types, which would
> have different Drop implementations, and providing a way to convert locked into
> unlocked. Just thinking out loud, so no, nothing specific here.
> At very least we could add 'rust_helper_spin_assert_is_held() ?'
I don't see how having two types of reservations would help.
Can you help me understand how you'd use `rust_helper_spin_assert_is_held` here?
> >
> > > > + }
> > > > }
> > > >
> > > > // SAFETY: `XArray<T>` has no shared mutable state so it is `Send` iff `T` is `Send`.
> > > > @@ -282,3 +617,136 @@ unsafe impl<T: ForeignOwnable + Send> Send for XArray<T> {}
> > > > // SAFETY: `XArray<T>` serialises the interior mutability it provides so it is `Sync` iff `T` is
> > > > // `Send`.
> > > > unsafe impl<T: ForeignOwnable + Send> Sync for XArray<T> {}
> > > > +
> > > > +#[macros::kunit_tests(rust_xarray_kunit)]
> > > > +mod tests {
> > > > + use super::*;
> > > > + use pin_init::stack_pin_init;
> > > > +
> > > > + fn new_kbox<T>(value: T) -> Result<KBox<T>> {
> > > > + KBox::new(value, GFP_KERNEL).map_err(Into::into)
> > > I believe this should be GFP_ATOMIC as it is being called while holding the xa
> > > lock.
> >
> > I'm not sure what you mean - this function can be called in any
> > context, and besides: it is test-only code.
> Actually it cannot: allocations using GFP_KERNEL can sleep so should not be
> called from atomic context, which is what is happening in the test cases.
I see. There are no threads involved in these tests, so I think it is
just fine to sleep with this particular lock held. Can you help me
understand why this is incorrect?
>
> ---
> BR
> Beata
> >
> > >
> > > Otherwise:
> > >
> > > Tested-By: Beata Michalska <beata.michalska@....com>
> >
> > Thanks!
> > Tamir
> >
> > >
> > > ---
> > > BR
> > > Beata
> > > > + }
> > > > +
> > > > + #[test]
> > > > + fn test_alloc_kind_alloc() -> Result {
> > > > + test_alloc_kind(AllocKind::Alloc, 0)
> > > > + }
> > > > +
> > > > + #[test]
> > > > + fn test_alloc_kind_alloc1() -> Result {
> > > > + test_alloc_kind(AllocKind::Alloc1, 1)
> > > > + }
> > > > +
> > > > + fn test_alloc_kind(kind: AllocKind, expected_index: usize) -> Result {
> > > > + stack_pin_init!(let xa = XArray::new(kind));
> > > > + let mut guard = xa.lock();
> > > > +
> > > > + let reservation = guard.reserve_limit(.., GFP_KERNEL)?;
> > > > + assert_eq!(reservation.index(), expected_index);
> > > > + reservation.release_locked(&mut guard)?;
> > > > +
> > > > + let insertion = guard.insert_limit(.., new_kbox(0x1337)?, GFP_KERNEL);
> > > > + assert!(insertion.is_ok());
> > > > + let insertion_index = insertion.unwrap();
> > > > + assert_eq!(insertion_index, expected_index);
> > > > +
> > > > + Ok(())
> > > > + }
> > > > +
> > > > + #[test]
> > > > + fn test_insert_and_reserve_interaction() -> Result {
> > > > + const IDX: usize = 0x1337;
> > > > +
> > > > + fn insert<T: ForeignOwnable>(
> > > > + guard: &mut Guard<'_, T>,
> > > > + value: T,
> > > > + ) -> Result<(), StoreError<T>> {
> > > > + guard.insert(IDX, value, GFP_KERNEL)
> > > > + }
> > > > +
> > > > + fn reserve<'a, T: ForeignOwnable>(guard: &mut Guard<'a, T>) -> Result<Reservation<'a, T>> {
> > > > + guard.reserve(IDX, GFP_KERNEL)
> > > > + }
> > > > +
> > > > + #[track_caller]
> > > > + fn check_not_vacant<'a>(guard: &mut Guard<'a, KBox<usize>>) -> Result {
> > > > + // Insertion fails.
> > > > + {
> > > > + let beef = new_kbox(0xbeef)?;
> > > > + let ret = insert(guard, beef);
> > > > + assert!(ret.is_err());
> > > > + let StoreError { error, value } = ret.unwrap_err();
> > > > + assert_eq!(error, EBUSY);
> > > > + assert_eq!(*value, 0xbeef);
> > > > + }
> > > > +
> > > > + // Reservation fails.
> > > > + {
> > > > + let ret = reserve(guard);
> > > > + assert!(ret.is_err());
> > > > + assert_eq!(ret.unwrap_err(), EBUSY);
> > > > + }
> > > > +
> > > > + Ok(())
> > > > + }
> > > > +
> > > > + stack_pin_init!(let xa = XArray::new(Default::default()));
> > > > + let mut guard = xa.lock();
> > > > +
> > > > + // Vacant.
> > > > + assert_eq!(guard.get(IDX), None);
> > > > +
> > > > + // Reservation succeeds.
> > > > + let reservation = {
> > > > + let ret = reserve(&mut guard);
> > > > + assert!(ret.is_ok());
> > > > + ret.unwrap()
> > > > + };
> > > > +
> > > > + // Reserved presents as vacant.
> > > > + assert_eq!(guard.get(IDX), None);
> > > > +
> > > > + check_not_vacant(&mut guard)?;
> > > > +
> > > > + // Release reservation.
> > > > + {
> > > > + let ret = reservation.release_locked(&mut guard);
> > > > + assert!(ret.is_ok());
> > > > + let () = ret.unwrap();
> > > > + }
> > > > +
> > > > + // Vacant again.
> > > > + assert_eq!(guard.get(IDX), None);
> > > > +
> > > > + // Insert succeeds.
> > > > + {
> > > > + let dead = new_kbox(0xdead)?;
> > > > + let ret = insert(&mut guard, dead);
> > > > + assert!(ret.is_ok());
> > > > + let () = ret.unwrap();
> > > > + }
> > > > +
> > > > + check_not_vacant(&mut guard)?;
> > > > +
> > > > + // Remove.
> > > > + assert_eq!(guard.remove(IDX).as_deref(), Some(&0xdead));
> > > > +
> > > > + // Reserve and fill.
> > > > + {
> > > > + let beef = new_kbox(0xbeef)?;
> > > > + let ret = reserve(&mut guard);
> > > > + assert!(ret.is_ok());
> > > > + let reservation = ret.unwrap();
> > > > + let ret = reservation.fill_locked(&mut guard, beef);
> > > > + assert!(ret.is_ok());
> > > > + let () = ret.unwrap();
> > > > + };
> > > > +
> > > > + check_not_vacant(&mut guard)?;
> > > > +
> > > > + // Remove.
> > > > + assert_eq!(guard.remove(IDX).as_deref(), Some(&0xbeef));
> > > > +
> > > > + Ok(())
> > > > + }
> > > > +}
> > > >
> > > > --
> > > > 2.50.1
> > > >
> > > >
> >
Powered by blists - more mailing lists