netdev - Re: [PATCH net-next v1 3/4] rust: net::phy support Firmware API

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: <20240418.215108.816248101599824703.fujita.tomonori@gmail.com>
Date: Thu, 18 Apr 2024 21:51:08 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: gregkh@...uxfoundation.org
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org, andrew@...n.ch,
 rust-for-linux@...r.kernel.org, tmgross@...ch.edu, mcgrof@...nel.org,
 russ.weight@...ux.dev
Subject: Re: [PATCH net-next v1 3/4] rust: net::phy support Firmware API

Hi,

On Mon, 15 Apr 2024 13:10:59 +0200
Greg KH <gregkh@...uxfoundation.org> wrote:

> On Mon, Apr 15, 2024 at 07:47:00PM +0900, FUJITA Tomonori wrote:
>> This patch adds support to the following basic Firmware API:
>> 
>> - request_firmware
>> - release_firmware
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
>> CC: Luis Chamberlain <mcgrof@...nel.org>
>> CC: Russ Weight <russ.weight@...ux.dev>
>> ---
>>  drivers/net/phy/Kconfig         |  1 +
>>  rust/bindings/bindings_helper.h |  1 +
>>  rust/kernel/net/phy.rs          | 45 +++++++++++++++++++++++++++++++++
> 
> Please do not bury this in the phy.rs file, put it in drivers/base/ next
> to the firmware functions it is calling.

Sure. I had a version of creating rust/kernel/firmware.rs but I wanted
to know if a temporary solution could be accepted.

With the build system for Rust, we can't put it in drivers/base/ yet.


>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> index 421a231421f5..095dc3ccc553 100644
>> --- a/rust/kernel/net/phy.rs
>> +++ b/rust/kernel/net/phy.rs
>> @@ -9,6 +9,51 @@
>>  use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
>>  
>>  use core::marker::PhantomData;
>> +use core::ptr::{self, NonNull};
>> +
>> +/// A pointer to the kernel's `struct firmware`.
>> +///
>> +/// # Invariants
>> +///
>> +/// The pointer points at a `struct firmware`, and has ownership over the object.
>> +pub struct Firmware(NonNull<bindings::firmware>);
>> +
>> +impl Firmware {
>> +    /// Loads a firmware.
>> +    pub fn new(name: &CStr, dev: &Device) -> Result<Firmware> {
>> +        let phydev = dev.0.get();
> 
> That's risky, how do you "know" this device really is a phydev?

That's guaranteed. The above `Device` is phy::Device.

> That's not how the firmware api works, use a real 'struct device' please.

Right, I'll do in v2.

> 
>> +        let mut ptr: *mut bindings::firmware = ptr::null_mut();
>> +        let p_ptr: *mut *mut bindings::firmware = &mut ptr;
> 
> I'm sorry, but I don't understand the two step thing here, you need a
> pointer for where the C code can put something, is this really how you
> do that in rust?  Shouldn't it point to data somehow down below?

Oops, can be simpler:

let mut ptr: *const bindings::firmware = ptr::null_mut();
let ret = unsafe {
        bindings::request_firmware(&mut ptr, name.as_char_ptr().cast(), &mut (*phydev).mdio.dev)
};

>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
> 
> Again, phydev is not part of the firmware api.
> 
>> +        // So it's just an FFI call.
>> +        let ret = unsafe {
>> +            bindings::request_firmware(
>> +                p_ptr as *mut *const bindings::firmware,
> 
> Where is this coming from?
> 
>> +                name.as_char_ptr().cast(),
>> +                &mut (*phydev).mdio.dev,
>> +            )
>> +        };
>> +        let fw = NonNull::new(ptr).ok_or_else(|| Error::from_errno(ret))?;
>> +        // INVARIANT: We checked that the firmware was successfully loaded.
>> +        Ok(Firmware(fw))
>> +    }
>> +
>> +    /// Accesses the firmware contents.
>> +    pub fn data(&self) -> &[u8] {
> 
> But firmware is not a u8, it's a structure.  Can't the rust bindings
> handle that?  Oh wait, data is a u8, but the bindings should show that,
> right?

In the C side:

struct firmware {
        size_t size;
        const u8 *data;
        /* firmware loader private fields */
        void *priv;
};

data() function allows a driver in Rust to access to the above data
member in Rust.

A driver could define its own structure over &[u8]. 

> 
>> +        // SAFETY: The type invariants guarantee that `self.0.as_ptr()` is valid.
>> +        // They also guarantee that `self.0.as_ptr().data` pointers to
>> +        // a valid memory region of size `self.0.as_ptr().size`.
>> +        unsafe { core::slice::from_raw_parts((*self.0.as_ptr()).data, (*self.0.as_ptr()).size) }
> 
> If new fails, does accessing this also fail?

If a new() fails, a Firmware object isn't created. So data() function
cannot be called.


> And how are you using this?  I guess I'll dig in the next patch...
> 
>> +    }
>> +}
>> +
>> +impl Drop for Firmware {
>> +    fn drop(&mut self) {
>> +        // SAFETY: By the type invariants, `self.0.as_ptr()` is valid and
>> +        // we have ownership of the object so can free it.
>> +        unsafe { bindings::release_firmware(self.0.as_ptr()) }
> 
> So drop will never be called if new fails?

Yes, like data() function.