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] [day] [month] [year] [list]
Message-ID: <dad04702-17bc-4554-b64b-e57f34d371a7@gmail.com>
Date: Mon, 30 Jun 2025 10:11:23 +0300
From: Abdiel Janulgue <abdiel.janulgue@...il.com>
To: Alexandre Courbot <acourbot@...dia.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 28/06/2025 14:18, Alexandre Courbot wrote:
> 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.

Thanks for the explanation, indeed that's the limitation if we use a 
lifetime bound on SGTable.
Anyways, I just submitted the v2[1] which should be able to enforce the 
ownership of the pages to the SGTable.

/Abdiel

[1]https://lore.kernel.org/rust-for-linux/20250626203247.816273-1-abdiel.janulgue@gmail.com/