linux-kernel - Re: [PATCH] rust: zpool: add abstraction for zpool drivers

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: <DC77PBXCZP32.1FJ6BGM9YV27H@kernel.org>
Date: Wed, 20 Aug 2025 13:18:23 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Vitaly Wool" <vitaly.wool@...sulko.se>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 "Uladzislau Rezki" <urezki@...il.com>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Vlastimil Babka" <vbabka@...e.cz>, "Lorenzo Stoakes"
 <lorenzo.stoakes@...cle.com>, "Liam R . Howlett" <Liam.Howlett@...cle.com>,
 "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>,
 "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>, "Bjorn
 Roy Baron" <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor Gross"
 <tmgross@...ch.edu>, "Johannes Weiner" <hannes@...xchg.org>, "Yosry Ahmed"
 <yosry.ahmed@...ux.dev>, "Nhat Pham" <nphamcs@...il.com>,
 <linux-mm@...ck.org>
Subject: Re: [PATCH] rust: zpool: add abstraction for zpool drivers

On Wed Aug 20, 2025 at 12:44 PM CEST, Vitaly Wool wrote:
>> On Aug 20, 2025, at 12:16 PM, Danilo Krummrich <dakr@...nel.org> wrote:
>>> +/// zpool API
>> 
>> It would be nice to have some more documentation on this trait, including a
>> doc-test illustrating some example usage.
>> 
>>> +pub trait Zpool {
>>> +    /// Opaque Rust representation of `struct zpool`.
>>> +    type Pool: ForeignOwnable;
>> 
>> Something that embedds a struct zpool, such as struct zswap_pool? If so, isn't
>> this type simply Self?
>
> I think ForeignOwnable provides a good representation of 'struct zpool’ and it’s convenient to borrow it, as done later in the patch.

ForeignOwnable is not a representation for a specific type, but rather something
that originates from the Rust side and is owned by the C side. But that's not
the case here.

Regarding the "convenient to borrow" part, why can't Zpool::create() return
Result<Self> and e.g. malloc be defined as

	fn malloc(
	   &self,
	   size: usize,
	   gfp: Flags,
	   nid: NumaNode,
	) -> Result<usize>;

i.e. why does it need to be ForeignOwnable in the semantic sense of the trait?

>>> +    /// Make all the necessary preparations for the caller to be able to read from the object
>>> +    /// represented by `handle` and return a valid pointer to the `handle` memory to be read.
>>> +    fn read_begin(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize)
>>> +        -> *mut c_void;
>> 
>> Why does this return a raw pointer? I think this needs a proper type
>> representation.
>
> The zpool API wants a raw pointer here so I decided not to overcomplicate it. I thought of using NonNull<[u8]> but it doesn’t seem to be a good fit. We’re basically returning a pointer to a place in memory which is guaranteed to be allocated and owned by us, but it is a raw pointer at the end of the day. What would you recommend here instead?

I don't know the exact semantics behind read_begin(), but we should at least
encapsulate the pointer into a new type and restrict its lifetime to the
validity of the encapsulated pointer, such that it can't be used in the wrong
way.

More general, if we don't cover such things and use raw pointers for convenience
instead, we may not provide enough of a benefit compared to what we can do in
a C API.

>>> +
>>> +    /// Finish reading from a previously allocated `handle`. `handle_mem` must be the pointer
>>> +    /// previously returned by `read_begin`.
>>> +    fn read_end(
>>> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
>>> +        handle: usize,
>>> +        handle_mem: *mut c_void,
>>> +    );
>> 
>> Same here...
>> 
>>> +
>>> +    /// Write to the object represented by a previously allocated `handle`. `handle_mem` points
>>> +    /// to the memory to copy data from, and `mem_len` defines the length of the data block to
>>> +    /// be copied.
>>> +    fn write(
>>> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
>>> +        handle: usize,
>>> +        handle_mem: *mut c_void,
>> 
>> ...and here.
>> 
>>> +        mem_len: usize,
>>> +    );
>>> +
>>> +    /// Get the number of pages used by the `pool`.
>>> +    fn total_pages(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>) -> u64;
>>> +}
>>> +
>>> +/// Zpool driver registration trait.
>>> +pub trait Registration {
>> 
>> I think you should use the kernel::driver::Registration instead, it's
>> specifically for the purpose you defined this trait and ZpoolDriver for.
>> 
>> As for the C callbacks, they should go into the Adapter type (which implements
>> kernel::driver::RegistrationOps) directly, they don't need to be in a trait.
>> 
>> This way a new Zpool Registration is created with:
>> 
>> driver::Registration<zpool::Adapter>::new()
>> 
>> This also allows you to take advantage of the module_driver!() macro to provide
>> your own module_zpool_driver!() macro.
>
> There was once a problem with that but I don’t remember what the problem was :) I’ll try that one more time, thanks.

I'm pretty sure this should work out well (just like it does for driver
registrations such as PCI, platform, auxiliary, I2C, etc.). However, if you run
into issues, please let me know, I'm happy to help out resolving them.