Date: Wed, 21 Jun 2023 22:13:49 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: benno.lossin@...ton.me
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org, aliceryhl@...gle.com, andrew@...n.ch,
 miguel.ojeda.sandonis@...il.com
Subject: Re: [PATCH 1/5] rust: core abstractions for network device drivers

Hi,
Thanks for reviewing.

On Thu, 15 Jun 2023 13:01:50 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:

> On 6/13/23 06:53, FUJITA Tomonori wrote:
>> This patch adds very basic abstractions to implement network device
>> drivers, corresponds to the kernel's net_device and net_device_ops
>> structs with support for register_netdev/unregister_netdev functions.
>> 
>> allows the const_maybe_uninit_zeroed feature for
>> core::mem::MaybeUinit::<T>::zeroed() in const function.
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
>> ---
>>  rust/bindings/bindings_helper.h |   2 +
>>  rust/helpers.c                  |  16 ++
>>  rust/kernel/lib.rs              |   3 +
>>  rust/kernel/net.rs              |   5 +
>>  rust/kernel/net/dev.rs          | 344 ++++++++++++++++++++++++++++++++
>>  5 files changed, 370 insertions(+)
>>  create mode 100644 rust/kernel/net.rs
>>  create mode 100644 rust/kernel/net/dev.rs
>> 
>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>> index 3e601ce2548d..468bf606f174 100644
>> --- a/rust/bindings/bindings_helper.h
>> +++ b/rust/bindings/bindings_helper.h
>> @@ -7,6 +7,8 @@
>>   */
>> 
>>  #include <linux/errname.h>
>> +#include <linux/etherdevice.h>
>> +#include <linux/netdevice.h>
>>  #include <linux/slab.h>
>>  #include <linux/refcount.h>
>>  #include <linux/wait.h>
>> diff --git a/rust/helpers.c b/rust/helpers.c
>> index bb594da56137..70d50767ff4e 100644
>> --- a/rust/helpers.c
>> +++ b/rust/helpers.c
>> @@ -24,10 +24,26 @@
>>  #include <linux/errname.h>
>>  #include <linux/refcount.h>
>>  #include <linux/mutex.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/skbuff.h>
>>  #include <linux/spinlock.h>
>>  #include <linux/sched/signal.h>
>>  #include <linux/wait.h>
>> 
>> +#ifdef CONFIG_NET
>> +void *rust_helper_netdev_priv(const struct net_device *dev)
>> +{
>> +	return netdev_priv(dev);
>> +}
>> +EXPORT_SYMBOL_GPL(rust_helper_netdev_priv);
>> +
>> +void rust_helper_skb_tx_timestamp(struct sk_buff *skb)
>> +{
>> +	skb_tx_timestamp(skb);
>> +}
>> +EXPORT_SYMBOL_GPL(rust_helper_skb_tx_timestamp);
>> +#endif
>> +
>>  __noreturn void rust_helper_BUG(void)
>>  {
>>  	BUG();
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 85b261209977..fc7d048d359d 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -13,6 +13,7 @@
>> 
>>  #![no_std]
>>  #![feature(allocator_api)]
>> +#![feature(const_maybe_uninit_zeroed)]
>>  #![feature(coerce_unsized)]
>>  #![feature(dispatch_from_dyn)]
>>  #![feature(new_uninit)]
>> @@ -34,6 +35,8 @@
>>  pub mod error;
>>  pub mod init;
>>  pub mod ioctl;
>> +#[cfg(CONFIG_NET)]
>> +pub mod net;
>>  pub mod prelude;
>>  pub mod print;
>>  mod static_assert;
>> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
>> new file mode 100644
>> index 000000000000..28fe8f398463
>> --- /dev/null
>> +++ b/rust/kernel/net.rs
>> @@ -0,0 +1,5 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Networking core.
>> +
>> +pub mod dev;
>> diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
>> new file mode 100644
>> index 000000000000..d072c81f99ce
>> --- /dev/null
>> +++ b/rust/kernel/net/dev.rs
>> @@ -0,0 +1,344 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Network device.
>> +//!
>> +//! C headers: [`include/linux/etherdevice.h`](../../../../include/linux/etherdevice.h),
>> +//! [`include/linux/ethtool.h`](../../../../include/linux/ethtool.h),
>> +//! [`include/linux/netdevice.h`](../../../../include/linux/netdevice.h),
>> +//! [`include/linux/skbuff.h`](../../../../include/linux/skbuff.h),
>> +//! [`include/uapi/linux/if_link.h`](../../../../include/uapi/linux/if_link.h).
>> +
>> +use crate::{bindings, error::*, prelude::vtable, types::ForeignOwnable};
>> +use {core::ffi::c_void, core::marker::PhantomData};
>> +
>> +/// Corresponds to the kernel's `struct net_device`.
>> +///
>> +/// # Invariants
>> +///
>> +/// The pointer is valid.
>> +pub struct Device(*mut bindings::net_device);
>> +
>> +impl Device {
>> +    /// Creates a new [`Device`] instance.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must ensure that `ptr` must be valid.
>> +    unsafe fn from_ptr(ptr: *mut bindings::net_device) -> Self {
>> +        // INVARIANT: The safety requirements ensure the invariant.
>> +        Self(ptr)
>> +    }
>> +
>> +    /// Gets a pointer to network device private data.
>> +    fn priv_data_ptr(&self) -> *const c_void {
>> +        // SAFETY: The type invariants guarantee that `self.0` is valid.
>> +        // During the initialization of `Registration` instance, the kernel allocates
>> +        // contiguous memory for `struct net_device` and a pointer to its private data.
>> +        // So it's safe to read an address from the returned address from `netdev_priv()`.
>> +        unsafe { core::ptr::read(bindings::netdev_priv(self.0) as *const *const c_void) }
> 
> Why are at least `size_of::<*const c_void>` bytes allocated? Why is it a
> `*const c_void` pointer? This function does not give any guarantees about
> this pointer, is it valid?

The reason is a device driver needs its data structure. It needs to
access to it via a pointer to bindings::net_device struct. The space
for the pointer is allocated during initialization of Registration and
it's valid until the Registration object is dropped.

> I know that you are allocating exactly this amount in `Registration`, but
> `Device` does not know about that. Should this be a type invariant?
> It might be a good idea to make `Driver` generic over `D`, the data that is
> stored behind this pointer. You could then return `D::Borrowed` instead.

We could do:

impl<D: DriverData> Device<D> {
...
    /// Gets the private data of a device driver.
    pub fn drv_priv_data(&self) -> <D::Data as ForeignOwnable>::Borrowed<'_> {
        unsafe {
            D::Data::borrow(core::ptr::read(
                bindings::netdev_priv(self.ptr) as *const *const c_void
            ))
        }
    }
}


>> +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used
>> +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync`
>> +// so it's safe to sharing its pointer.
>> +unsafe impl Send for Device {}
>> +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used
>> +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync`,
>> +// can be used from any thread too.
>> +unsafe impl Sync for Device {}
>> +
>> +/// Trait for device driver specific information.
>> +///
>> +/// This data structure is passed to a driver with the operations for `struct net_device`
>> +/// like `struct net_device_ops`, `struct ethtool_ops`, `struct rtnl_link_ops`, etc.
>> +pub trait DriverData {
>> +    /// The object are stored in C object, `struct net_device`.
>> +    type Data: ForeignOwnable + Send + Sync;
> 
> Why is this an associated type? Could you not use
> `D: ForeignOwnable + Send + Sync` everywhere instead?
> I think this should be possible, since `DriverData` does not define
> anything else.

With that approach, is it possible to allow a device driver to define
own data structure and functions taking the structure as aurgument
(like DevOps structutre in the 5th patch)


>> +/// Registration structure for a network device driver.
>> +///
>> +/// This allocates and owns a `struct net_device` object.
>> +/// Once the `net_device` object is registered via `register_netdev` function,
>> +/// the kernel calls various functions such as `struct net_device_ops` operations with
>> +/// the `net_device` object.
>> +///
>> +/// A driver must implement `struct net_device_ops` so the trait for it is tied.
>> +/// Other operations like `struct ethtool_ops` are optional.
>> +pub struct Registration<T: DeviceOperations<D>, D: DriverData> {
>> +    dev: Device,
>> +    is_registered: bool,
>> +    _p: PhantomData<(D, T)>,
>> +}
>> +
>> +impl<D: DriverData, T: DeviceOperations<D>> Drop for Registration<T, D> {
>> +    fn drop(&mut self) {
>> +        // SAFETY: The type invariants guarantee that `self.dev.0` is valid.
>> +        unsafe {
>> +            let _ = D::Data::from_foreign(self.dev.priv_data_ptr());
> 
> Why is `self.dev.priv_data_ptr()` a valid pointer?
> This `unsafe` block should be split to better explain the different safety
> requirements.

Explained above.

>> +            if self.is_registered {
>> +                bindings::unregister_netdev(self.dev.0);
>> +            }
>> +            bindings::free_netdev(self.dev.0);
>> +        }
>> +    }
>> +}
>> +
>> +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> {
>> +    /// Creates a new [`Registration`] instance for ethernet device.
>> +    ///
>> +    /// A device driver can pass private data.
>> +    pub fn try_new_ether(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> {
>> +        // SAFETY: FFI call.
> 
> If this FFI call has no safety requirements then say so.

SAFETY: FFI call has no safety requirements.

?

>> +    const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
>> +        ndo_init: if <T>::HAS_INIT {
>> +            Some(Self::init_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_uninit: if <T>::HAS_UNINIT {
>> +            Some(Self::uninit_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_open: if <T>::HAS_OPEN {
>> +            Some(Self::open_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_stop: if <T>::HAS_STOP {
>> +            Some(Self::stop_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_start_xmit: if <T>::HAS_START_XMIT {
>> +            Some(Self::start_xmit_callback)
>> +        } else {
>> +            None
>> +        },
>> +        // SAFETY: The rest is zeroed out to initialize `struct net_device_ops`,
>> +        // set `Option<&F>` to be `None`.
>> +        ..unsafe { core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() }
>> +    };
>> +
>> +    const fn build_device_ops() -> &'static bindings::net_device_ops {
>> +        &Self::DEVICE_OPS
>> +    }
> 
> Why does this function exist?

To get const struct net_device_ops *netdev_ops.

>> +
>> +    unsafe extern "C" fn init_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int {
>> +        from_result(|| {
> 
> Since you are the first user of `from_result`, you can remove the
> `#[allow(dead_code)]` attribute.
> 
> @Reviewers/Maintainers: Or would we prefer to make that change ourselves?

Ah, either is fine by me.

>> +            // SAFETY: The C API guarantees that `netdev` is valid while this function is running.
>> +            let mut dev = unsafe { Device::from_ptr(netdev) };
>> +            // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when
>> +            // `Registration` object was created.
>> +            // `D::Data::from_foreign` is only called by the object was released.
>> +            // So we know `data` is valid while this function is running.
> 
> This should be a type invariant of `Registration`.

Understood.

>> +            let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) };
>> +            T::init(&mut dev, data)?;
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    unsafe extern "C" fn uninit_callback(netdev: *mut bindings::net_device) {
>> +        // SAFETY: The C API guarantees that `netdev` is valid while this function is running.
>> +        let mut dev = unsafe { Device::from_ptr(netdev) };
>> +        // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when
>> +        // `Registration` object was created.
>> +        // `D::Data::from_foreign` is only called by the object was released.
>> +        // So we know `data` is valid while this function is running.
>> +        let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) };
>> +        T::uninit(&mut dev, data);
>> +    }
>> +
>> +    unsafe extern "C" fn open_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int {
>> +        from_result(|| {
>> +            // SAFETY: The C API guarantees that `netdev` is valid while this function is running.
>> +            let mut dev = unsafe { Device::from_ptr(netdev) };
>> +            // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when
>> +            // `Registration` object was created.
>> +            // `D::Data::from_foreign` is only called by the object was released.
>> +            // So we know `data` is valid while this function is running.
>> +            let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) };
>> +            T::open(&mut dev, data)?;
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    unsafe extern "C" fn stop_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int {
>> +        from_result(|| {
>> +            // SAFETY: The C API guarantees that `netdev` is valid while this function is running.
>> +            let mut dev = unsafe { Device::from_ptr(netdev) };
>> +            // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when
>> +            // `Registration` object was created.
>> +            // `D::Data::from_foreign` is only called by the object was released.
>> +            // So we know `data` is valid while this function is running.
>> +            let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) };
>> +            T::stop(&mut dev, data)?;
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    unsafe extern "C" fn start_xmit_callback(
>> +        skb: *mut bindings::sk_buff,
>> +        netdev: *mut bindings::net_device,
>> +    ) -> bindings::netdev_tx_t {
>> +        // SAFETY: The C API guarantees that `netdev` is valid while this function is running.
>> +        let mut dev = unsafe { Device::from_ptr(netdev) };
>> +        // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when
>> +        // `Registration` object was created.
>> +        // `D::Data::from_foreign` is only called by the object was released.
>> +        // So we know `data` is valid while this function is running.
>> +        let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) };
>> +        // SAFETY: The C API guarantees that `skb` is valid while this function is running.
>> +        let skb = unsafe { SkBuff::from_ptr(skb) };
>> +        T::start_xmit(&mut dev, data, skb) as bindings::netdev_tx_t
>> +    }
>> +}
>> +
>> +// SAFETY: `Registration` exposes only `Device` object which can be used from
>> +// any thread.
>> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {}
>> +// SAFETY: `Registration` exposes only `Device` object which can be used from
>> +// any thread.
>> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Sync for Registration<T, D> {}
>> +
>> +/// Corresponds to the kernel's `enum netdev_tx`.
>> +#[repr(i32)]
>> +pub enum TxCode {
>> +    /// Driver took care of packet.
>> +    Ok = bindings::netdev_tx_NETDEV_TX_OK,
>> +    /// Driver tx path was busy.
>> +    Busy = bindings::netdev_tx_NETDEV_TX_BUSY,
>> +}
>> +
>> +/// Corresponds to the kernel's `struct net_device_ops`.
>> +///
>> +/// A device driver must implement this. Only very basic operations are supported for now.
>> +#[vtable]
>> +pub trait DeviceOperations<D: DriverData> {
> 
> Why is this trait generic over `D`? Why is this not `Self` or an associated
> type?

DriverData also used in EtherOperationsAdapter (the second patch) and
there are other operations that uses DriverData (not in this patchset).


>> +    /// Corresponds to `ndo_init` in `struct net_device_ops`.
>> +    fn init(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result {
> 
> Why do all of these functions take a `&mut Device`? `Device` already is a
> pointer, so why the double indirection?

I guess that I follow the existing code like

https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/amba.rs

>> +/// Corresponds to the kernel's `struct sk_buff`.
>> +///
>> +/// A driver manages `struct sk_buff` in two ways. In both ways, the ownership is transferred
>> +/// between C and Rust. The allocation and release are done asymmetrically.
>> +///
>> +/// On the tx side (`ndo_start_xmit` operation in `struct net_device_ops`), the kernel allocates
>> +/// a `sk_buff' object and passes it to the driver. The driver is responsible for the release
>> +/// after transmission.
>> +/// On the rx side, the driver allocates a `sk_buff` object then passes it to the kernel
>> +/// after receiving data.
>> +///
>> +/// # Invariants
>> +///
>> +/// The pointer is valid.
>> +pub struct SkBuff(*mut bindings::sk_buff);
>> +
>> +impl SkBuff {
>> +    /// Creates a new [`SkBuff`] instance.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must ensure that `ptr` must be valid.
>> +    unsafe fn from_ptr(ptr: *mut bindings::sk_buff) -> Self {
>> +        // INVARIANT: The safety requirements ensure the invariant.
>> +        Self(ptr)
>> +    }
>> +
>> +    /// Provides a time stamp.
>> +    pub fn tx_timestamp(&mut self) {
>> +        // SAFETY: The type invariants guarantee that `self.0` is valid.
>> +        unsafe {
>> +            bindings::skb_tx_timestamp(self.0);
>> +        }
>> +    }
>> +}
>> +
>> +impl Drop for SkBuff {
>> +    fn drop(&mut self) {
>> +        // SAFETY: The type invariants guarantee that `self.0` is valid.
>> +        unsafe {
>> +            bindings::kfree_skb_reason(
>> +                self.0,
>> +                bindings::skb_drop_reason_SKB_DROP_REASON_NOT_SPECIFIED,
>> +            )
> 
> AFAICT this function frees the `struct sk_buff`, why is this safe? This
> function also has as a requirement that all other pointers to this struct
> are never used again. How do you guarantee this?
> You mentioned above that there are two us cases for an SkBuff, in one case
> the kernel frees it and in another the driver. How do we know that we can
> free it here?

This can handle only the tx case.

As you can see, we had a good discussion on this and seems that found
a solution. It'll be fixed.

Powered by blists - more mailing lists