linux-kernel - Re: [PATCH 1/2] rust: add initial scatterlist bindings

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 for Android: free password hash cracker in your pocket

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

Message-ID: <aEG5X7FduqFvgXxH@Mac.home>
Date: Thu, 5 Jun 2025 08:35:59 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Jason Gunthorpe <jgg@...pe.ca>,
	Abdiel Janulgue <abdiel.janulgue@...il.com>, dakr@...nel.org,
	lyude@...hat.com, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...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>,
	Valentin Obst <kernel@...entinobst.de>,
	open list <linux-kernel@...r.kernel.org>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Robin Murphy <robin.murphy@....com>, airlied@...hat.com,
	rust-for-linux@...r.kernel.org,
	"open list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>,
	Petr Tesarik <petr@...arici.cz>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Sui Jingfeng <sui.jingfeng@...ux.dev>,
	Randy Dunlap <rdunlap@...radead.org>,
	Michael Kelley <mhklinux@...look.com>
Subject: Re: [PATCH 1/2] rust: add initial scatterlist bindings

On Fri, May 30, 2025 at 11:02:02PM +0900, Alexandre Courbot wrote:
> On Thu May 29, 2025 at 9:45 AM JST, Jason Gunthorpe wrote:
> > On Thu, May 29, 2025 at 01:14:05AM +0300, Abdiel Janulgue wrote:
> >> +impl SGEntry<Unmapped> {
> >> +    /// Set this entry to point at a given page.
> >> +    pub fn set_page(&mut self, page: &Page, length: u32, offset: u32) {
> >> +        let c: *mut bindings::scatterlist = self.0.get();
> >> +        // SAFETY: according to the `SGEntry` invariant, the scatterlist pointer is valid.
> >> +        // `Page` invariant also ensures the pointer is valid.
> >> +        unsafe { bindings::sg_set_page(c, page.as_ptr(), length, offset) };
> >> +    }
> >> +}
> >
> > Wrong safety statement. sg_set_page captures the page.as_ptr() inside
> > the C datastructure so the caller must ensure it holds a reference on
> > the page while it is contained within the scatterlist.
> >
> > Which this API doesn't force to happen.
> >
> > Most likely for this to work for rust you have to take a page
> > reference here and ensure the page reference is put back during sg
> > destruction. A typical normal pattern would 'move' the reference from
> > the caller into the scatterlist.
> 
> As Jason mentioned, we need to make sure that the backing pages don't get
> dropped while the `SGTable` is alive. The example provided unfortunately fails
> to do that:
> 
>     let sgt = SGTable::alloc_table(4, GFP_KERNEL)?;
>     let sgt = sgt.init(|iter| {
>         for sg in iter {
>             sg.set_page(&Page::alloc_page(GFP_KERNEL)?, PAGE_SIZE as u32, 0);
>         }
>         Ok(())
>     })?;
> 
> Here the allocated `Page`s are dropped immediately after their address is
> written by `set_page`, giving the device access to memory that may now be used
> for completely different purposes. As long as the `SGTable` exists, the memory
> it points to must not be released or reallocated in any way.
> 

Late to the party, but seems to me the main problem here is that we
cannot pass a reference to .set_page(), note that there is some work
that would change the Rust struct Page from a `*mut page` to a
`page`[0], and then we can impl Ownable[1] and AlwaysRefCounted for
`Page`, if that's done, then I believe the correct parameter for
set_page() would be an ARef<Page>.

The Rust `Page` right now is a owning pointer, which can be only used in
very simple cases. I think it's better that we improve that first.
Another idea is keeping `Page` a `Ownable` type, but wrapping `folio` as
refcounted (i.e. impl AlwaysRefCounted). 

Hmm... after reading more on the following discussion, seems the current
plan is to let SGTable own the pages? That looks reasonable for now, but
one day or another we will need to face this page/folio reference issue.

[0]: https://lore.kernel.org/rust-for-linux/20250202-rust-page-v1-0-e3170d7fe55e@asahilina.net/
[1]: https://lore.kernel.org/rust-for-linux/20250502-unique-ref-v10-0-25de64c0307f@pm.me/

Regards,
Boqun

> To that effect, we could simply store the `Page`s into the `SGTable`, but that
> would cover only one of the many ways they can be constructed. For instance we
> may want to share a `VVec` with a device and this just won't allow doing it.
> 
> So we need a way to keep the provider of the pages alive into the `SGTable`,
> while also having a convenient way to get its list of pages. Here is rough idea
> for doing this, it is very crude and probably not bulletproof but hopefully it
> can constitute a start.
> 
[..]