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

From: Abdiel Janulgue
Date: Mon Jun 30 2025 - 03:12:03 EST



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@xxxxxxxxx/