| 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
| ||
|
Message-ID: <CAJ-ks9=OG2zPPPPfZd5KhGKgNsv3Qm9iHr2eWXFeL7Zv16QVdw@mail.gmail.com>
Date: Mon, 17 Feb 2025 09:02:12 -0500
From: Tamir Duberstein <tamird@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: 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>,
Matthew Wilcox <willy@...radead.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
FUJITA Tomonori <fujita.tomonori@...il.com>, "Rob Herring (Arm)" <robh@...nel.org>,
Maíra Canal <mcanal@...lia.com>,
Asahi Lina <lina@...hilina.net>, rust-for-linux@...r.kernel.org,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pci@...r.kernel.org, Fiona Behrens <me@...enk.dev>
Subject: Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`
On Mon, Feb 17, 2025 at 7:13 AM Danilo Krummrich <dakr@...nel.org> 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@...gle.com>
> > Reviewed-by: Andreas Hindborg <a.hindborg@...nel.org>
> > Reviewed-by: Fiona Behrens <me@...enk.dev>
>
> 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@kernel.org/
>
> > Signed-off-by: Tamir Duberstein <tamird@...il.com>
> > ---
> > 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@gmail.com/
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
>
Powered by blists - more mailing lists