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: <e2677c26-1c25-4a34-b666-9dcfa9642fd1@gmail.com>
Date: Thu, 26 Jun 2025 23:31:15 +0300
From: Abdiel Janulgue <abdiel.janulgue@...il.com>
To: Alexandre Courbot <acourbot@...dia.com>, Lyude Paul <lyude@...hat.com>,
 Jason Gunthorpe <jgg@...pe.ca>
Cc: dakr@...nel.org, 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 18/06/2025 04:03, Alexandre Courbot wrote:
> Hi Lyude, sorry for taking so long to come back to this!
> 
> On Tue Jun 10, 2025 at 2:44 AM JST, Lyude Paul wrote:
>> On Thu, 2025-06-05 at 22:56 +0900, Alexandre Courbot wrote:
>>> On Thu Jun 5, 2025 at 10:30 PM JST, Abdiel Janulgue wrote:
>>>>
>>>>
>>>> On 05/06/2025 08:51, Alexandre Courbot wrote:
>>>>> Maybe I need more context to understand your problem, but the point of
>>>>> this design is precisely that it doesn't make any assumption about the
>>>>> memory layout - all `P` needs to do is provide the pages describing the
>>>>> memory backing.
>>>>>
>>>>> Assuming that `_owner` here is the owner of the memory, couldn't you
>>>>> flip your data layout and pass `_owner` (or rather a newtype wrapping
>>>>> it) to `SGTable`, thus removing the need for a custom type?
>>>>
>>>> I think what Lyude has in mind here (Lyude, correct me if I'm wrong) is
>>>> for cases where we need to have a rust SGTable instances for those
>>>> struct sg_table that we didn't allocate ourselves for instance in the
>>>> gem shmem bindings. So memory layout needs to match for
>>>> #[repr(transparent)] to work
>>>
>>> Thanks, I think I am starting to understand and this is a problem
>>> indeed. I should probably take a look at the DRM code to get my answers,
>>> but IIUC in `OwnedSGTable`, `sg_table` is already provided by the C side
>>> and is backed by `_owner`?
>>
>> Correct. You generally create a gem shmem object, and then you can call a
>> function to ask gem to create an sg_table and hand it back to you. I should
>> note my priorities have shifted a bit from trying to get shmem bindings
>> upstream, but currently it's still the best example I have of this usecase.
>>
>> So, for gem shmem this means we can operate with an SGTable in two ways:
>>
>>   * gem_shmem_object.sg_table() -> Result<&kernel::scatterlist::SGTable>
>>   * gem_shmem_object.owned_sg_table() ->
>>     Result<kernel::drm::gem::shmem::OwnedSGTable<T: DriverObject>
>>
>> I'm going to call the first return type SGTable and the second one
>> OwnedSGTable, just so I can type a bit less.
>>
>> The first, sg_table(), quite literally just calls drm_gem_shmem_get_pages_sgt
>> which attempts to allocate the table and return a pointer to it on success. We
>> then cast that to &SGTable and return it to the user. This can be good for
> 
> Mmm but if you cast the returned C pointer into a `&SGTable`, then who
> owns (and is responsible for freeing) the `SGTable` it refers to? If the
> pointer is just turned into a reference then this might leak the `struct
> sg_table`.
> 

Just commenting on this bit. From what I've seen, we don't actually leak 
anything. The cast only creates a reference to the original C `struct 
sg_table` object which was allocated and owned by whichever kernel 
subsystem called sg_alloc_table(). Rust doesn't even allow us to take 
ownership or to dereference the value, so this one is safe. Destructors 
are not called on those "casted" objects.

/Abdiel