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: <CALNs47ujBcwHG+sgeH3m7gvkW6JKWtD0ZS66ujmswLODuExJhg@mail.gmail.com> Date: Sun, 8 Oct 2023 02:07:44 -0400 From: Trevor Gross <tmgross@...ch.edu> To: Andrew Lunn <andrew@...n.ch> Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com, greg@...ah.com Subject: Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers On Sat, Oct 7, 2023 at 11:13 AM Andrew Lunn <andrew@...n.ch> wrote: > > > The safety comment here still needs something like > > > > with the exception of fields that are synchronized via the `lock` mutex > > I'm not sure that really adds much useful information. Which values > are protected by the lock? More importantly, which are not protected > by the lock? > > As a general rule of thumb, driver writers don't understand > locking. Yes, there are some which do, but many don't. So the > workaround to that is make it so they don't need to understand > locking. All the locking happens in the core. > > The exception is suspend and resume, which are called without the > lock. So if i was to add a comment about locking, i would only put a > comment on those two. This doesn't get used by driver implementations, it's only used within the abstractions here. I think anyone who needs the details can refer to the C side, I just suggested to note the locking caveat based on your second comment at https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/ Fujita - since this doesn't get exposed, could this be pub(crate)?) > > Andrew, are there any restrictions about calling phy_init_hw more than > > once? Or are there certain things that you are not allowed to do until > > you call that function? > > phy_init_hw can be called multiple times. It used by drivers as a work > around to broken hardware/firmware to get the device back into a good > state. It is also used during resume, since often the PHY looses its > settings when suspended. Great, thank you for the clarification > > > + unsafe extern "C" fn read_mmd_callback( > > > + phydev: *mut bindings::phy_device, > > > + devnum: i32, > > > + regnum: u16, > > > + ) -> i32 { > > > + from_result(|| { > > > + // SAFETY: The C API guarantees that `phydev` is valid while this function is running. > > > + let dev = unsafe { Device::from_raw(phydev) }; > > > + let ret = T::read_mmd(dev, devnum as u8, regnum)?; > > > + Ok(ret.into()) > > > + }) > > > + } > > > > Since your're reading a bus, it probably doesn't hurt to do a quick > > check when converting > > > > let devnum_u8 = u8::try_from(devnum).(|_| { > > warn_once!("devnum {devnum} exceeds u8 limits"); > > code::EINVAL > > })? > > I would actually say this is the wrong place to do that. Such checks > should happen in the core, so it checks all drivers, not just the > current one Rust driver. Feel free to submit a C patch adding this. > > Andrew I guess it does that already: https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/net/phy/phy-core.c#L556 Fujita, I think we started doing comments when we know that lossy/bitwise `as` casts are correct. Maybe just leave the code as-is but add // CAST: the C side verifies devnum < 32 ?
Powered by blists - more mailing lists