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

[Abdiel Janulgue <abdiel.janulgue@gmail.com>]

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


Regards,
Abdiel