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: <d8b23faa-4041-4789-ae96-5d8bf87070ad@proton.me> Date: Sat, 21 Oct 2023 12:13:32 +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 21.10.23 13:36, FUJITA Tomonori wrote: > On Sat, 21 Oct 2023 11:21:12 +0000 > Benno Lossin <benno.lossin@...ton.me> wrote: > >> On 21.10.23 12:27, FUJITA Tomonori wrote: >>> On Sat, 21 Oct 2023 08:37:08 +0000 >>> Benno Lossin <benno.lossin@...ton.me> wrote: >>> >>>> On 21.10.23 09:30, FUJITA Tomonori wrote: >>>>> On Sat, 21 Oct 2023 07:25:17 +0000 >>>>> Benno Lossin <benno.lossin@...ton.me> wrote: >>>>> >>>>>> On 20.10.23 14:54, FUJITA Tomonori wrote: >>>>>>> On Fri, 20 Oct 2023 09:34:46 +0900 (JST) >>>>>>> FUJITA Tomonori <fujita.tomonori@...il.com> wrote: >>>>>>> >>>>>>>> On Thu, 19 Oct 2023 15:20:51 +0000 >>>>>>>> Benno Lossin <benno.lossin@...ton.me> wrote: >>>>>>>> >>>>>>>>> I would like to remove the mutable static variable and simplify >>>>>>>>> the macro. >>>>>>>> >>>>>>>> How about adding DriverVTable array to Registration? >>>>>>>> >>>>>>>> /// Registration structure for a PHY driver. >>>>>>>> /// >>>>>>>> /// # Invariants >>>>>>>> /// >>>>>>>> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`. >>>>>>>> pub struct Registration<const N: usize> { >>>>>>>> drivers: [DriverVTable; N], >>>>>>>> } >>>>>>>> >>>>>>>> impl<const N: usize> Registration<{ N }> { >>>>>>>> /// Registers a PHY driver. >>>>>>>> pub fn register( >>>>>>>> module: &'static crate::ThisModule, >>>>>>>> drivers: [DriverVTable; N], >>>>>>>> ) -> Result<Self> { >>>>>>>> let mut reg = Registration { drivers }; >>>>>>>> let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>(); >>>>>>>> // 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. >>>>>>>> to_result(unsafe { >>>>>>>> bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0) >>>>>>>> })?; >>>>>>>> // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`. >>>>>>>> Ok(reg) >>>>>>>> } >>>>>>>> } >>>>>>> >>>>>>> Scratch this. >>>>>>> >>>>>>> This doesn't work. Also simply putting slice of DriverVTable into >>>>>>> Module strcut doesn't work. >>>>>> >>>>>> Why does it not work? I tried it and it compiled fine for me. >>>>> >>>>> You can compile but the kernel crashes. The addresses of the callback >>>>> functions are invalid. >>>> >>>> Can you please share your setup and the error? For me it booted >>>> fine. >>> >>> You use ASIX PHY hardware? >> >> It seems I have configured something wrong. Can you share your testing >> setup? Do you use a virtual PHY device in qemu, or do you boot it from >> real hardware with a real ASIX PHY device? > > real hardware with real ASIX PHY device. I see. > Qemu supports a virtual PHY device? I have no idea. [...] >> I think this is very weird, do you have any idea why this >> could happen? > > DriverVtable is created on kernel stack, I guess. But how does that invalidate the function pointers? >> If you don't mind, could you try if the following changes >> anything? > > I don't think it works. If you use const for DriverTable, DriverTable > is placed on read-only pages. The C side modifies DriverVTable array > so it does't work. Did you try it? Note that I copy the `DriverVTable` into the Module struct, so it will not be placed on a read-only page. >> (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => { >> const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+); >> struct Module { >> _drivers: [::kernel::net::phy::DriverVTable; N], >> } >> >> $crate::prelude::module! { >> type: Module, >> $($f)* >> } >> >> unsafe impl Sync for Module {} >> >> impl ::kernel::Module for Module { >> fn init(module: &'static ThisModule) -> Result<Self> { >> const DRIVERS: [::kernel::net::phy::DriverVTable; N] = [$(::kernel::net::phy::create_phy_driver::<$driver>()),+]; >> let mut m = Module { >> _drivers: unsafe { core::ptr::read(&DRIVERS) }, >> }; >> let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>(); >> ::kernel::error::to_result(unsafe { >> kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr()) >> })?; >> Ok(m) >> } >> } >> >> and also the variation where you replace `const DRIVERS` with >> `static DRIVERS`. > > Probably works. But looks like similar with the current code? This is > simpler? Just curious if it has to do with using `static` vs `const`. -- Cheers, Benno
Powered by blists - more mailing lists