On Mon, Feb 17, 2025 at 7:13 AM Danilo Krummrich <dakr@xxxxxxxxxx> wrote: > > On Fri, Feb 07, 2025 at 08:58:25AM -0500, Tamir Duberstein wrote: > > Allow implementors to specify the foreign pointer type; this exposes > > information about the pointed-to type such as its alignment. > > > > This requires the trait to be `unsafe` since it is now possible for > > implementors to break soundness by returning a misaligned pointer. > > > > Encoding the pointer type in the trait (and avoiding pointer casts) > > allows the compiler to check that implementors return the correct > > pointer type. This is preferable to directly encoding the alignment in > > the trait using a constant as the compiler would be unable to check it. > > > > Reviewed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx> > > Reviewed-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx> > > Reviewed-by: Fiona Behrens <me@xxxxxxxxxx> > > I know that Andreas also asked you to pick up the RBs from [1], but - without > speaking for any of the people above - given that you changed this commit after > you received all those RBs you should also consider dropping them. Especially, > since you do not mention the changes you did for this commit in the version > history. > > Just to be clear, often it is also fine to keep tags for minor changes, but then > you should make people aware of them in the version history, such that they get > the chance to double check. I will drop their RBs. > [1] https://lore.kernel.org/rust-for-linux/20250131-configfs-v1-1-87947611401c@xxxxxxxxxx/ > > > Signed-off-by: Tamir Duberstein <tamird@xxxxxxxxx> > > --- > > rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------ > > rust/kernel/miscdevice.rs | 7 ++++++- > > rust/kernel/pci.rs | 2 ++ > > rust/kernel/platform.rs | 2 ++ > > rust/kernel/sync/arc.rs | 21 ++++++++++++--------- > > rust/kernel/types.rs | 46 +++++++++++++++++++++++++++++++--------------- > > 6 files changed, 73 insertions(+), 43 deletions(-) > > > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs > > index e14433b2ab9d..f1a081dd64c7 100644 > > --- a/rust/kernel/miscdevice.rs > > +++ b/rust/kernel/miscdevice.rs > > @@ -225,13 +225,15 @@ impl<T: MiscDevice> VtableHelper<T> { > > Ok(ptr) => ptr, > > Err(err) => return err.to_errno(), > > }; > > + let ptr = ptr.into_foreign(); > > + let ptr = ptr.cast(); > > I still think that it's unnecessary to factor this out, you can just do it > inline as you did in previous versions of this patch and as this code did > before. I will do as you ask here. > > > > > // This overwrites the private data with the value specified by the user, changing the type of > > // this file's private data. All future accesses to the private data is performed by other > > // fops_* methods in this file, which all correctly cast the private data to the new type. > > // > > // SAFETY: The open call of a file can access the private data. > > - unsafe { (*raw_file).private_data = ptr.into_foreign() }; > > + unsafe { (*raw_file).private_data = ptr }; > > > > 0 > > } > > @@ -246,6 +248,7 @@ impl<T: MiscDevice> VtableHelper<T> { > > ) -> c_int { > > // SAFETY: The release call of a file owns the private data. > > let private = unsafe { (*file).private_data }; > > + let private = private.cast(); > > // SAFETY: The release call of a file owns the private data. > > let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) }; > > > > @@ -267,6 +270,7 @@ impl<T: MiscDevice> VtableHelper<T> { > > ) -> c_long { > > // SAFETY: The ioctl call of a file can access the private data. > > let private = unsafe { (*file).private_data }; > > + let private = private.cast(); > > // SAFETY: Ioctl calls can borrow the private data of the file. > > let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) }; > > > > @@ -316,6 +320,7 @@ impl<T: MiscDevice> VtableHelper<T> { > > ) { > > // SAFETY: The release call of a file owns the private data. > > let private = unsafe { (*file).private_data }; > > + let private = private.cast(); > > // SAFETY: Ioctl calls can borrow the private data of the file. > > let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) }; > > // SAFETY: > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs > > index 6c3bc14b42ad..eb25fabbff9c 100644 > > --- a/rust/kernel/pci.rs > > +++ b/rust/kernel/pci.rs > > @@ -73,6 +73,7 @@ extern "C" fn probe_callback( > > match T::probe(&mut pdev, info) { > > Ok(data) => { > > let data = data.into_foreign(); > > + let data = data.cast(); > > Same here and below, see also [2]. You're the maintainer, so I'll do what you ask here as well. I did it this way because it avoids shadowing the git history with this change, which I thought was the dominant preference. > I understand you like this style and I'm not saying it's wrong or forbidden and > for code that you maintain such nits are entirely up to you as far as I'm > concerned. > > But I also don't think there is a necessity to convert things to your preference > wherever you touch existing code. This isn't a conversion, it's a choice made specifically to avoid touching code that doesn't need to be touched (in this instance). > I already explicitly asked you not to do so in [3] and yet you did so while > keeping my ACK. :( > > (Only saying the latter for reference, no need to send a new version of [3], > otherwise I would have replied.) > > [2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/ > [3] https://lore.kernel.org/rust-for-linux/20250213-aligned-alloc-v7-1-d2a2d0be164b@xxxxxxxxx/ I will drop [2] and leave the `as _` casts in place to minimize controversy here. > > // Let the `struct pci_dev` own a reference of the driver's private data. > > // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a > > // `struct pci_dev`. > > @@ -88,6 +89,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) { > > // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a > > // `struct pci_dev`. > > let ptr = unsafe { bindings::pci_get_drvdata(pdev) }; > > + let ptr = ptr.cast(); > > > > // SAFETY: `remove_callback` is only ever called after a successful call to > > // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized > > diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs > > index dea104563fa9..53764cb7f804 100644 > > --- a/rust/kernel/platform.rs > > +++ b/rust/kernel/platform.rs > > @@ -64,6 +64,7 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff > > match T::probe(&mut pdev, info) { > > Ok(data) => { > > let data = data.into_foreign(); > > + let data = data.cast(); > > // Let the `struct platform_device` own a reference of the driver's private data. > > // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a > > // `struct platform_device`. > > @@ -78,6 +79,7 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff > > extern "C" fn remove_callback(pdev: *mut bindings::platform_device) { > > // SAFETY: `pdev` is a valid pointer to a `struct platform_device`. > > let ptr = unsafe { bindings::platform_get_drvdata(pdev) }; > > + let ptr = ptr.cast(); > > > > // SAFETY: `remove_callback` is only ever called after a successful call to > > // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized >
