| 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: <aDrZ7ma_aNki3FRz@pollux>
Date: Sat, 31 May 2025 12:29:02 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <lossin@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org,
alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
chrisi.schrefl@...il.com, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/7] rust: sample: misc: implement device driver sample
On Sat, May 31, 2025 at 10:11:08AM +0200, Benno Lossin wrote:
> On Sat May 31, 2025 at 12:24 AM CEST, Danilo Krummrich wrote:
> > On Fri, May 30, 2025 at 10:15:37PM +0200, Benno Lossin wrote:
> >> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> >> > In order to demonstrate and test a MiscDeviceRegistration with a parent
> >> > device, introduce CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT.
> >> >
> >> > If CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT=y the misc device sample
> >> > is initialized with a parent device (faux), otherwise it is initialized
> >> > without a parent device, i.e. the exact same way as without this patch.
> >> >
> >> > Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> >> > ---
> >> > samples/rust/Kconfig | 8 +++++
> >> > samples/rust/rust_misc_device.rs | 50 +++++++++++++++++++++++++++++---
> >> > 2 files changed, 54 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> >> > index b1006ab4bc3c..9948ec0939ef 100644
> >> > --- a/samples/rust/Kconfig
> >> > +++ b/samples/rust/Kconfig
> >> > @@ -30,6 +30,14 @@ config SAMPLE_RUST_MISC_DEVICE
> >> >
> >> > If unsure, say N.
> >> >
> >> > +config SAMPLE_RUST_MISC_DEVICE_WITH_PARENT
> >> > + bool "Create a misc device with a parent device"
> >> > + depends on SAMPLE_RUST_MISC_DEVICE
> >> > + default n
> >> > + help
> >> > + Say Y here if you want the misc device sample to create a misc
> >> > + device with a parent device.
> >> > +
> >>
> >> Why not create a separate file? The `cfg`s might confuse newcomers
> >> looking at the sample.
> >
> > It would be a lot of duplicated code, unless we really *only* exercise the
> > device creation and registration part, which would be a bit unfortunate, given
> > that this sample is also a pretty good test.
>
> We could separate the common parts into a single file and then
> `include!` that file from the two samples. (Or if the build system
> supports multi-file samples then just use that, but my gut feeling is
> that it doesn't)
The samples are normal modules, where we can have multiple files. But I don't
see how that helps.
`include!` works, but I'm not sure it's that much better.
Another option would be to put the `cfg` on the module!() macro itself and have
two separate module types, this way there is only a `cfg` on the two module!()
invocations.
>
> I really would like to avoid `cfg` in samples.
>
> >> > config SAMPLE_RUST_PRINT
> >> > tristate "Printing macros"
> >> > help
> >> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> >> > index 9bf1a0f64e6e..175638d6d341 100644
> >> > --- a/samples/rust/rust_misc_device.rs
> >> > +++ b/samples/rust/rust_misc_device.rs
> >> > @@ -167,6 +167,9 @@
> >> > uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> >> > };
> >> >
> >> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> >> > +use kernel::faux;
> >> > +
> >> > const RUST_MISC_DEV_HELLO: u32 = _IO('|' as u32, 0x80);
> >> > const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('|' as u32, 0x81);
> >> > const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('|' as u32, 0x82);
> >> > @@ -181,19 +184,33 @@
> >> > license: "GPL",
> >> > }
> >> >
> >> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> >> > #[pin_data]
> >> > struct RustMiscDeviceModule {
> >> > #[pin]
> >> > _miscdev: MiscDeviceRegistration<RustMiscDevice>,
> >> > }
> >> >
> >> > -impl kernel::InPlaceModule for RustMiscDeviceModule {
> >> > - fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> >> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> >> > +struct RustMiscDeviceModule {
> >> > + _faux: faux::Registration,
> >> > + _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> >> > +}
> >> > +
> >> > +impl RustMiscDeviceModule {
> >> > + fn init() -> MiscDeviceOptions {
> >> > pr_info!("Initializing Rust Misc Device Sample\n");
> >> >
> >> > - let options = MiscDeviceOptions {
> >> > + MiscDeviceOptions {
> >> > name: c_str!("rust-misc-device"),
> >> > - };
> >> > + }
> >> > + }
> >> > +}
> >> > +
> >> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> >> > +impl kernel::InPlaceModule for RustMiscDeviceModule {
> >> > + fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> >> > + let options = Self::init();
> >> >
> >> > try_pin_init!(Self {
> >> > _miscdev <- MiscDeviceRegistration::register(
> >> > @@ -205,6 +222,31 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> >> > }
> >> > }
> >> >
> >> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> >> > +impl kernel::Module for RustMiscDeviceModule {
> >> > + fn init(_module: &'static ThisModule) -> Result<Self> {
> >> > + let options = Self::init();
> >> > + let faux = faux::Registration::new(c_str!("rust-misc-device-sample"), None)?;
> >> > +
> >> > + // For every other bus, this would be called from Driver::probe(), which would return a
>
> Missing '`' around Driver::probe().
>
> >> > + // `Result<Pin<KBox<T>>>`, but faux always binds to a "dummy" driver, hence probe() is
> >>
> >> Not clear what `T` is supposed to be, do you mean `Self`?
> >
> > From the perspective of the type implementing the corresponding Driver trait it
> > would indeed be `Self`. But I found it ambiguous to write `Self`, since I do *not*
> > mean `RustMiscDeviceModule` with `Self`.
>
> Yeah that makes sense, I already entered into the `impl Driver` context
> :) How about we use `<T as Driver>::probe()` above and then `T` makes
> sense?
Yep, that sounds good.
> Another thing: faux devices don't have a `probe` in rust, so saying "not
> required" doesn't make much sense, right?
In Rust, faux does not have probe() indeed, but that's because it's "not
required"; I can't think of a use-case for a new driver (yet), where this isn't
just unnecessary overhead.
> >> > + // not required.
> >> > + let misc = KBox::pin_init(
> >> > + MiscDeviceRegistration::register(
> >> > + options,
> >> > + Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
> >> > + Some(faux.as_ref()),
> >> > + ),
> >> > + GFP_KERNEL,
> >> > + )?;
> >>
> >> You could also initialize this module variation in-place. (this would
> >> also require the pin-init change to reference initialized fields)
> >
> > Yes, I also thought about that. But this way is a bit closer to what things
> > would look like within a probe() callback.
>
> Yeah then let's do that :)
>
> ---
> Cheers,
> Benno
Powered by blists - more mailing lists