Received-SPF: pass (google.com: domain of linux-kernel+bounces-126989-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 01 Apr 2024 19:27:52 +0000
To: Wedson Almeida Filho <wedsonaf@gmail.com>
From: Benno Lossin <benno.lossin@proton.me>
Cc: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, Andreas Hindborg <a.hindborg@samsung.com>, Alice Ryhl <aliceryhl@google.com>, Martin Rodriguez Reboredo <yakoyoku@gmail.com>, Asahi Lina <lina@asahilina.net>, Eric Curtin <ecurtin@redhat.com>, Neal Gompa <neal@gompa.dev>, Thomas Bertschinger <tahbertschinger@gmail.com>, Andrea Righi <andrea.righi@canonical.com>, Sumera Priyadarsini <sylphrenadin@gmail.com>, Finn Behrens <me@kloenk.dev>, Adam Bratschi-Kaye <ark.email@gmail.com>, stable@vger.kernel.org, Daniel Xu <dxu@dxuuu.xyz>, rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rust: macros: fix soundness issue in `module!` macro
Message-ID: <06d5ccdb-a5c3-4630-9f97-9a7bdf7b7a48@proton.me>
In-Reply-To: <CANeycqr_AkxTj2iNdnjRFrC-C8npsBtS34V4hNy35RpQHszG9w@mail.gmail.com>
References: <20240327160346.22442-1-benno.lossin@proton.me> <CANeycqp0o-HKBx6nuGCy9DD6mAwoGWzTR6bm5ceajsUhKcZuQg@mail.gmail.com> <d41d123e-d682-4685-88f5-e45567cc1975@proton.me> <CANeycqr_AkxTj2iNdnjRFrC-C8npsBtS34V4hNy35RpQHszG9w@mail.gmail.com>
Feedback-ID: 71780778:user:proton
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 01.04.24 21:10, Wedson Almeida Filho wrote:
> On Sun, 31 Mar 2024 at 07:27, Benno Lossin <benno.lossin@proton.me> wrote=
:
>>
>> On 31.03.24 03:00, Wedson Almeida Filho wrote:
>>> On Wed, 27 Mar 2024 at 13:04, Benno Lossin <benno.lossin@proton.me> wro=
te:
>>>> -            fn __init() -> core::ffi::c_int {{
>>>> -                match <{type_} as kernel::Module>::init(&THIS_MODULE)=
 {{
>>>> -                    Ok(m) =3D> {{
>>>> +                    /// # Safety
>>>> +                    ///
>>>> +                    /// This function must
>>>> +                    /// - only be called once,
>>>> +                    /// - be called after `__init`,
>>>> +                    /// - not be called concurrently with `__init`.
>>>
>>> The second item is incomplete: it must be called after `__init` *succee=
ds*.
>>
>> Indeed.
>>
>>>
>>> With that added (which is a different precondition), I think the third
>>> item can be dropped because if you have to wait to see whether
>>> `__init` succeeded or failed before you can call `__exit`, then
>>> certainly you cannot call it concurrently with `__init`.
>>
>> I would love to drop that requirement, but I am not sure we can. With
>> that requirement, I wanted to ensure that no data race on `__MOD` can
>> happen. If you need to verify that `__init` succeeded, one might think
>> that it is not possible to call `__exit` such that a data race occurs,
>> but I think it could theoretically be done if the concrete `Module`
>> implementation never failed.
>=20
> I see. If you're concerned about compiler reordering, then we need
> compiler barriers.
>=20
>> Do you have any suggestion for what I could add to the "be called after
>> `__init` was called and returned `0`" requirement to make a data race
>> impossible?
>=20
> If you're concerned with reordering from the processor as well, then
> we need cpu barriers. You'd have to say that the cpu/thread executing
> `__init` must have a release barrier after `__init` completes, and the
> thread/cpu doing `__exit` must have an acquire barrier before starting
> `__exit`.
>=20
> But I'm not sure we need to go that far. Mostly because C is going to
> guarantee that ordering for us, so I'd say we can just omit this or
> perhaps say "This function must only be called from the exit module
> implementation".

Yeah, though I do not exactly know where or what the "exit module
implementation" is. If you are happy with v2, then I think we can go
with that. This piece of code is also not really something people will
need to read.

--=20
Cheers,
Benno