Implement TryFrom<&device::Device> for &Device.
This allows us to get a &pci::Device from a generic &Device in a safe
way; the conversion fails if the device' bus type does not match with
the PCI bus type.
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
rust/kernel/device.rs | 1 -
rust/kernel/pci.rs | 21 +++++++++++++++++++--
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 1b554fdd93e9..190719a04686 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -81,7 +81,6 @@ pub fn parent<'a>(&self) -> Option<&'a Self> {
}
/// Returns a raw pointer to the device' bus type.
- #[expect(unused)]
pub(crate) fn bus_type_raw(&self) -> *const bindings::bus_type {
// SAFETY:
// - By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 22a32172b108..b717bcdb9abf 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -6,7 +6,7 @@
use crate::{
alloc::flags::*,
- bindings, device,
+ bindings, container_of, device,
device_id::RawDeviceId,
devres::Devres,
driver,
@@ -20,7 +20,7 @@
use core::{
marker::PhantomData,
ops::Deref,
- ptr::{addr_of_mut, NonNull},
+ ptr::{addr_of, addr_of_mut, NonNull},
};
use kernel::prelude::*;
@@ -466,6 +466,23 @@ fn as_ref(&self) -> &device::Device {
}
}
+impl TryFrom<&device::Device> for &Device {
+ type Error = kernel::error::Error;
+
+ fn try_from(dev: &device::Device) -> Result<Self, Self::Error> {
+ if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
+ return Err(EINVAL);
+ }
+
+ // SAFETY: We've just verified that the bus type of `dev` equals `bindings::pci_bus_type`,
+ // hence `dev` must be embedded in a valid `struct pci_dev`.
+ let pdev = unsafe { container_of!(dev.as_raw(), bindings::pci_dev, dev) };
+
+ // SAFETY: `pdev` is a valid pointer to a `struct pci_dev`.
+ Ok(unsafe { &*pdev.cast() })
+ }
+}
+
// SAFETY: A `Device` is always reference-counted and can be released from any thread.
unsafe impl Send for Device {}
--
2.48.1Hi Danilo,
kernel test robot noticed the following build errors:
[auto build test ERROR on 51d0de7596a458096756c895cfed6bc4a7ecac10]
url: https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/rust-device-implement-Device-parent/20250321-063101
base: 51d0de7596a458096756c895cfed6bc4a7ecac10
patch link: https://lore.kernel.org/r/20250320222823.16509-4-dakr%40kernel.org
patch subject: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device
config: x86_64-buildonly-randconfig-005-20250321 (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/config)
compiler: clang version 20.1.1 (https://github.com/llvm/llvm-project 424c2d9b7e4de40d0804dd374721e6411c27d1d1)
rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503220040.TDePlxma-lkp@intel.com/
All errors (new ones prefixed by >>):
***
*** Rust bindings generator 'bindgen' < 0.69.5 together with libclang >= 19.1
*** may not work due to a bug (https://github.com/rust-lang/rust-bindgen/pull/2824),
*** unless patched (like Debian's).
*** Your bindgen version: 0.65.1
*** Your libclang version: 20.1.1
***
***
*** Please see Documentation/rust/quick-start.rst for details
*** on how to set up the Rust support.
***
>> error[E0133]: use of extern static is unsafe and requires unsafe block
--> rust/kernel/pci.rs:473:43
|
473 | if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
| ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
|
= note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri Mar 21, 2025 at 5:56 PM CET, kernel test robot wrote:
> Hi Danilo,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on 51d0de7596a458096756c895cfed6bc4a7ecac10]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/rust-device-implement-Device-parent/20250321-063101
> base: 51d0de7596a458096756c895cfed6bc4a7ecac10
> patch link: https://lore.kernel.org/r/20250320222823.16509-4-dakr%40kernel.org
> patch subject: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device
> config: x86_64-buildonly-randconfig-005-20250321 (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/config)
> compiler: clang version 20.1.1 (https://github.com/llvm/llvm-project 424c2d9b7e4de40d0804dd374721e6411c27d1d1)
> rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202503220040.TDePlxma-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> ***
> *** Rust bindings generator 'bindgen' < 0.69.5 together with libclang >= 19.1
> *** may not work due to a bug (https://github.com/rust-lang/rust-bindgen/pull/2824),
> *** unless patched (like Debian's).
> *** Your bindgen version: 0.65.1
> *** Your libclang version: 20.1.1
> ***
> ***
> *** Please see Documentation/rust/quick-start.rst for details
> *** on how to set up the Rust support.
> ***
>>> error[E0133]: use of extern static is unsafe and requires unsafe block
> --> rust/kernel/pci.rs:473:43
> |
> 473 | if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
> | ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
> |
> = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior
This is just a minor annoyance with these error reports, but I would
very much like the error to have the correct indentation:
>> error[E0133]: use of extern static is unsafe and requires unsafe block
--> rust/kernel/pci.rs:473:43
|
473 | if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
| ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
|
Would that be possible to fix? That way the `^^^^` align with the item
they are referencing.
Otherwise they are very helpful!
---
Cheers,
Benno
On Sat, Mar 22, 2025 at 12:56:58AM +0800, kernel test robot wrote:
> >> error[E0133]: use of extern static is unsafe and requires unsafe block
> --> rust/kernel/pci.rs:473:43
> |
> 473 | if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
> | ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
> |
> = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior
This requires an unsafe block for compilers < 1.82. For compilers >= 1.82 it
turns into a warning *if* using an unsafe block.
*Not* requiring unsafe for this seems like the correct thing -- was this a
bugfix in the compiler?
I guess to make it work for all compiler versions supported by the kernel we
have to use unsafe and suppress the warning?
On Fri, Mar 21, 2025 at 6:44 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> This requires an unsafe block for compilers < 1.82. For compilers >= 1.82 it
> turns into a warning *if* using an unsafe block.
>
> *Not* requiring unsafe for this seems like the correct thing -- was this a
> bugfix in the compiler?
>
> I guess to make it work for all compiler versions supported by the kernel we
> have to use unsafe and suppress the warning?
It was a feature, but it has been fairly annoying -- it affected
several series, e.g. the latest KUnit one as well as:
https://lore.kernel.org/rust-for-linux/CANiq72kuebpOa4aPxmTXNMA0eo-SLL+Ht9u1SGHymXBF5_92eA@mail.gmail.com/
Please see:
https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics
So, yeah, we use `allow(unused_unsafe)` (no `expect`, since it depends
on the version).
I hope that helps.
Cheers,
Miguel
On Fri, Mar 21, 2025 at 07:59:08PM +0100, Miguel Ojeda wrote: > On Fri, Mar 21, 2025 at 6:44 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > This requires an unsafe block for compilers < 1.82. For compilers >= 1.82 it > > turns into a warning *if* using an unsafe block. > > > > *Not* requiring unsafe for this seems like the correct thing -- was this a > > bugfix in the compiler? > > > > I guess to make it work for all compiler versions supported by the kernel we > > have to use unsafe and suppress the warning? > > It was a feature, but it has been fairly annoying -- it affected > several series, e.g. the latest KUnit one as well as: From the second link: "Previously, the compiler's safety checks were not aware that the raw ref operator did not actually affect the operand's place, treating it as a possible read or write to a pointer. No unsafety is actually present, however, as it just creates a pointer. That sounds like it was a bug, or do I miss anything? > > https://lore.kernel.org/rust-for-linux/CANiq72kuebpOa4aPxmTXNMA0eo-SLL+Ht9u1SGHymXBF5_92eA@mail.gmail.com/ > > Please see: > > https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics > > So, yeah, we use `allow(unused_unsafe)` (no `expect`, since it depends > on the version). > > I hope that helps. Yeah, thanks a lot. Especially for the second link, I couldn't find it even after quite a while of searching. I will respin right away, since otherwise the patches of v3 are reviewed.
On Fri, Mar 21, 2025 at 8:11 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> From the second link:
>
> "Previously, the compiler's safety checks were not aware that the raw ref
> operator did not actually affect the operand's place, treating it as a possible
> read or write to a pointer. No unsafety is actually present, however, as it just
> creates a pointer.
>
> That sounds like it was a bug, or do I miss anything?
Yeah, if they didn't intend it originally, then I would call it a bug
-- they also seemed to considered it a bug themselves in the end, so I
think you are right.
I meant it from the point of view of the language, i.e. in the sense
that it was a restriction before, and now they relaxed it, so more
programs are accepted, and the feature would be "you need less
`unsafe`" etc.
The blog post seems to mention both sides of the coin ("This code is
now allowed", "Relaxed this", "A future version of Rust is expected to
generalize this").
> Yeah, thanks a lot. Especially for the second link, I couldn't find it even
> after quite a while of searching.
You're welcome!
Cheers,
Miguel
On Thu Mar 20, 2025 at 11:27 PM CET, Danilo Krummrich wrote:
> @@ -466,6 +466,23 @@ fn as_ref(&self) -> &device::Device {
> }
> }
>
> +impl TryFrom<&device::Device> for &Device {
> + type Error = kernel::error::Error;
> +
> + fn try_from(dev: &device::Device) -> Result<Self, Self::Error> {
> + if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
> + return Err(EINVAL);
> + }
> +
> + // SAFETY: We've just verified that the bus type of `dev` equals `bindings::pci_bus_type`,
> + // hence `dev` must be embedded in a valid `struct pci_dev`.
I think it'd be a good idea to mention that this is something guaranteed
by the C side. Something like "... must be embedded in a valid `struct
pci_dev` by the C side." or similar.
With that:
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
---
Cheers,
Benno
> + let pdev = unsafe { container_of!(dev.as_raw(), bindings::pci_dev, dev) };
> +
> + // SAFETY: `pdev` is a valid pointer to a `struct pci_dev`.
> + Ok(unsafe { &*pdev.cast() })
> + }
> +}
> +
> // SAFETY: A `Device` is always reference-counted and can be released from any thread.
> unsafe impl Send for Device {}
>
On Thu, Mar 20, 2025 at 11:44:01PM +0000, Benno Lossin wrote:
> On Thu Mar 20, 2025 at 11:27 PM CET, Danilo Krummrich wrote:
> > @@ -466,6 +466,23 @@ fn as_ref(&self) -> &device::Device {
> > }
> > }
> >
> > +impl TryFrom<&device::Device> for &Device {
> > + type Error = kernel::error::Error;
> > +
> > + fn try_from(dev: &device::Device) -> Result<Self, Self::Error> {
> > + if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
> > + return Err(EINVAL);
> > + }
> > +
> > + // SAFETY: We've just verified that the bus type of `dev` equals `bindings::pci_bus_type`,
> > + // hence `dev` must be embedded in a valid `struct pci_dev`.
>
> I think it'd be a good idea to mention that this is something guaranteed
> by the C side. Something like "... must be embedded in a valid `struct
> pci_dev` by the C side." or similar.
Sure, sounds reasonable.
>
> With that:
>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
>
> ---
> Cheers,
> Benno
>
> > + let pdev = unsafe { container_of!(dev.as_raw(), bindings::pci_dev, dev) };
> > +
> > + // SAFETY: `pdev` is a valid pointer to a `struct pci_dev`.
> > + Ok(unsafe { &*pdev.cast() })
> > + }
> > +}
> > +
> > // SAFETY: A `Device` is always reference-counted and can be released from any thread.
> > unsafe impl Send for Device {}
> >
>
>
© 2016 - 2025 Red Hat, Inc.