Received-SPF: pass (google.com: domain of linux-kernel+bounces-127123-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Feedback-ID: iad51458e:Fastmail
Date: Mon, 1 Apr 2024 15:17:53 -0700
From: Boqun Feng <boqun.feng@gmail.com>
To: Benno Lossin <benno.lossin@proton.me>
Cc: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>,
	Wedson Almeida Filho <wedsonaf@gmail.com>,
	Gary Guo <gary@garyguo.net>,
	=?iso-8859-1?Q?Bj=F6rn?= 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>,
	Sumera Priyadarsini <sylphrenadin@gmail.com>,
	Neal Gompa <neal@gompa.dev>,
	Thomas Bertschinger <tahbertschinger@gmail.com>,
	Andrea Righi <andrea.righi@canonical.com>,
	Matthew Bakhtiari <dev@mtbk.me>,
	Adam Bratschi-Kaye <ark.email@gmail.com>, stable@vger.kernel.org,
	Masahiro Yamada <masahiroy@kernel.org>,
	Wedson Almeida Filho <wedsonaf@google.com>,
	Finn Behrens <me@kloenk.dev>, rust-for-linux@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] rust: macros: fix soundness issue in `module!` macro
Message-ID: <ZgsykVwMBsULtxce@boqun-archlinux>
References: <20240401185222.12015-1-benno.lossin@proton.me>
 <Zgsiumknfshjbi9J@boqun-archlinux>
 <20fcbbd0-4a7a-49b1-a383-f8b388153066@proton.me>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20fcbbd0-4a7a-49b1-a383-f8b388153066@proton.me>

On Mon, Apr 01, 2024 at 10:01:34PM +0000, Benno Lossin wrote:
> On 01.04.24 23:10, Boqun Feng wrote:
> > On Mon, Apr 01, 2024 at 06:52:50PM +0000, Benno Lossin wrote:
> > [...]
> >> +            // Double nested modules, since then nobody can access the public items inside.
> >> +            mod __module_init {{
> >> +                mod __module_init {{
> >> +                    use super::super::{type_};
> >> +
> >> +                    /// The \"Rust loadable module\" mark.
> >> +                    //
> >> +                    // This may be best done another way later on, e.g. as a new modinfo
> >> +                    // key or a new section. For the moment, keep it simple.
> >> +                    #[cfg(MODULE)]
> >> +                    #[doc(hidden)]
> >> +                    #[used]
> >> +                    static __IS_RUST_MODULE: () = ();
> >> +
> >> +                    static mut __MOD: Option<{type_}> = None;
> >> +
> >> +                    // SAFETY: `__this_module` is constructed by the kernel at load time and will not be
> >> +                    // freed until the module is unloaded.
> >> +                    #[cfg(MODULE)]
> >> +                    static THIS_MODULE: kernel::ThisModule = unsafe {{
> >> +                        kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _)
> > 
> > While we're at it, probably we want the following as well? I.e. using
> > `Opaque` and extern block, because __this_module is certainly something
> > interior mutable and !Unpin.
> > 
> > diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> > index 293beca0a583..8aa4eed6578c 100644
> > --- a/rust/macros/module.rs
> > +++ b/rust/macros/module.rs
> > @@ -219,7 +219,11 @@ mod __module_init {{
> >                       // freed until the module is unloaded.
> >                       #[cfg(MODULE)]
> >                       static THIS_MODULE: kernel::ThisModule = unsafe {{
> > -                        kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _)
> > +                        extern \"C\" {{
> > +                            static __this_module: kernel::types::Opaque<kernel::bindings::module>;
> > +                        }}
> > +
> > +                        kernel::ThisModule::from_ptr(__this_module.get())
> >                       }};
> >                       #[cfg(not(MODULE))]
> >                       static THIS_MODULE: kernel::ThisModule = unsafe {{
> > 
> > Thoughts?
> 
> I am not sure we need it. Bindgen generates
> 
>      extern "C" {
>          pub static mut __this_module: module;
>      }
> 
> And the `mut` should take care of the "it might be modified by other
> threads".

Hmm.. but there could a C thread modifies some field of __this_module
while Rust code uses it, e.g. struct module has a list_head in it, which
could be used by C code to put another module next to it.

> The only thing that sticks out to me is the borrow, it should probably
> be using `addr_of_mut!` instead. Then we don't need to re-import it
> again manually.
> 
> I think it should be a separate patch though.
> 

Yes, agreed.

Regards,
Boqun

> -- 
> Cheers,
> Benno
> 
> > 
> > Note this requires `Opaque::get` to be `const`, which I will send out
> > shortly, I think it's a good change regardless of the usage here.
> > 
> > Regards,
> > Boqun
> > 
>