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: <20240801.091708.676650759968461334.fujita.tomonori@gmail.com>
Date: Thu, 01 Aug 2024 09:17:08 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: aliceryhl@...gle.com
Cc: andrew@...n.ch, fujita.tomonori@...il.com, 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

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.