netdev - Re: [PATCH net-next v2 2/6] rust: net::phy support probe callback

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH5fLgj05_rZ=MnRhqUMnthLKioiXJ9xvMAtHyc3-MzNcfbkBg@mail.gmail.com>
Date: Thu, 1 Aug 2024 11:07:18 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: andrew@...n.ch, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, 
	tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, benno.lossin@...ton.me
Subject: Re: [PATCH net-next v2 2/6] rust: net::phy support probe callback

On Thu, Aug 1, 2024 at 2:17 AM FUJITA Tomonori
<fujita.tomonori@...il.com> wrote:
>
> Thanks for the review!
>
> On Wed, 31 Jul 2024 14:32:18 +0200
> Alice Ryhl <aliceryhl@...gle.com> wrote:
>
> >> > +    /// # Safety
> >> > +    ///
> >> > +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> >> > +    unsafe extern "C" fn probe_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> >> > +        from_result(|| {
> >> > +            // SAFETY: This callback is called only in contexts
> >> > +            // where we can exclusively access to `phy_device`, so the accessors on
> >> > +            // `Device` are okay to call.
> >>
> >> This one is slightly different to other callbacks. probe is called
> >> without the mutex. Instead, probe is called before the device is
> >> published. So the comment is correct, but given how important Rust
> >> people take these SAFETY comments, maybe it should indicate it is
> >> different to others?
> >
> > Interesting. Given that we don't hold the mutex, does that mean that
> > some of the methods on Device are not safe to call in this context? Or
> > is there something else that makes it okay to call them despite not
> > holding the mutex?
>
> Before the callback, the device object was initialized properly by
> PHYLIB and no concurrent access so all the methods can be called
> safely (no kernel panic), I think.
>
> If the safety comment needs to updated, how about the following?
>
> SAFETY: This callback is called only in contexts where we can
> exclusively access to `phy_device` because it's not published yet, so
> the accessors on `Device` are okay to call.

Yes, that sounds good to me. With the updated safety comment, feel
free to include my Reviewed-by in your next version.

Alice