linux-kernel - Re: [PATCH v2] rust: add `module

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALNs47tLRa4uVU8t8w6=oi+sMPxEVdw5GQdLwpatc-kEREQg6A@mail.gmail.com>
Date: Sat, 24 Aug 2024 16:23:40 -0500
From: Trevor Gross <tmgross@...ch.edu>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: Andreas Hindborg <nmi@...aspace.dk>, Luis Chamberlain <mcgrof@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, 
	Andreas Hindborg <a.hindborg@...sung.com>, Adam Bratschi-Kaye <ark.email@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Alice Ryhl <aliceryhl@...gle.com>, Daniel Gomez <da.gomez@...sung.com>, 
	rust-for-linux@...r.kernel.org, linux-modules@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rust: add `module_params` macro

On Sat, Aug 24, 2024 at 8:16 AM Benno Lossin <benno.lossin@...ton.me> wrote:
> > We shouldn't enable `const_mut_refs`. It is indeed close to
> > stabilization, but it is still kind of churny right now and we don't
> > want to enable the sharp edges everywhere.
> >
> > If the change from `static mut` to `UnsafeCell` that I mentioned above
> > happens, `addr_of_mut!` turns into a `.get().cast::<...>()` takes the
> > place of `addr_of_mut!` and doesn't require this feature (and also
> > isn't unsafe).
>
> I think this is a good idea. There might only be a problem with not
> being `Sync` though... So probably need to use `SyncUnsafeCell` instead.

Ah whoops, yeah that is what I meant.

> > If you prefer not to make that change, I think
> > `addr_of!(...).cast_mut()` might be the best solution.
>
> Won't that be resulting in the wrong provenance? I.e. the pointer won't
> be allowed to write to that location?
>
> I just checked with miri, it doesn't complain (even with
> `strict-provenance`), so I guess it's fine? It feels rather wrong to me
> to allow writing through a pointer obtained via `addr_of!`.

I think that `static mut` gets the interior mutability rules that
`UnsafeCell` has, that *const and *mut become interchangeable. Quick
demo for the `UnsafeCell` at [1]. We would probably have to ask opsem
to clarify.

Coincidentally I had been talking to Ralf about this very pattern
before seeing this, at [2].

> > Other thought: would a wrapper type make more sense here? Something like this:
> >
> > ```
> > /* implementation */
> > struct ModParam<T>(UnsafeCell<T>);
> > [...]
> > module! {
> >     // ...
> >     // instantiated as:
> >     // `static MY_PARAM: ModParam<i64> = ModParam::new(1);`
>
> We used to do this, but it lead to problems: normally the parameter has
> a lower case name, since that's the convention in the kernel. [...]

To me it seemed logical to keep the uppercase names here since it
matches the convention for what they are (statics), and the macro
could lowercase it for the bits exposed to the kernel. But I
absolutely get the consistency argument here.

> [...] But then
> pattern matching prioritises the static instead of introducing it as a
> local parameter:
>
>     let MY_PARAM = ...;
>
> would fail, since you can't match MY_PARAM.
>
> This is also the reason why they live in their own module.

I'm not sure I follow the example here. It looks like it is shadowing
a static's name as a local, why would you want that? Or if you meant
the other way around `let SomePat(...) = MY_PARAM`, wouldn't it just
be `let SomePat(...) = MY_PARAM.get()`? (Sorry if I missed some
context here).

> But you can still do the modification of creating `ModParam` and using
> that as the type of the static.

Do you mean an expansion like this?

    // module_parameters is kind of a long name
    mod mod_params {
        #[allow(non_upper_case_globals)]
       static my_param: ModParam<i32> = ModParam::new(...);
    }

I don't mind that, even if the name is a bit weird by rust conventions.

(For what it's worth, I used this wrapper type pattern for a plugin
project that does shared variables in a similar way. I have been quite
happy with it.)

- Trevor

[1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=43664620f50384b7a3d5bf74ce7c3e39
[2]: https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/More.20accidental.20stabilization