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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <DAY4I5PUNEHR.3UBD2WCPS1ZBV@nvidia.com>
Date: Sat, 28 Jun 2025 20:18:00 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Abdiel Janulgue" <abdiel.janulgue@...il.com>, "Jason Gunthorpe"
 <jgg@...pe.ca>
Cc: <dakr@...nel.org>, <lyude@...hat.com>, "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>,
 "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 Thu Jun 5, 2025 at 10:22 PM JST, Abdiel Janulgue wrote:
>
>
> On 30/05/2025 17:02, 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.
>
>
> Hi just a silly observation while trying to think about other ways to 
> tie the page lifetime to the sgtable. Why can't we just use a lifetime 
> bound annotation?
>
> It's simpler and it seems to work:
>
>
> impl<'b> SGEntry<'b, Unmapped> {
>      pub fn set_page<'a: 'b> (&mut self, page: &'a Page, length: u32, 
> offset: u32)
>
> So with this, my erroneous example fails to compile. Here the compiler 
> enforces the use  of the api so that the page of the lifetime is always 
> tied to the sgtable:
>
>
> let sgt = sgt.init(|iter| {
>     |                             ---- has type 
> `kernel::scatterlist::SGTableIterMut<'1>`
> 71 |             for sg in iter {
>     |                 -- assignment requires that borrow lasts for `'1`
> 72 |                 sg.set_page(&Page::alloc_page(GFP_KERNEL)?, 
> PAGE_SIZE as u32, 0);
>     |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 
>               - temporary value is freed at the end of this statement
>     |                              |
>     |                              creates a temporary value which is 
> freed while still in use

That would work for this example, but IIUC the bound lifetime will also
prevent you from doing any sort of dynamic lifetime management using a
smart pointer, meaning you cannot store the SGTable into another object?

Whereas storing any generic owner lets use pass a regular reference
(which lifetime will thus propagate to the SGTable) to serve your
example, but also works with any smart pointer.