Message-Id: <DB1HDQS17VOQ.YDFIYGP0NH7K@kernel.org>
Date: Wed, 02 Jul 2025 12:01:28 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Andreas Hindborg" <a.hindborg@...nel.org>, "Miguel Ojeda"
 <miguel.ojeda.sandonis@...il.com>
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>, "Alice Ryhl" <aliceryhl@...gle.com>, "Masahiro
 Yamada" <masahiroy@...nel.org>, "Nathan Chancellor" <nathan@...nel.org>,
 "Luis Chamberlain" <mcgrof@...nel.org>, "Danilo Krummrich"
 <dakr@...nel.org>, "Nicolas Schier" <nicolas.schier@...ux.dev>, "Trevor
 Gross" <tmgross@...ch.edu>, "Adam Bratschi-Kaye" <ark.email@...il.com>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-kbuild@...r.kernel.org>, "Petr Pavlu" <petr.pavlu@...e.com>, "Sami
 Tolvanen" <samitolvanen@...gle.com>, "Daniel Gomez" <da.gomez@...sung.com>,
 "Simona Vetter" <simona.vetter@...ll.ch>, "Greg KH"
 <gregkh@...uxfoundation.org>, "Fiona Behrens" <me@...enk.dev>, "Daniel
 Almeida" <daniel.almeida@...labora.com>, <linux-modules@...r.kernel.org>
Subject: Re: [PATCH v13 2/6] rust: introduce module_param module

On Wed Jul 2, 2025 at 10:26 AM CEST, Andreas Hindborg wrote:
> "Miguel Ojeda" <miguel.ojeda.sandonis@...il.com> writes:
>
>> On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@...nel.org> wrote:
>>>
>>> Ultimately this is something for Miguel to decide.
>>
>> Only if you all cannot get to an agreement ;)
>
>>
>> If Andreas wants to have it already added, then I would say just mark
>> it `unsafe` as Benno recommends (possibly with an overbearing
>> precondition), given it has proven subtle/forgettable enough and that,
>> if I understand correctly, it would actually become unsafe if someone
>> "just" added "reasonably-looking code" elsewhere.
>
> You are right that if someone added code to the API, the API could
> become unsound. But that is the deal with all our APIs and I don't agree
> that the details are very subtle here. Someone would need to add sysfs
> support or user provided parameter parsing to cause the unsoundness we
> are talking about.

Normally the safety requirements & invariants are *local*. We do have
some global ones, but this one would be another one. And it's not easy
to control IMO (no code is allowed to run before parameter parsing
finished!).

> Anyone attempting such a task should have proper understanding of the
> code first, and given the ample amount of `NOTE` comments I have added,
> it should be clear that the concurrent accesses that this addition would
> introduce, needs to be accounted for, to avoid data races.

Well if I add a way to add a task to a work queue before parameter
parsing, I don't need to touch this file or even know about it.

> I will add myself as a reviewer for the rust module parameter parsing
> code if that is OK with module maintainers.

I think it's a good idea, but it is orthogonal and doesn't address the
issue, since any tree can merge code that breaks the invariant above.

>> That way we have an incentive to make it safe later on and, more
>> importantly, to think again about it when such a patch lands,
>> justifying it properly. And it could plausibly protect out-of-tree
>> users, too.
>
> Again, I do not think it is reasonable to mark this function unsafe.

We mark it `unsafe` only until atomics are merged and then we make it
safe.

You proposed to do it the other way and make it safe, though possibly
unsound when someone adds code breaking the invariant and making it
fully sound later.

I don't think we should have global invariants that are temporary.

---
Cheers,
Benno

Powered by blists - more mailing lists