From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-40134.protonmail.ch (mail-40134.protonmail.ch [185.70.40.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B500F6E602 for ; Tue, 2 Apr 2024 12:47:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.70.40.134 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712062064; cv=none; b=TLe7FXP8wkWUKfoPheOtTFz+TfrVoPvpo2t+iHpnANm7G3Dw7O9F5iuglK7U3GDp+PuxwkoLlEQWIkeH99nMKRtFfopLCTPcnT7VJ7pmmg6PL+Cup6aCKzvi/iTFsKwsh47jSRSSCl3F4lBRcr/X9KBrCJO9eseUQgE/mlcr7EY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712062064; c=relaxed/simple; bh=tv77wEY2gXIZxwexHBZDtQXMh//LD9JALwsTToG42s0=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=p8T7ZtUXEYjxXHjYy1YivlldfV56+1JYEVItdgIfv0ZBytb6nKPjNKo1BGO6/F3I5xsqVReCyqd9FjXBlxL6u/xtBqv0p97rRxHWTUjIuOiUAv2O9oCuJfsCvjP1RIFlzyZObMHOW+Jy5fjJ0zOWtKksUV8MOvZCJ3TgNiukPZI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=proton.me; spf=pass smtp.mailfrom=proton.me; dkim=pass (2048-bit key) header.d=proton.me header.i=@proton.me header.b=TG+kzr3T; arc=none smtp.client-ip=185.70.40.134 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=proton.me Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=proton.me Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=proton.me header.i=@proton.me header.b="TG+kzr3T" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1712062059; x=1712321259; bh=9xyuTCGaaAe31ycaYB+ZYD6nsgUvzbGxycpDYEU663s=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=TG+kzr3TwsqWUcLbtWZ6PfdIh3iPg/oJ5W4qWGbI0K5IiRAjwggyJuXOQnxm+8hSL YmiNT9OyPZGO61rWBLv+7aeZWLe5yrw373XGF4ufEKO5MCANKNmiu6wDwEvg6w8/h6 a6djz9Ga2cVYRPd3PuVtvchroM2sSLb9HWbwCqqaFivjh9zgp1B2OZtxlbo2AlO9uU Wbzo6C/viRR8gBHPlTcknRsSdec9oJQssEV2hAeCtjb2eXtqxiZRUkbcgTTVQpTabK AEQzqdoESVFRa8ccs+00opCqXH2EQfYMEJQB0scv77yNeh5HQN0g0vMkjCeHyHVhTW l2mEfF8g2ZdaQ== Date: Tue, 02 Apr 2024 12:47:34 +0000 To: Boqun Feng , Gary Guo From: Benno Lossin Cc: Miguel Ojeda , Alex Gaynor , Wedson Almeida Filho , =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= , Andreas Hindborg , Alice Ryhl , Martin Rodriguez Reboredo , Asahi Lina , Sumera Priyadarsini , Neal Gompa , Thomas Bertschinger , Andrea Righi , Matthew Bakhtiari , Adam Bratschi-Kaye , stable@vger.kernel.org, Masahiro Yamada , Wedson Almeida Filho , Finn Behrens , 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: In-Reply-To: References: <20240401185222.12015-1-benno.lossin@proton.me> <20fcbbd0-4a7a-49b1-a383-f8b388153066@proton.me> Feedback-ID: 71780778:user:proton Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02.04.24 00:17, Boqun Feng wrote: > 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 th= e 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: () =3D (); >>>> + >>>> + static mut __MOD: Option<{type_}> =3D 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 =3D unsafe= {{ >>>> + kernel::ThisModule::from_ptr(&kernel::binding= s::__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 =3D unsaf= e {{ >>> - kernel::ThisModule::from_ptr(&kernel::bindings= ::__this_module as *const _ as *mut _) >>> + extern \"C\" {{ >>> + static __this_module: kernel::types::Opaqu= e; >>> + }} >>> + >>> + kernel::ThisModule::from_ptr(__this_module.get= ()) >>> }}; >>> #[cfg(not(MODULE))] >>> static THIS_MODULE: kernel::ThisModule =3D unsaf= e {{ >>> >>> 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". >=20 > 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. This still should not be a problem, since we never actually read or write to the mutable static. The only thing we are doing is taking its address. `addr_of_mut!` should be sufficient. (AFAIK `static mut` is designed such that it can be mutated at any time by any thread. Maybe Gary knows more?) --=20 Cheers, Benno