| 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: <DAA6ZTTNP0CM.270XX92YOFGWB@kernel.org>
Date: Sat, 31 May 2025 10:11:08 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...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 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)
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?
Another thing: faux devices don't have a `probe` in rust, so saying "not
required" doesn't make much sense, right?
>> > + // 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