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
| ||
|
Message-ID: <8096fd6e-53b7-4f26-91cf-44e6f46b6ec3@proton.me> Date: Mon, 23 Oct 2023 06:37:46 +0000 From: Benno Lossin <benno.lossin@...ton.me> To: FUJITA Tomonori <fujita.tomonori@...il.com> Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com, greg@...ah.com Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers On 23.10.23 08:35, Benno Lossin wrote: > On 21.10.23 23:45, FUJITA Tomonori wrote: >> On Sat, 21 Oct 2023 13:35:57 +0000 >> Benno Lossin <benno.lossin@...ton.me> wrote: >> >>>> Currently, it needs &'static DriverVTable >>>> array so it works. >>> >>> That is actually also incorrect. As the C side is going to modify >>> the `DriverVTable`, you should actually use `&'static mut DriverVTable`. >>> But since it is not allowed to be moved you have to use >>> `Pin<&'static mut DriverVTable>`. >> >> I updated Registration::register(). Needs to add comments on requirement? >> >> impl Registration { >> /// Registers a PHY driver. >> pub fn register( >> module: &'static crate::ThisModule, >> drivers: Pin<&'static mut [DriverVTable]>, >> ) -> Result<Self> { >> // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice >> // are initialized properly. So an FFI call with a valid pointer. > > This SAFETY comment needs to mention that `drivers[0].0.get()` are Sorry, I meant `drivers` instead of `drivers[0].0.get()` -- Cheers, Benno > pinned and will not change address. > >> to_result(unsafe { >> bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0) >> })?; >> // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`. >> Ok(Registration { drivers }) >> } >> } > > Otherwise this looks good. > >> >> >>>> The C side uses static allocation too. If someone asks for, we could >>>> loosen the restriction with a complicated implentation. But I doubt >>>> that someone would ask for such. >>> >>> With Wedson's patch you also would be using the static allocation >>> from `module!`. What my problem is, is that you are using a `static mut` >>> which is `unsafe` and you do not actually have to use it (with >>> Wedson's patch of course). >> >> Like your vtable patch, I improve the code when something useful is >> available. > > Sure. If you have the time though, it would be helpful to know > if the patch actually fixes the issue. I am pretty sure it will, > but you never know unless you try.
Powered by blists - more mailing lists