[PATCH 0/5] Rust abstractions for network device drivers

[PATCH 0/5] Rust abstractions for network device drivers

[FUJITA Tomonori]

This patchset adds minimum Rust abstractions for network device
drivers and an example of a Rust network device driver, a simpler
version of drivers/net/dummy.c.

The dummy network device driver doesn't attach any bus such as PCI so
the dependency is minimum. Hopefully, it would make reviewing easier.

Thanks a lot for reviewing on RFC patchset at rust-for-linux ml.
Hopefully, I've addressed all the issues.

FUJITA Tomonori (5):
  rust: core abstractions for network device drivers
  rust: add support for ethernet operations
  rust: add support for get_stats64 in struct net_device_ops
  rust: add methods for configure net_device
  samples: rust: add dummy network driver

 rust/bindings/bindings_helper.h |   3 +
 rust/helpers.c                  |  23 ++
 rust/kernel/lib.rs              |   3 +
 rust/kernel/net.rs              |   5 +
 rust/kernel/net/dev.rs          | 697 ++++++++++++++++++++++++++++++++
 samples/rust/Kconfig            |  12 +
 samples/rust/Makefile           |   1 +
 samples/rust/rust_net_dummy.rs  |  81 ++++
 scripts/Makefile.build          |   2 +-
 9 files changed, 826 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/dev.rs
 create mode 100644 samples/rust/rust_net_dummy.rs


base-commit: d2e3115d717197cb2bc020dd1f06b06538474ac3
-- 
2.34.1

[PATCH 1/5] rust: core abstractions for network device drivers

[FUJITA Tomonori]

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@gmail.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) }
+    }
+}
+
+// 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;
+}
+
+/// 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());
+            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.
+        let ptr = from_err_ptr(unsafe {
+            bindings::alloc_etherdev_mqs(
+                core::mem::size_of::<*const c_void>() as i32,
+                tx_queue_size,
+                rx_queue_size,
+            )
+        })?;
+
+        // SAFETY: `ptr` is valid and non-null since `alloc_etherdev_mqs()`
+        // returned a valid pointer which was null-checked.
+        let dev = unsafe { Device::from_ptr(ptr) };
+        // SAFETY: It's safe to write an address to the returned pointer
+        // from `netdev_priv()` because `alloc_etherdev_mqs()` allocates
+        // contiguous memory for `struct net_device` and a pointer.
+        unsafe {
+            let priv_ptr = bindings::netdev_priv(ptr) as *mut *const c_void;
+            core::ptr::write(priv_ptr, data.into_foreign());
+        }
+        Ok(Registration {
+            dev,
+            is_registered: false,
+            _p: PhantomData,
+        })
+    }
+
+    /// Returns a network device.
+    ///
+    /// A device driver normally configures the device before registration.
+    pub fn dev_get(&mut self) -> &mut Device {
+        &mut self.dev
+    }
+
+    /// Registers a network device.
+    pub fn register(&mut self) -> Result {
+        if self.is_registered {
+            return Err(code::EINVAL);
+        }
+        // SAFETY: The type invariants guarantee that `self.dev.0` is valid.
+        let ret = unsafe {
+            (*self.dev.0).netdev_ops = Self::build_device_ops();
+            bindings::register_netdev(self.dev.0)
+        };
+        if ret != 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            self.is_registered = true;
+            Ok(())
+        }
+    }
+
+    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
+    }
+
+    unsafe extern "C" fn init_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::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> {
+    /// Corresponds to `ndo_init` in `struct net_device_ops`.
+    fn init(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result {
+        Ok(())
+    }
+
+    /// Corresponds to `ndo_uninit` in `struct net_device_ops`.
+    fn uninit(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) {}
+
+    /// Corresponds to `ndo_open` in `struct net_device_ops`.
+    fn open(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result {
+        Ok(())
+    }
+
+    /// Corresponds to `ndo_stop` in `struct net_device_ops`.
+    fn stop(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result {
+        Ok(())
+    }
+
+    /// Corresponds to `ndo_start_xmit` in `struct net_device_ops`.
+    fn start_xmit(
+        _dev: &mut Device,
+        _data: <D::Data as ForeignOwnable>::Borrowed<'_>,
+        _skb: SkBuff,
+    ) -> TxCode {
+        TxCode::Busy
+    }
+}
+
+/// 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,
+            )
+        }
+    }
+}
-- 
2.34.1

[PATCH 2/5] rust: add support for ethernet operations

[FUJITA Tomonori]

This improves abstractions for network device drivers to implement
struct ethtool_ops, the majority of ethernet device drivers need to
do.

struct ethtool_ops also needs to access to device private data like
struct net_devicve_ops.

Currently, only get_ts_info operation is supported. The following
patch adds the Rust version of the dummy network driver, which uses
the operation.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/net/dev.rs          | 108 ++++++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 468bf606f174..6446ff764980 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -8,6 +8,7 @@
 
 #include <linux/errname.h>
 #include <linux/etherdevice.h>
+#include <linux/ethtool.h>
 #include <linux/netdevice.h>
 #include <linux/slab.h>
 #include <linux/refcount.h>
diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
index d072c81f99ce..d6012b2eea33 100644
--- a/rust/kernel/net/dev.rs
+++ b/rust/kernel/net/dev.rs
@@ -141,6 +141,18 @@ pub fn register(&mut self) -> Result {
         }
     }
 
+    /// Sets `ethtool_ops` of `net_device`.
+    pub fn set_ether_operations<U>(&mut self) -> Result
+    where
+        U: EtherOperations<D>,
+    {
+        if self.is_registered {
+            return Err(code::EINVAL);
+        }
+        EtherOperationsAdapter::<U, D>::new().set_ether_ops(&mut self.dev);
+        Ok(())
+    }
+
     const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
         ndo_init: if <T>::HAS_INIT {
             Some(Self::init_callback)
@@ -342,3 +354,99 @@ fn drop(&mut self) {
         }
     }
 }
+
+/// Builds the kernel's `struct ethtool_ops`.
+struct EtherOperationsAdapter<D, T> {
+    _p: PhantomData<(D, T)>,
+}
+
+impl<D, T> EtherOperationsAdapter<T, D>
+where
+    D: DriverData,
+    T: EtherOperations<D>,
+{
+    /// Creates a new instance.
+    fn new() -> Self {
+        EtherOperationsAdapter { _p: PhantomData }
+    }
+
+    unsafe extern "C" fn get_ts_info_callback(
+        netdev: *mut bindings::net_device,
+        info: *mut bindings::ethtool_ts_info,
+    ) -> 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()) };
+            // SAFETY: The C API guarantees that `info` is valid while this function is running.
+            let mut info = unsafe { EthtoolTsInfo::from_ptr(info) };
+            T::get_ts_info(&mut dev, data, &mut info)?;
+            Ok(0)
+        })
+    }
+
+    const ETHER_OPS: bindings::ethtool_ops = bindings::ethtool_ops {
+        get_ts_info: if <T>::HAS_GET_TS_INFO {
+            Some(Self::get_ts_info_callback)
+        } else {
+            None
+        },
+        ..unsafe { core::mem::MaybeUninit::<bindings::ethtool_ops>::zeroed().assume_init() }
+    };
+
+    const fn build_ether_ops() -> &'static bindings::ethtool_ops {
+        &Self::ETHER_OPS
+    }
+
+    fn set_ether_ops(&self, dev: &mut Device) {
+        // SAFETY: The type invariants guarantee that `dev.0` is valid.
+        unsafe {
+            (*dev.0).ethtool_ops = Self::build_ether_ops();
+        }
+    }
+}
+
+/// Corresponds to the kernel's `struct ethtool_ops`.
+#[vtable]
+pub trait EtherOperations<D: DriverData> {
+    /// Corresponds to `get_ts_info` in `struct ethtool_ops`.
+    fn get_ts_info(
+        _dev: &mut Device,
+        _data: <D::Data as ForeignOwnable>::Borrowed<'_>,
+        _info: &mut EthtoolTsInfo,
+    ) -> Result {
+        Err(Error::from_errno(bindings::EOPNOTSUPP as i32))
+    }
+}
+
+/// Corresponds to the kernel's `struct ethtool_ts_info`.
+///
+/// # Invariants
+///
+/// The pointer is valid.
+pub struct EthtoolTsInfo(*mut bindings::ethtool_ts_info);
+
+impl EthtoolTsInfo {
+    /// Creates a new `EthtoolTsInfo' instance.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` must be valid.
+    unsafe fn from_ptr(ptr: *mut bindings::ethtool_ts_info) -> Self {
+        // INVARIANT: The safety requirements ensure the invariant.
+        Self(ptr)
+    }
+}
+
+/// Sets up the info for software timestamping.
+pub fn ethtool_op_get_ts_info(dev: &mut Device, info: &mut EthtoolTsInfo) -> Result {
+    // SAFETY: The type invariants guarantee that `dev.0` and `info.0` are valid.
+    unsafe {
+        bindings::ethtool_op_get_ts_info(dev.0, info.0);
+    }
+    Ok(())
+}
-- 
2.34.1

[PATCH 3/5] rust: add support for get_stats64 in struct net_device_ops

[FUJITA Tomonori]

get_stats64() is used to return the stats for user-space like the
number of packets, which network device drivers are supposed to
support.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/dev.rs | 64 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
index d6012b2eea33..452944cf9fb8 100644
--- a/rust/kernel/net/dev.rs
+++ b/rust/kernel/net/dev.rs
@@ -8,7 +8,12 @@
 //! [`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 crate::{
+    bindings,
+    error::*,
+    prelude::vtable,
+    types::{ForeignOwnable, Opaque},
+};
 use {core::ffi::c_void, core::marker::PhantomData};
 
 /// Corresponds to the kernel's `struct net_device`.
@@ -179,6 +184,11 @@ pub fn set_ether_operations<U>(&mut self) -> Result
         } else {
             None
         },
+        ndo_get_stats64: if <T>::HAS_GET_STATS64 {
+            Some(Self::get_stats64_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() }
@@ -256,6 +266,22 @@ const fn build_device_ops() -> &'static bindings::net_device_ops {
         let skb = unsafe { SkBuff::from_ptr(skb) };
         T::start_xmit(&mut dev, data, skb) as bindings::netdev_tx_t
     }
+
+    unsafe extern "C" fn get_stats64_callback(
+        netdev: *mut bindings::net_device,
+        stats: *mut bindings::rtnl_link_stats64,
+    ) {
+        // 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: for writing and nobody else will read or write to it.
+        let stats = unsafe { RtnlLinkStats64::from_ptr(stats) };
+        T::get_stats64(&mut dev, data, stats);
+    }
 }
 
 // SAFETY: `Registration` exposes only `Device` object which can be used from
@@ -305,6 +331,14 @@ fn start_xmit(
     ) -> TxCode {
         TxCode::Busy
     }
+
+    /// Corresponds to `ndo_get_stats64` in `struct net_device_ops`.
+    fn get_stats64(
+        _dev: &mut Device,
+        _data: <D::Data as ForeignOwnable>::Borrowed<'_>,
+        _stats: &mut RtnlLinkStats64,
+    ) {
+    }
 }
 
 /// Corresponds to the kernel's `struct sk_buff`.
@@ -355,6 +389,34 @@ fn drop(&mut self) {
     }
 }
 
+/// Corresponds to the kernel's `struct rtnl_link_stats64`.
+#[repr(transparent)]
+pub struct RtnlLinkStats64(Opaque<bindings::rtnl_link_stats64>);
+
+impl RtnlLinkStats64 {
+    /// Creates a new [`RtnlLinkStats64`] instance.
+    ///
+    /// # Safety
+    ///
+    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
+    /// may read or write to the `rtnl_link_stats64` object.
+    unsafe fn from_ptr<'a>(ptr: *mut bindings::rtnl_link_stats64) -> &'a mut Self {
+        unsafe { &mut *(ptr as *mut Self) }
+    }
+
+    /// Updates TX stats.
+    pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) {
+        // SAFETY: We have exclusive access to the `rtnl_link_stats64`, so writing to it is okay.
+        unsafe {
+            let inner = Opaque::get(&self.0);
+            (*inner).tx_packets = packets;
+            (*inner).tx_bytes = bytes;
+            (*inner).tx_errors = errors;
+            (*inner).tx_dropped = dropped;
+        }
+    }
+}
+
 /// Builds the kernel's `struct ethtool_ops`.
 struct EtherOperationsAdapter<D, T> {
     _p: PhantomData<(D, T)>,
-- 
2.34.1

[PATCH 4/5] rust: add methods for configure net_device

[FUJITA Tomonori]

adds methods to net::Device for the basic configurations of
net_device.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers.c         |   7 ++
 rust/kernel/net/dev.rs | 183 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 190 insertions(+)

diff --git a/rust/helpers.c b/rust/helpers.c
index 70d50767ff4e..6c51deb18dc1 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -22,6 +22,7 @@
 #include <linux/build_bug.h>
 #include <linux/err.h>
 #include <linux/errname.h>
+#include <linux/etherdevice.h>
 #include <linux/refcount.h>
 #include <linux/mutex.h>
 #include <linux/netdevice.h>
@@ -31,6 +32,12 @@
 #include <linux/wait.h>
 
 #ifdef CONFIG_NET
+void rust_helper_eth_hw_addr_random(struct net_device *dev)
+{
+	eth_hw_addr_random(dev);
+}
+EXPORT_SYMBOL_GPL(rust_helper_eth_hw_addr_random);
+
 void *rust_helper_netdev_priv(const struct net_device *dev)
 {
 	return netdev_priv(dev);
diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
index 452944cf9fb8..4767f331973e 100644
--- a/rust/kernel/net/dev.rs
+++ b/rust/kernel/net/dev.rs
@@ -12,10 +12,118 @@
     bindings,
     error::*,
     prelude::vtable,
+    str::CStr,
     types::{ForeignOwnable, Opaque},
 };
 use {core::ffi::c_void, core::marker::PhantomData};
 
+/// Flags associated with a [`Device`].
+pub mod flags {
+    /// Interface is up.
+    pub const IFF_UP: u32 = bindings::net_device_flags_IFF_UP;
+    /// Broadcast address valid.
+    pub const IFF_BROADCAST: u32 = bindings::net_device_flags_IFF_BROADCAST;
+    /// Device on debugging.
+    pub const IFF_DEBUG: u32 = bindings::net_device_flags_IFF_DEBUG;
+    /// Loopback device.
+    pub const IFF_LOOPBACK: u32 = bindings::net_device_flags_IFF_LOOPBACK;
+    /// Has p-p link.
+    pub const IFF_POINTOPOINT: u32 = bindings::net_device_flags_IFF_POINTOPOINT;
+    /// Avoids use of trailers.
+    pub const IFF_NOTRAILERS: u32 = bindings::net_device_flags_IFF_NOTRAILERS;
+    /// Interface RFC2863 OPER_UP.
+    pub const IFF_RUNNING: u32 = bindings::net_device_flags_IFF_RUNNING;
+    /// No ARP protocol.
+    pub const IFF_NOARP: u32 = bindings::net_device_flags_IFF_NOARP;
+    /// Receives all packets.
+    pub const IFF_PROMISC: u32 = bindings::net_device_flags_IFF_PROMISC;
+    /// Receive all multicast packets.
+    pub const IFF_ALLMULTI: u32 = bindings::net_device_flags_IFF_ALLMULTI;
+    /// Master of a load balancer.
+    pub const IFF_MASTER: u32 = bindings::net_device_flags_IFF_MASTER;
+    /// Slave of a load balancer.
+    pub const IFF_SLAVE: u32 = bindings::net_device_flags_IFF_SLAVE;
+    /// Supports multicast.
+    pub const IFF_MULTICAST: u32 = bindings::net_device_flags_IFF_MULTICAST;
+    /// Capable of setting media type.
+    pub const IFF_PORTSEL: u32 = bindings::net_device_flags_IFF_PORTSEL;
+    /// Auto media select active.
+    pub const IFF_AUTOMEDIA: u32 = bindings::net_device_flags_IFF_AUTOMEDIA;
+    /// Dialup device with changing addresses.
+    pub const IFF_DYNAMIC: u32 = bindings::net_device_flags_IFF_DYNAMIC;
+}
+
+/// Private flags associated with a [`Device`].
+pub mod priv_flags {
+    /// 802.1Q VLAN device.
+    pub const IFF_802_1Q_VLAN: u64 = bindings::netdev_priv_flags_IFF_802_1Q_VLAN;
+    /// Ethernet bridging device.
+    pub const IFF_EBRIDGE: u64 = bindings::netdev_priv_flags_IFF_EBRIDGE;
+    /// Bonding master or slave device.
+    pub const IFF_BONDING: u64 = bindings::netdev_priv_flags_IFF_BONDING;
+    /// ISATAP interface (RFC4214).
+    pub const IFF_ISATAP: u64 = bindings::netdev_priv_flags_IFF_ISATAP;
+    /// WAN HDLC device.
+    pub const IFF_WAN_HDLC: u64 = bindings::netdev_priv_flags_IFF_WAN_HDLC;
+    /// dev_hard_start_xmit() is allowed to release skb->dst.
+    pub const IFF_XMIT_DST_RELEASE: u64 = bindings::netdev_priv_flags_IFF_XMIT_DST_RELEASE;
+    /// Disallows bridging this ether device.
+    pub const IFF_DONT_BRIDGE: u64 = bindings::netdev_priv_flags_IFF_DONT_BRIDGE;
+    /// Disables netpoll at run-time.
+    pub const IFF_DISABLE_NETPOLL: u64 = bindings::netdev_priv_flags_IFF_DISABLE_NETPOLL;
+    /// Device used as macvlan port.
+    pub const IFF_MACVLAN_PORT: u64 = bindings::netdev_priv_flags_IFF_MACVLAN_PORT;
+    /// Device used as bridge port.
+    pub const IFF_BRIDGE_PORT: u64 = bindings::netdev_priv_flags_IFF_BRIDGE_PORT;
+    /// Device used as Open vSwitch datapath port.
+    pub const IFF_OVS_DATAPATH: u64 = bindings::netdev_priv_flags_IFF_OVS_DATAPATH;
+    /// The interface supports sharing skbs on transmit.
+    pub const IFF_TX_SKB_SHARING: u64 = bindings::netdev_priv_flags_IFF_TX_SKB_SHARING;
+    /// Supports unicast filtering.
+    pub const IFF_UNICAST_FLT: u64 = bindings::netdev_priv_flags_IFF_UNICAST_FLT;
+    /// Device used as team port.
+    pub const IFF_TEAM_PORT: u64 = bindings::netdev_priv_flags_IFF_TEAM_PORT;
+    /// Device supports sending custom FCS.
+    pub const IFF_SUPP_NOFCS: u64 = bindings::netdev_priv_flags_IFF_SUPP_NOFCS;
+    /// Device supports hardware address change when it's running.
+    pub const IFF_LIVE_ADDR_CHANGE: u64 = bindings::netdev_priv_flags_IFF_LIVE_ADDR_CHANGE;
+    /// Macvlan device.
+    pub const IFF_MACVLAN: u64 = bindings::netdev_priv_flags_IFF_MACVLAN;
+    /// IFF_XMIT_DST_RELEASE not taking into account underlying stacked devices.
+    pub const IFF_XMIT_DST_RELEASE_PERM: u64 =
+        bindings::netdev_priv_flags_IFF_XMIT_DST_RELEASE_PERM;
+    /// L3 master device.
+    pub const IFF_L3MDEV_MASTER: u64 = bindings::netdev_priv_flags_IFF_L3MDEV_MASTER;
+    /// Device can run without qdisc attached.
+    pub const IFF_NO_QUEUE: u64 = bindings::netdev_priv_flags_IFF_NO_QUEUE;
+    /// Device is a Open vSwitch master.
+    pub const IFF_OPENVSWITCH: u64 = bindings::netdev_priv_flags_IFF_OPENVSWITCH;
+    /// Device is enslaved to an L3 master.
+    pub const IFF_L3MDEV_SLAVE: u64 = bindings::netdev_priv_flags_IFF_L3MDEV_SLAVE;
+    /// Team device.
+    pub const IFF_TEAM: u64 = bindings::netdev_priv_flags_IFF_TEAM;
+    /// Device has had Rx Flow indirection table configured.
+    pub const IFF_RXFH_CONFIGURED: u64 = bindings::netdev_priv_flags_IFF_RXFH_CONFIGURED;
+    /// The headroom value is controlled by an external entity.
+    pub const IFF_PHONY_HEADROOM: u64 = bindings::netdev_priv_flags_IFF_PHONY_HEADROOM;
+    /// MACsec device.
+    pub const IFF_MACSEC: u64 = bindings::netdev_priv_flags_IFF_MACSEC;
+    /// Device doesn't support the rx_handler hook.
+    pub const IFF_NO_RX_HANDLER: u64 = bindings::netdev_priv_flags_IFF_NO_RX_HANDLER;
+    /// Failover master device.
+    pub const IFF_FAILOVER: u64 = bindings::netdev_priv_flags_IFF_FAILOVER;
+    /// Lower device of a failover master device.
+    pub const IFF_FAILOVER_SLAVE: u64 = bindings::netdev_priv_flags_IFF_FAILOVER_SLAVE;
+    /// Only invokes the rx handler of L3 master device.
+    pub const IFF_L3MDEV_RX_HANDLER: u64 = bindings::netdev_priv_flags_IFF_L3MDEV_RX_HANDLER;
+    /// Prevents ipv6 addrconf.
+    pub const IFF_NO_ADDRCONF: u64 = bindings::netdev_priv_flags_IFF_NO_ADDRCONF;
+    /// Capable of xmitting frames with skb_headlen(skb) == 0.
+    pub const IFF_TX_SKB_NO_LINEAR: u64 = bindings::netdev_priv_flags_IFF_TX_SKB_NO_LINEAR;
+    /// Supports setting carrier via IFLA_PROTO_DOWN.
+    pub const IFF_CHANGE_PROTO_DOWN: u64 = bindings::netdev_priv_flags_IFF_CHANGE_PROTO_DOWN;
+}
+
 /// Corresponds to the kernel's `struct net_device`.
 ///
 /// # Invariants
@@ -42,6 +150,81 @@ fn priv_data_ptr(&self) -> *const c_void {
         // 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) }
     }
+
+    /// Sets the name of a device.
+    pub fn set_name(&mut self, name: &CStr) -> Result {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            if name.len() > (*self.0).name.len() {
+                return Err(code::EINVAL);
+            }
+            for i in 0..name.len() {
+                (*self.0).name[i] = name[i] as i8;
+            }
+        }
+        Ok(())
+    }
+
+    /// Sets carrier.
+    pub fn netif_carrier_on(&mut self) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { bindings::netif_carrier_on(self.0) }
+    }
+
+    /// Clears carrier.
+    pub fn netif_carrier_off(&mut self) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { bindings::netif_carrier_off(self.0) }
+    }
+
+    /// Sets the max mtu of the device.
+    pub fn set_max_mtu(&mut self, max_mtu: u32) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            (*self.0).max_mtu = max_mtu;
+        }
+    }
+
+    /// Sets the minimum mtu of the device.
+    pub fn set_min_mtu(&mut self, min_mtu: u32) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            (*self.0).min_mtu = min_mtu;
+        }
+    }
+
+    /// Returns the flags of the device.
+    pub fn get_flags(&self) -> u32 {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { (*self.0).flags }
+    }
+
+    /// Sets the flags of the device.
+    pub fn set_flags(&mut self, flags: u32) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            (*self.0).flags = flags;
+        }
+    }
+
+    /// Returns the priv_flags of the device.
+    pub fn get_priv_flags(&self) -> u64 {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { (*self.0).priv_flags }
+    }
+
+    /// Sets the priv_flags of the device.
+    pub fn set_priv_flags(&mut self, flags: u64) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { (*self.0).priv_flags = flags }
+    }
+
+    /// Generate a random Ethernet address (MAC) to be used by a net device
+    /// and set addr_assign_type.
+    pub fn set_random_eth_hw_addr(&mut self) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { bindings::eth_hw_addr_random(self.0) }
+    }
 }
 
 // SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used
-- 
2.34.1

[PATCH 5/5] samples: rust: add dummy network driver

[FUJITA Tomonori]

This is a simpler version of drivers/net/dummy.c.

This demonstrates the usage of abstractions for network device drivers.

Allows allocator_api feature for Box::try_new();

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 samples/rust/Kconfig           | 12 +++++
 samples/rust/Makefile          |  1 +
 samples/rust/rust_net_dummy.rs | 81 ++++++++++++++++++++++++++++++++++
 scripts/Makefile.build         |  2 +-
 4 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100644 samples/rust/rust_net_dummy.rs

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9..8b52ba620ae3 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -30,6 +30,18 @@ config SAMPLE_RUST_PRINT
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_NET_DUMMY
+	tristate "Dummy network driver"
+	depends on NET
+	help
+	  This is the simpler version of drivers/net/dummy.c. No intention to replace it.
+	  This provides educational information for Rust abstractions for network drivers.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_minimal.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_HOSTPROGS
 	bool "Host programs"
 	help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 03086dabbea4..440dee2971ba 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -2,5 +2,6 @@
 
 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
+obj-$(CONFIG_SAMPLE_RUST_NET_DUMMY)		+= rust_net_dummy.o
 
 subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS)		+= hostprogs
diff --git a/samples/rust/rust_net_dummy.rs b/samples/rust/rust_net_dummy.rs
new file mode 100644
index 000000000000..6c49a7ba7ba2
--- /dev/null
+++ b/samples/rust/rust_net_dummy.rs
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+//! Rust dummy netdev.
+
+use kernel::{
+    c_str,
+    net::dev::{
+        ethtool_op_get_ts_info, flags, priv_flags, Device, DeviceOperations, DriverData,
+        EtherOperations, EthtoolTsInfo, Registration, RtnlLinkStats64, SkBuff, TxCode,
+    },
+    prelude::*,
+};
+
+module! {
+    type: DummyNetdev,
+    name: "rust_net_dummy",
+    author: "Rust for Linux Contributors",
+    description: "Rust dummy netdev",
+    license: "GPL v2",
+}
+
+struct DevOps {}
+
+#[vtable]
+impl<D: DriverData<Data = Box<Stats>>> DeviceOperations<D> for DevOps {
+    fn init(_dev: &mut Device, _data: &Stats) -> Result {
+        Ok(())
+    }
+
+    fn start_xmit(_dev: &mut Device, _data: &Stats, mut skb: SkBuff) -> TxCode {
+        skb.tx_timestamp();
+        TxCode::Ok
+    }
+
+    fn get_stats64(_dev: &mut Device, _data: &Stats, _stats: &mut RtnlLinkStats64) {}
+}
+
+/// For device driver specific information.
+struct Stats {}
+
+impl DriverData for Stats {
+    type Data = Box<Stats>;
+}
+
+struct DummyNetdev {
+    _r: Registration<DevOps, Stats>,
+}
+
+struct EtherOps {}
+
+#[vtable]
+impl<D: DriverData<Data = Box<Stats>>> EtherOperations<D> for EtherOps {
+    fn get_ts_info(dev: &mut Device, _data: &Stats, info: &mut EthtoolTsInfo) -> Result {
+        ethtool_op_get_ts_info(dev, info)
+    }
+}
+
+impl kernel::Module for DummyNetdev {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        let data = Box::try_new(Stats {})?;
+        let mut r = Registration::<DevOps, Stats>::try_new_ether(1, 1, data)?;
+        r.set_ether_operations::<EtherOps>()?;
+
+        let netdev = r.dev_get();
+        netdev.set_name(c_str!("dummy%d"))?;
+
+        netdev.set_flags(netdev.get_flags() | flags::IFF_NOARP & !flags::IFF_MULTICAST);
+        netdev.set_priv_flags(
+            netdev.get_priv_flags() | priv_flags::IFF_LIVE_ADDR_CHANGE | priv_flags::IFF_NO_QUEUE,
+        );
+        netdev.set_random_eth_hw_addr();
+        netdev.set_min_mtu(0);
+        netdev.set_max_mtu(0);
+
+        r.register()?;
+
+        // TODO: Replaces pr_info with the wrapper of netdev_info().
+        pr_info!("Hello Rust dummy netdev!");
+        Ok(DummyNetdev { _r: r })
+    }
+}
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 78175231c969..1404967e908e 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -277,7 +277,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
 # Compile Rust sources (.rs)
 # ---------------------------------------------------------------------------
 
-rust_allowed_features := new_uninit
+rust_allowed_features := allocator_api,new_uninit
 
 rust_common_cmd = \
 	RUST_MODFILE=$(modfile) $(RUSTC_OR_CLIPPY) $(rust_flags) \
-- 
2.34.1

Re: [PATCH 2/5] rust: add support for ethernet operations

[Ariel Miculas]

You have a typo in the commit message: net_devicve_ops should be net_device_ops

On Tue, Jun 13, 2023 at 8:20 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> This improves abstractions for network device drivers to implement
> struct ethtool_ops, the majority of ethernet device drivers need to
> do.
>
> struct ethtool_ops also needs to access to device private data like
> struct net_devicve_ops.
>
> Currently, only get_ts_info operation is supported. The following
> patch adds the Rust version of the dummy network driver, which uses
> the operation.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/kernel/net/dev.rs          | 108 ++++++++++++++++++++++++++++++++
>  2 files changed, 109 insertions(+)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 468bf606f174..6446ff764980 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -8,6 +8,7 @@
>
>  #include <linux/errname.h>
>  #include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
>  #include <linux/netdevice.h>
>  #include <linux/slab.h>
>  #include <linux/refcount.h>
> diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
> index d072c81f99ce..d6012b2eea33 100644
> --- a/rust/kernel/net/dev.rs
> +++ b/rust/kernel/net/dev.rs
> @@ -141,6 +141,18 @@ pub fn register(&mut self) -> Result {
>          }
>      }
>
> +    /// Sets `ethtool_ops` of `net_device`.
> +    pub fn set_ether_operations<U>(&mut self) -> Result
> +    where
> +        U: EtherOperations<D>,
> +    {
> +        if self.is_registered {
> +            return Err(code::EINVAL);
> +        }
> +        EtherOperationsAdapter::<U, D>::new().set_ether_ops(&mut self.dev);
> +        Ok(())
> +    }
> +
>      const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
>          ndo_init: if <T>::HAS_INIT {
>              Some(Self::init_callback)
> @@ -342,3 +354,99 @@ fn drop(&mut self) {
>          }
>      }
>  }
> +
> +/// Builds the kernel's `struct ethtool_ops`.
> +struct EtherOperationsAdapter<D, T> {
> +    _p: PhantomData<(D, T)>,
> +}
> +
> +impl<D, T> EtherOperationsAdapter<T, D>
> +where
> +    D: DriverData,
> +    T: EtherOperations<D>,
> +{
> +    /// Creates a new instance.
> +    fn new() -> Self {
> +        EtherOperationsAdapter { _p: PhantomData }
> +    }
> +
> +    unsafe extern "C" fn get_ts_info_callback(
> +        netdev: *mut bindings::net_device,
> +        info: *mut bindings::ethtool_ts_info,
> +    ) -> 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()) };
> +            // SAFETY: The C API guarantees that `info` is valid while this function is running.
> +            let mut info = unsafe { EthtoolTsInfo::from_ptr(info) };
> +            T::get_ts_info(&mut dev, data, &mut info)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    const ETHER_OPS: bindings::ethtool_ops = bindings::ethtool_ops {
> +        get_ts_info: if <T>::HAS_GET_TS_INFO {
> +            Some(Self::get_ts_info_callback)
> +        } else {
> +            None
> +        },
> +        ..unsafe { core::mem::MaybeUninit::<bindings::ethtool_ops>::zeroed().assume_init() }
> +    };
> +
> +    const fn build_ether_ops() -> &'static bindings::ethtool_ops {
> +        &Self::ETHER_OPS
> +    }
> +
> +    fn set_ether_ops(&self, dev: &mut Device) {
> +        // SAFETY: The type invariants guarantee that `dev.0` is valid.
> +        unsafe {
> +            (*dev.0).ethtool_ops = Self::build_ether_ops();
> +        }
> +    }
> +}
> +
> +/// Corresponds to the kernel's `struct ethtool_ops`.
> +#[vtable]
> +pub trait EtherOperations<D: DriverData> {
> +    /// Corresponds to `get_ts_info` in `struct ethtool_ops`.
> +    fn get_ts_info(
> +        _dev: &mut Device,
> +        _data: <D::Data as ForeignOwnable>::Borrowed<'_>,
> +        _info: &mut EthtoolTsInfo,
> +    ) -> Result {
> +        Err(Error::from_errno(bindings::EOPNOTSUPP as i32))
> +    }
> +}
> +
> +/// Corresponds to the kernel's `struct ethtool_ts_info`.
> +///
> +/// # Invariants
> +///
> +/// The pointer is valid.
> +pub struct EthtoolTsInfo(*mut bindings::ethtool_ts_info);
> +
> +impl EthtoolTsInfo {
> +    /// Creates a new `EthtoolTsInfo' instance.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `ptr` must be valid.
> +    unsafe fn from_ptr(ptr: *mut bindings::ethtool_ts_info) -> Self {
> +        // INVARIANT: The safety requirements ensure the invariant.
> +        Self(ptr)
> +    }
> +}
> +
> +/// Sets up the info for software timestamping.
> +pub fn ethtool_op_get_ts_info(dev: &mut Device, info: &mut EthtoolTsInfo) -> Result {
> +    // SAFETY: The type invariants guarantee that `dev.0` and `info.0` are valid.
> +    unsafe {
> +        bindings::ethtool_op_get_ts_info(dev.0, info.0);
> +    }
> +    Ok(())
> +}
> --
> 2.34.1
>

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Jakub Kicinski]

On Tue, 13 Jun 2023 13:53:21 +0900 FUJITA Tomonori wrote:
> This patchset adds minimum Rust abstractions for network device
> drivers and an example of a Rust network device driver, a simpler
> version of drivers/net/dummy.c.
> 
> The dummy network device driver doesn't attach any bus such as PCI so
> the dependency is minimum. Hopefully, it would make reviewing easier.
> 
> Thanks a lot for reviewing on RFC patchset at rust-for-linux ml.
> Hopefully, I've addressed all the issues.

First things first, what are the current expectations for subsystems
accepting rust code?

I was hoping someone from the Rust side is going to review this.
We try to review stuff within 48h at netdev, and there's no review :S

My immediate instinct is that I'd rather not merge toy implementations
unless someone within the netdev community can vouch for the code.

>  rust/bindings/bindings_helper.h |   3 +
>  rust/helpers.c                  |  23 ++
>  rust/kernel/lib.rs              |   3 +
>  rust/kernel/net.rs              |   5 +
>  rust/kernel/net/dev.rs          | 697 ++++++++++++++++++++++++++++++++
>  samples/rust/Kconfig            |  12 +
>  samples/rust/Makefile           |   1 +
>  samples/rust/rust_net_dummy.rs  |  81 ++++
>  scripts/Makefile.build          |   2 +-

You seem to create a rust/net/ directory without adding anything 
to MAINTAINERS. Are we building a parallel directory structure?
Are the maintainers also different?
-- 
pw-bot: defer

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Miguel Ojeda]

On Thu, Jun 15, 2023 at 8:01 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> First things first, what are the current expectations for subsystems
> accepting rust code?

If you mean generally: it is up to each subsystem to decide whether to
accept (or not) Rust code. Though we ask maintainers to make an effort
to be flexible when things are so "core" that they could potentially
block all other work, in order to conduct the Rust experiment and to
try things out.

On our side, we have a few guidelines for contributors [1].

[1] https://rust-for-linux.com/contributing#the-rust-subsystem

> I was hoping someone from the Rust side is going to review this.
> We try to review stuff within 48h at netdev, and there's no review :S

I think the version number got reset, but Tomonori had a couple
versions on the rust-for-linux@vger list [2][3].

Andrew Lunn was taking a look, and there were some other comments going on, too.

The email threading is broken in [2][3], though, so it may be easiest
to use a query like "f:lunn" [4] to find those.

[2] https://lore.kernel.org/rust-for-linux/01010188843258ec-552cca54-4849-4424-b671-7a5bf9b8651a-000000@us-west-2.amazonses.com/
[3] https://lore.kernel.org/rust-for-linux/01010188a42d5244-fffbd047-446b-4cbf-8a62-9c036d177276-000000@us-west-2.amazonses.com/
[4] https://lore.kernel.org/rust-for-linux/?q=f%3Alunn

> My immediate instinct is that I'd rather not merge toy implementations
> unless someone within the netdev community can vouch for the code.

Yes, in general, the goal is that maintainers actually understand what
is getting merged, get involved, etc. So patch submitters of Rust
code, at this time, should be expected/ready to explain Rust if
needed. We can also help from the Rust subsystem side on that.

But, yeah, knowledgeable people should review the code.

> You seem to create a rust/net/ directory without adding anything
> to MAINTAINERS. Are we building a parallel directory structure?
> Are the maintainers also different?

The plan is to split the `kernel` crate and move the files to their
proper subsystems if the experiment goes well.

But, indeed, it is best if a `F:` entry is added wherever you think it
is best. Some subsystems may just add it to their entry (e.g. KUnit
wants to do that). Others may decide to split the Rust part into
another entry, so that maintainers may be a subset (or a different set
-- sometimes this could be done, for instance, if a new maintainer
shows up that wants to take care of the Rust abstractions).

Cheers,
Miguel

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Andrew Lunn]

On Wed, Jun 14, 2023 at 11:01:28PM -0700, Jakub Kicinski wrote:
> On Tue, 13 Jun 2023 13:53:21 +0900 FUJITA Tomonori wrote:
> > This patchset adds minimum Rust abstractions for network device
> > drivers and an example of a Rust network device driver, a simpler
> > version of drivers/net/dummy.c.
> > 
> > The dummy network device driver doesn't attach any bus such as PCI so
> > the dependency is minimum. Hopefully, it would make reviewing easier.
> > 
> > Thanks a lot for reviewing on RFC patchset at rust-for-linux ml.
> > Hopefully, I've addressed all the issues.
> 
> First things first, what are the current expectations for subsystems
> accepting rust code?
> 
> I was hoping someone from the Rust side is going to review this.
> We try to review stuff within 48h at netdev, and there's no review :S

As pointed out elsewhere, i've looked the code over. I cannot say
anything about the rust, but i don't see anything too obviously wrong
with the way it use a few netdev API calls.

> My immediate instinct is that I'd rather not merge toy implementations
> unless someone within the netdev community can vouch for the code.

It is definitely toy. But you have to start somewhere.

What might be useful is an idea of the roadmap. How does this go from
toy to something useful?

I see two different threads of work.

One is getting enough in place to find where rust is lacking. netdev
uses a lot of inline C functions, which don't seem to map too well to
Rust. And rust does not seem to have the concept of per CPU memory,
needed for statistics. So it would be good to be able to have some
code which can be profiled to see how bad this actually is, and then
provide a test bed for finding solutions for these problems.

The second is just wrapping more API calls to allow more capable
drivers to be written. Implementing the loopback driver seems like the
next obvious step. Then maybe a big jump to virtio_net? Like dummy and
loopback, it is something which everybody can have, no physical
hardware needed. It also has a lot of netdev features, NAPI, RSS,
statistics, BPF & XDP. So it is a good playground, both for features
and to profiling and performance optimisation.

    Andrew

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[Benno Lossin]

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@gmail.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?

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.

> +    }
> +}
> +
> +// 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.

> +}
> +
> +/// 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.

> +            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.

> +        let ptr = from_err_ptr(unsafe {
> +            bindings::alloc_etherdev_mqs(
> +                core::mem::size_of::<*const c_void>() as i32,
> +                tx_queue_size,
> +                rx_queue_size,
> +            )
> +        })?;
> +
> +        // SAFETY: `ptr` is valid and non-null since `alloc_etherdev_mqs()`
> +        // returned a valid pointer which was null-checked.
> +        let dev = unsafe { Device::from_ptr(ptr) };
> +        // SAFETY: It's safe to write an address to the returned pointer
> +        // from `netdev_priv()` because `alloc_etherdev_mqs()` allocates
> +        // contiguous memory for `struct net_device` and a pointer.
> +        unsafe {
> +            let priv_ptr = bindings::netdev_priv(ptr) as *mut *const c_void;
> +            core::ptr::write(priv_ptr, data.into_foreign());
> +        }
> +        Ok(Registration {
> +            dev,
> +            is_registered: false,
> +            _p: PhantomData,
> +        })
> +    }
> +
> +    /// Returns a network device.
> +    ///
> +    /// A device driver normally configures the device before registration.
> +    pub fn dev_get(&mut self) -> &mut Device {
> +        &mut self.dev
> +    }
> +
> +    /// Registers a network device.
> +    pub fn register(&mut self) -> Result {
> +        if self.is_registered {
> +            return Err(code::EINVAL);
> +        }
> +        // SAFETY: The type invariants guarantee that `self.dev.0` is valid.
> +        let ret = unsafe {
> +            (*self.dev.0).netdev_ops = Self::build_device_ops();
> +            bindings::register_netdev(self.dev.0)
> +        };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            self.is_registered = true;
> +            Ok(())
> +        }
> +    }
> +
> +    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?

> +
> +    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?

> +            // 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`.

> +            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?

> +    /// 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?

> +        Ok(())
> +    }
> +
> +    /// Corresponds to `ndo_uninit` in `struct net_device_ops`.
> +    fn uninit(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) {}
> +
> +    /// Corresponds to `ndo_open` in `struct net_device_ops`.
> +    fn open(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result {
> +        Ok(())
> +    }
> +
> +    /// Corresponds to `ndo_stop` in `struct net_device_ops`.
> +    fn stop(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result {
> +        Ok(())
> +    }
> +
> +    /// Corresponds to `ndo_start_xmit` in `struct net_device_ops`.
> +    fn start_xmit(
> +        _dev: &mut Device,
> +        _data: <D::Data as ForeignOwnable>::Borrowed<'_>,
> +        _skb: SkBuff,
> +    ) -> TxCode {
> +        TxCode::Busy
> +    }
> +}
> +
> +/// 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?

--
Cheers,
Benno

> +        }
> +    }
> +}
> --
> 2.34.1
>

Re: [PATCH 2/5] rust: add support for ethernet operations

[Benno Lossin]

On 6/13/23 06:53, FUJITA Tomonori wrote:
> This improves abstractions for network device drivers to implement
> struct ethtool_ops, the majority of ethernet device drivers need to
> do.
> 
> struct ethtool_ops also needs to access to device private data like
> struct net_devicve_ops.
> 
> Currently, only get_ts_info operation is supported. The following
> patch adds the Rust version of the dummy network driver, which uses
> the operation.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/kernel/net/dev.rs          | 108 ++++++++++++++++++++++++++++++++
>  2 files changed, 109 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 468bf606f174..6446ff764980 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -8,6 +8,7 @@
> 
>  #include <linux/errname.h>
>  #include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
>  #include <linux/netdevice.h>
>  #include <linux/slab.h>
>  #include <linux/refcount.h>
> diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
> index d072c81f99ce..d6012b2eea33 100644
> --- a/rust/kernel/net/dev.rs
> +++ b/rust/kernel/net/dev.rs
> @@ -141,6 +141,18 @@ pub fn register(&mut self) -> Result {
>          }
>      }
> 
> +    /// Sets `ethtool_ops` of `net_device`.
> +    pub fn set_ether_operations<U>(&mut self) -> Result
> +    where
> +        U: EtherOperations<D>,
> +    {
> +        if self.is_registered {
> +            return Err(code::EINVAL);
> +        }
> +        EtherOperationsAdapter::<U, D>::new().set_ether_ops(&mut self.dev);
> +        Ok(())
> +    }

Is it really necessary for a device to be able to have multiple different
`EtherOperations`? Couldnt you also just make `T` implement 
`EtherOperations<D>`?

> +
>      const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
>          ndo_init: if <T>::HAS_INIT {
>              Some(Self::init_callback)
> @@ -342,3 +354,99 @@ fn drop(&mut self) {
>          }
>      }
>  }
> +
> +/// Builds the kernel's `struct ethtool_ops`.
> +struct EtherOperationsAdapter<D, T> {
> +    _p: PhantomData<(D, T)>,
> +}
> +
> +impl<D, T> EtherOperationsAdapter<T, D>

I think it would be a good idea to add a comment here that explains why
this is needed, i.e. why `ETHER_OPS` cannot be implemented via a constant
in `set_ether_ops`.

> +where
> +    D: DriverData,
> +    T: EtherOperations<D>,
> +{
> +    /// Creates a new instance.
> +    fn new() -> Self {
> +        EtherOperationsAdapter { _p: PhantomData }
> +    }
> +
> +    unsafe extern "C" fn get_ts_info_callback(
> +        netdev: *mut bindings::net_device,
> +        info: *mut bindings::ethtool_ts_info,
> +    ) -> 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()) };
> +            // SAFETY: The C API guarantees that `info` is valid while this function is running.
> +            let mut info = unsafe { EthtoolTsInfo::from_ptr(info) };
> +            T::get_ts_info(&mut dev, data, &mut info)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    const ETHER_OPS: bindings::ethtool_ops = bindings::ethtool_ops {
> +        get_ts_info: if <T>::HAS_GET_TS_INFO {
> +            Some(Self::get_ts_info_callback)
> +        } else {
> +            None
> +        },
> +        ..unsafe { core::mem::MaybeUninit::<bindings::ethtool_ops>::zeroed().assume_init() }
> +    };
> +
> +    const fn build_ether_ops() -> &'static bindings::ethtool_ops {
> +        &Self::ETHER_OPS
> +    }

Why is this a function?

> +
> +    fn set_ether_ops(&self, dev: &mut Device) {
> +        // SAFETY: The type invariants guarantee that `dev.0` is valid.
> +        unsafe {
> +            (*dev.0).ethtool_ops = Self::build_ether_ops();
> +        }
> +    }
> +}
> +
> +/// Corresponds to the kernel's `struct ethtool_ops`.
> +#[vtable]
> +pub trait EtherOperations<D: DriverData> {
> +    /// Corresponds to `get_ts_info` in `struct ethtool_ops`.
> +    fn get_ts_info(
> +        _dev: &mut Device,
> +        _data: <D::Data as ForeignOwnable>::Borrowed<'_>,
> +        _info: &mut EthtoolTsInfo,

`&mut EthtoolTsInfo` is again a double pointer.

> +    ) -> Result {
> +        Err(Error::from_errno(bindings::EOPNOTSUPP as i32))
> +    }
> +}
> +
> +/// Corresponds to the kernel's `struct ethtool_ts_info`.
> +///
> +/// # Invariants
> +///
> +/// The pointer is valid.
> +pub struct EthtoolTsInfo(*mut bindings::ethtool_ts_info);
> +
> +impl EthtoolTsInfo {
> +    /// Creates a new `EthtoolTsInfo' instance.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `ptr` must be valid.
> +    unsafe fn from_ptr(ptr: *mut bindings::ethtool_ts_info) -> Self {
> +        // INVARIANT: The safety requirements ensure the invariant.
> +        Self(ptr)
> +    }
> +}
> +
> +/// Sets up the info for software timestamping.
> +pub fn ethtool_op_get_ts_info(dev: &mut Device, info: &mut EthtoolTsInfo) -> Result {
> +    // SAFETY: The type invariants guarantee that `dev.0` and `info.0` are valid.
> +    unsafe {
> +        bindings::ethtool_op_get_ts_info(dev.0, info.0);
> +    }
> +    Ok(())
> +}

Why isn't this an associated function on `EthtoolTsInfo`?

--
Cheers,
Benno

> --
> 2.34.1
>

Re: [PATCH 4/5] rust: add methods for configure net_device

[Benno Lossin]

On 6/13/23 06:53, FUJITA Tomonori wrote:
> adds methods to net::Device for the basic configurations of
> net_device.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/helpers.c         |   7 ++
>  rust/kernel/net/dev.rs | 183 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 190 insertions(+)
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 70d50767ff4e..6c51deb18dc1 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -22,6 +22,7 @@
>  #include <linux/build_bug.h>
>  #include <linux/err.h>
>  #include <linux/errname.h>
> +#include <linux/etherdevice.h>
>  #include <linux/refcount.h>
>  #include <linux/mutex.h>
>  #include <linux/netdevice.h>
> @@ -31,6 +32,12 @@
>  #include <linux/wait.h>
> 
>  #ifdef CONFIG_NET
> +void rust_helper_eth_hw_addr_random(struct net_device *dev)
> +{
> +	eth_hw_addr_random(dev);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_eth_hw_addr_random);
> +
>  void *rust_helper_netdev_priv(const struct net_device *dev)
>  {
>  	return netdev_priv(dev);
> diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
> index 452944cf9fb8..4767f331973e 100644
> --- a/rust/kernel/net/dev.rs
> +++ b/rust/kernel/net/dev.rs
> @@ -12,10 +12,118 @@
>      bindings,
>      error::*,
>      prelude::vtable,
> +    str::CStr,
>      types::{ForeignOwnable, Opaque},
>  };
>  use {core::ffi::c_void, core::marker::PhantomData};
> 
> +/// Flags associated with a [`Device`].
> +pub mod flags {
> +    /// Interface is up.
> +    pub const IFF_UP: u32 = bindings::net_device_flags_IFF_UP;
> +    /// Broadcast address valid.
> +    pub const IFF_BROADCAST: u32 = bindings::net_device_flags_IFF_BROADCAST;
> +    /// Device on debugging.
> +    pub const IFF_DEBUG: u32 = bindings::net_device_flags_IFF_DEBUG;
> +    /// Loopback device.
> +    pub const IFF_LOOPBACK: u32 = bindings::net_device_flags_IFF_LOOPBACK;
> +    /// Has p-p link.
> +    pub const IFF_POINTOPOINT: u32 = bindings::net_device_flags_IFF_POINTOPOINT;
> +    /// Avoids use of trailers.
> +    pub const IFF_NOTRAILERS: u32 = bindings::net_device_flags_IFF_NOTRAILERS;
> +    /// Interface RFC2863 OPER_UP.
> +    pub const IFF_RUNNING: u32 = bindings::net_device_flags_IFF_RUNNING;
> +    /// No ARP protocol.
> +    pub const IFF_NOARP: u32 = bindings::net_device_flags_IFF_NOARP;
> +    /// Receives all packets.
> +    pub const IFF_PROMISC: u32 = bindings::net_device_flags_IFF_PROMISC;
> +    /// Receive all multicast packets.
> +    pub const IFF_ALLMULTI: u32 = bindings::net_device_flags_IFF_ALLMULTI;
> +    /// Master of a load balancer.
> +    pub const IFF_MASTER: u32 = bindings::net_device_flags_IFF_MASTER;
> +    /// Slave of a load balancer.
> +    pub const IFF_SLAVE: u32 = bindings::net_device_flags_IFF_SLAVE;
> +    /// Supports multicast.
> +    pub const IFF_MULTICAST: u32 = bindings::net_device_flags_IFF_MULTICAST;
> +    /// Capable of setting media type.
> +    pub const IFF_PORTSEL: u32 = bindings::net_device_flags_IFF_PORTSEL;
> +    /// Auto media select active.
> +    pub const IFF_AUTOMEDIA: u32 = bindings::net_device_flags_IFF_AUTOMEDIA;
> +    /// Dialup device with changing addresses.
> +    pub const IFF_DYNAMIC: u32 = bindings::net_device_flags_IFF_DYNAMIC;
> +}
> +
> +/// Private flags associated with a [`Device`].
> +pub mod priv_flags {
> +    /// 802.1Q VLAN device.
> +    pub const IFF_802_1Q_VLAN: u64 = bindings::netdev_priv_flags_IFF_802_1Q_VLAN;
> +    /// Ethernet bridging device.
> +    pub const IFF_EBRIDGE: u64 = bindings::netdev_priv_flags_IFF_EBRIDGE;
> +    /// Bonding master or slave device.
> +    pub const IFF_BONDING: u64 = bindings::netdev_priv_flags_IFF_BONDING;
> +    /// ISATAP interface (RFC4214).
> +    pub const IFF_ISATAP: u64 = bindings::netdev_priv_flags_IFF_ISATAP;
> +    /// WAN HDLC device.
> +    pub const IFF_WAN_HDLC: u64 = bindings::netdev_priv_flags_IFF_WAN_HDLC;
> +    /// dev_hard_start_xmit() is allowed to release skb->dst.
> +    pub const IFF_XMIT_DST_RELEASE: u64 = bindings::netdev_priv_flags_IFF_XMIT_DST_RELEASE;
> +    /// Disallows bridging this ether device.
> +    pub const IFF_DONT_BRIDGE: u64 = bindings::netdev_priv_flags_IFF_DONT_BRIDGE;
> +    /// Disables netpoll at run-time.
> +    pub const IFF_DISABLE_NETPOLL: u64 = bindings::netdev_priv_flags_IFF_DISABLE_NETPOLL;
> +    /// Device used as macvlan port.
> +    pub const IFF_MACVLAN_PORT: u64 = bindings::netdev_priv_flags_IFF_MACVLAN_PORT;
> +    /// Device used as bridge port.
> +    pub const IFF_BRIDGE_PORT: u64 = bindings::netdev_priv_flags_IFF_BRIDGE_PORT;
> +    /// Device used as Open vSwitch datapath port.
> +    pub const IFF_OVS_DATAPATH: u64 = bindings::netdev_priv_flags_IFF_OVS_DATAPATH;
> +    /// The interface supports sharing skbs on transmit.
> +    pub const IFF_TX_SKB_SHARING: u64 = bindings::netdev_priv_flags_IFF_TX_SKB_SHARING;
> +    /// Supports unicast filtering.
> +    pub const IFF_UNICAST_FLT: u64 = bindings::netdev_priv_flags_IFF_UNICAST_FLT;
> +    /// Device used as team port.
> +    pub const IFF_TEAM_PORT: u64 = bindings::netdev_priv_flags_IFF_TEAM_PORT;
> +    /// Device supports sending custom FCS.
> +    pub const IFF_SUPP_NOFCS: u64 = bindings::netdev_priv_flags_IFF_SUPP_NOFCS;
> +    /// Device supports hardware address change when it's running.
> +    pub const IFF_LIVE_ADDR_CHANGE: u64 = bindings::netdev_priv_flags_IFF_LIVE_ADDR_CHANGE;
> +    /// Macvlan device.
> +    pub const IFF_MACVLAN: u64 = bindings::netdev_priv_flags_IFF_MACVLAN;
> +    /// IFF_XMIT_DST_RELEASE not taking into account underlying stacked devices.
> +    pub const IFF_XMIT_DST_RELEASE_PERM: u64 =
> +        bindings::netdev_priv_flags_IFF_XMIT_DST_RELEASE_PERM;
> +    /// L3 master device.
> +    pub const IFF_L3MDEV_MASTER: u64 = bindings::netdev_priv_flags_IFF_L3MDEV_MASTER;
> +    /// Device can run without qdisc attached.
> +    pub const IFF_NO_QUEUE: u64 = bindings::netdev_priv_flags_IFF_NO_QUEUE;
> +    /// Device is a Open vSwitch master.
> +    pub const IFF_OPENVSWITCH: u64 = bindings::netdev_priv_flags_IFF_OPENVSWITCH;
> +    /// Device is enslaved to an L3 master.
> +    pub const IFF_L3MDEV_SLAVE: u64 = bindings::netdev_priv_flags_IFF_L3MDEV_SLAVE;
> +    /// Team device.
> +    pub const IFF_TEAM: u64 = bindings::netdev_priv_flags_IFF_TEAM;
> +    /// Device has had Rx Flow indirection table configured.
> +    pub const IFF_RXFH_CONFIGURED: u64 = bindings::netdev_priv_flags_IFF_RXFH_CONFIGURED;
> +    /// The headroom value is controlled by an external entity.
> +    pub const IFF_PHONY_HEADROOM: u64 = bindings::netdev_priv_flags_IFF_PHONY_HEADROOM;
> +    /// MACsec device.
> +    pub const IFF_MACSEC: u64 = bindings::netdev_priv_flags_IFF_MACSEC;
> +    /// Device doesn't support the rx_handler hook.
> +    pub const IFF_NO_RX_HANDLER: u64 = bindings::netdev_priv_flags_IFF_NO_RX_HANDLER;
> +    /// Failover master device.
> +    pub const IFF_FAILOVER: u64 = bindings::netdev_priv_flags_IFF_FAILOVER;
> +    /// Lower device of a failover master device.
> +    pub const IFF_FAILOVER_SLAVE: u64 = bindings::netdev_priv_flags_IFF_FAILOVER_SLAVE;
> +    /// Only invokes the rx handler of L3 master device.
> +    pub const IFF_L3MDEV_RX_HANDLER: u64 = bindings::netdev_priv_flags_IFF_L3MDEV_RX_HANDLER;
> +    /// Prevents ipv6 addrconf.
> +    pub const IFF_NO_ADDRCONF: u64 = bindings::netdev_priv_flags_IFF_NO_ADDRCONF;
> +    /// Capable of xmitting frames with skb_headlen(skb) == 0.
> +    pub const IFF_TX_SKB_NO_LINEAR: u64 = bindings::netdev_priv_flags_IFF_TX_SKB_NO_LINEAR;
> +    /// Supports setting carrier via IFLA_PROTO_DOWN.
> +    pub const IFF_CHANGE_PROTO_DOWN: u64 = bindings::netdev_priv_flags_IFF_CHANGE_PROTO_DOWN;
> +}
> +
>  /// Corresponds to the kernel's `struct net_device`.
>  ///
>  /// # Invariants
> @@ -42,6 +150,81 @@ fn priv_data_ptr(&self) -> *const c_void {
>          // 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) }
>      }
> +
> +    /// Sets the name of a device.
> +    pub fn set_name(&mut self, name: &CStr) -> Result {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.

Note that it is not the safety requirement that ensure this, but
rather the type invariant of `Self`. This also is the case further below.

> +        unsafe {
> +            if name.len() > (*self.0).name.len() {
> +                return Err(code::EINVAL);
> +            }
> +            for i in 0..name.len() {
> +                (*self.0).name[i] = name[i] as i8;

By using array access va an index `[]` you create a mutable reference to
the name of the struct, this is probably wrong. Instead of this loop,
you should use some direct copy function.

--
Cheers,
Benno

> +            }
> +        }
> +        Ok(())
> +    }
> +
> +    /// Sets carrier.
> +    pub fn netif_carrier_on(&mut self) {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe { bindings::netif_carrier_on(self.0) }
> +    }
> +
> +    /// Clears carrier.
> +    pub fn netif_carrier_off(&mut self) {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe { bindings::netif_carrier_off(self.0) }
> +    }
> +
> +    /// Sets the max mtu of the device.
> +    pub fn set_max_mtu(&mut self, max_mtu: u32) {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe {
> +            (*self.0).max_mtu = max_mtu;
> +        }
> +    }
> +
> +    /// Sets the minimum mtu of the device.
> +    pub fn set_min_mtu(&mut self, min_mtu: u32) {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe {
> +            (*self.0).min_mtu = min_mtu;
> +        }
> +    }
> +
> +    /// Returns the flags of the device.
> +    pub fn get_flags(&self) -> u32 {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe { (*self.0).flags }
> +    }
> +
> +    /// Sets the flags of the device.
> +    pub fn set_flags(&mut self, flags: u32) {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe {
> +            (*self.0).flags = flags;
> +        }
> +    }
> +
> +    /// Returns the priv_flags of the device.
> +    pub fn get_priv_flags(&self) -> u64 {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe { (*self.0).priv_flags }
> +    }
> +
> +    /// Sets the priv_flags of the device.
> +    pub fn set_priv_flags(&mut self, flags: u64) {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe { (*self.0).priv_flags = flags }
> +    }
> +
> +    /// Generate a random Ethernet address (MAC) to be used by a net device
> +    /// and set addr_assign_type.
> +    pub fn set_random_eth_hw_addr(&mut self) {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe { bindings::eth_hw_addr_random(self.0) }
> +    }
>  }
> 
>  // SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used
> --
> 2.34.1
>

Re: [PATCH 5/5] samples: rust: add dummy network driver

[Benno Lossin]

On 6/13/23 06:53, FUJITA Tomonori wrote:
> This is a simpler version of drivers/net/dummy.c.
> 
> This demonstrates the usage of abstractions for network device drivers.
> 
> Allows allocator_api feature for Box::try_new();
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  samples/rust/Kconfig           | 12 +++++
>  samples/rust/Makefile          |  1 +
>  samples/rust/rust_net_dummy.rs | 81 ++++++++++++++++++++++++++++++++++
>  scripts/Makefile.build         |  2 +-
>  4 files changed, 95 insertions(+), 1 deletion(-)
>  create mode 100644 samples/rust/rust_net_dummy.rs
> 
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index b0f74a81c8f9..8b52ba620ae3 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -30,6 +30,18 @@ config SAMPLE_RUST_PRINT
> 
>  	 If unsure, say N.
> 
> +config SAMPLE_RUST_NET_DUMMY
> +	tristate "Dummy network driver"
> +	depends on NET
> +	help
> +	 This is the simpler version of drivers/net/dummy.c. No intention to replace it.
> +	 This provides educational information for Rust abstractions for network drivers.
> +
> +	 To compile this as a module, choose M here:
> +	 the module will be called rust_minimal.

The module is not called `rust_minimal` :)

--
Cheers,
Benno

> +
> +	 If unsure, say N.
> +
>  config SAMPLE_RUST_HOSTPROGS
>  	bool "Host programs"
>  	help
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index 03086dabbea4..440dee2971ba 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -2,5 +2,6 @@
> 
>  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
>  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
> +obj-$(CONFIG_SAMPLE_RUST_NET_DUMMY)		+= rust_net_dummy.o
> 
>  subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS)		+= hostprogs
> diff --git a/samples/rust/rust_net_dummy.rs b/samples/rust/rust_net_dummy.rs
> new file mode 100644
> index 000000000000..6c49a7ba7ba2
> --- /dev/null
> +++ b/samples/rust/rust_net_dummy.rs
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +//! Rust dummy netdev.
> +
> +use kernel::{
> +    c_str,
> +    net::dev::{
> +        ethtool_op_get_ts_info, flags, priv_flags, Device, DeviceOperations, DriverData,
> +        EtherOperations, EthtoolTsInfo, Registration, RtnlLinkStats64, SkBuff, TxCode,
> +    },
> +    prelude::*,
> +};
> +
> +module! {
> +    type: DummyNetdev,
> +    name: "rust_net_dummy",
> +    author: "Rust for Linux Contributors",
> +    description: "Rust dummy netdev",
> +    license: "GPL v2",
> +}
> +
> +struct DevOps {}
> +
> +#[vtable]
> +impl<D: DriverData<Data = Box<Stats>>> DeviceOperations<D> for DevOps {
> +    fn init(_dev: &mut Device, _data: &Stats) -> Result {
> +        Ok(())
> +    }
> +
> +    fn start_xmit(_dev: &mut Device, _data: &Stats, mut skb: SkBuff) -> TxCode {
> +        skb.tx_timestamp();
> +        TxCode::Ok
> +    }
> +
> +    fn get_stats64(_dev: &mut Device, _data: &Stats, _stats: &mut RtnlLinkStats64) {}
> +}
> +
> +/// For device driver specific information.
> +struct Stats {}
> +
> +impl DriverData for Stats {
> +    type Data = Box<Stats>;
> +}
> +
> +struct DummyNetdev {
> +    _r: Registration<DevOps, Stats>,
> +}
> +
> +struct EtherOps {}
> +
> +#[vtable]
> +impl<D: DriverData<Data = Box<Stats>>> EtherOperations<D> for EtherOps {
> +    fn get_ts_info(dev: &mut Device, _data: &Stats, info: &mut EthtoolTsInfo) -> Result {
> +        ethtool_op_get_ts_info(dev, info)
> +    }
> +}
> +
> +impl kernel::Module for DummyNetdev {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        let data = Box::try_new(Stats {})?;
> +        let mut r = Registration::<DevOps, Stats>::try_new_ether(1, 1, data)?;
> +        r.set_ether_operations::<EtherOps>()?;
> +
> +        let netdev = r.dev_get();
> +        netdev.set_name(c_str!("dummy%d"))?;
> +
> +        netdev.set_flags(netdev.get_flags() | flags::IFF_NOARP & !flags::IFF_MULTICAST);
> +        netdev.set_priv_flags(
> +            netdev.get_priv_flags() | priv_flags::IFF_LIVE_ADDR_CHANGE | priv_flags::IFF_NO_QUEUE,
> +        );
> +        netdev.set_random_eth_hw_addr();
> +        netdev.set_min_mtu(0);
> +        netdev.set_max_mtu(0);
> +
> +        r.register()?;
> +
> +        // TODO: Replaces pr_info with the wrapper of netdev_info().
> +        pr_info!("Hello Rust dummy netdev!");
> +        Ok(DummyNetdev { _r: r })
> +    }
> +}
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 78175231c969..1404967e908e 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -277,7 +277,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
>  # Compile Rust sources (.rs)
>  # ---------------------------------------------------------------------------
> 
> -rust_allowed_features := new_uninit
> +rust_allowed_features := allocator_api,new_uninit
> 
>  rust_common_cmd = \
>  	RUST_MODFILE=$(modfile) $(RUSTC_OR_CLIPPY) $(rust_flags) \
> --
> 2.34.1
>

Re: [PATCH 2/5] rust: add support for ethernet operations

[Andrew Lunn]

> > +    /// Sets `ethtool_ops` of `net_device`.
> > +    pub fn set_ether_operations<U>(&mut self) -> Result
> > +    where
> > +        U: EtherOperations<D>,
> > +    {
> > +        if self.is_registered {
> > +            return Err(code::EINVAL);
> > +        }
> > +        EtherOperationsAdapter::<U, D>::new().set_ether_ops(&mut self.dev);
> > +        Ok(())
> > +    }
> 
> Is it really necessary for a device to be able to have multiple different
> `EtherOperations`? Couldnt you also just make `T` implement 
> `EtherOperations<D>`?

For 99% of drivers, netdev->ethtool_ops is set once before
register_netdev() is called and never changed.

There is one corner case i know of, and it is a hack:

https://elixir.bootlin.com/linux/v6.4-rc6/source/net/dsa/master.c#L223

Since this is a hack, you can probably skip it in rust. 

      Andrew

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Jakub Kicinski]

On Thu, 15 Jun 2023 14:51:10 +0200 Andrew Lunn wrote:
> On Wed, Jun 14, 2023 at 11:01:28PM -0700, Jakub Kicinski wrote:
> > On Tue, 13 Jun 2023 13:53:21 +0900 FUJITA Tomonori wrote:  
> > > This patchset adds minimum Rust abstractions for network device
> > > drivers and an example of a Rust network device driver, a simpler
> > > version of drivers/net/dummy.c.
> > > 
> > > The dummy network device driver doesn't attach any bus such as PCI so
> > > the dependency is minimum. Hopefully, it would make reviewing easier.
> > > 
> > > Thanks a lot for reviewing on RFC patchset at rust-for-linux ml.
> > > Hopefully, I've addressed all the issues.  
> > 
> > First things first, what are the current expectations for subsystems
> > accepting rust code?
> > 
> > I was hoping someone from the Rust side is going to review this.
> > We try to review stuff within 48h at netdev, and there's no review :S  
> 
> As pointed out elsewhere, i've looked the code over. I cannot say
> anything about the rust, but i don't see anything too obviously wrong
> with the way it use a few netdev API calls.

The skb freeing looks shady from functional perspective.
I'm guessing some form of destructor frees the skb automatically
in xmit handler(?), but (a) no reason support, (b) kfree_skb_reason()
is most certainly not safe to call on all xmit paths...

> > My immediate instinct is that I'd rather not merge toy implementations
> > unless someone within the netdev community can vouch for the code.  
> 
> It is definitely toy. But you have to start somewhere.
> 
> What might be useful is an idea of the roadmap. How does this go from
> toy to something useful?
> 
> I see two different threads of work.
> 
> One is getting enough in place to find where rust is lacking. netdev
> uses a lot of inline C functions, which don't seem to map too well to
> Rust. And rust does not seem to have the concept of per CPU memory,
> needed for statistics. So it would be good to be able to have some
> code which can be profiled to see how bad this actually is, and then
> provide a test bed for finding solutions for these problems.
> 
> The second is just wrapping more API calls to allow more capable
> drivers to be written. Implementing the loopback driver seems like the
> next obvious step. Then maybe a big jump to virtio_net?

I'd have thought that a simple SPI driver or some such would be 
a better match.

> Like dummy and loopback, it is something which everybody can have, no
> physical hardware needed. It also has a lot of netdev features, NAPI,
> RSS, statistics, BPF & XDP. So it is a good playground, both for
> features and to profiling and performance optimisation.

IMO we have too much useless playground stuff in C already, don't get me
started. Let's get a real device prove that this thing can actually work
:( We can waste years implementing imaginary device drivers that nobody
ends up using.

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Jakub Kicinski]

On Thu, 15 Jun 2023 10:58:50 +0200 Miguel Ojeda wrote:
> On Thu, Jun 15, 2023 at 8:01 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > I was hoping someone from the Rust side is going to review this.
> > We try to review stuff within 48h at netdev, and there's no review :S  
> 
> I think the version number got reset, but Tomonori had a couple
> versions on the rust-for-linux@vger list [2][3].
> 
> Andrew Lunn was taking a look, and there were some other comments going on, too.
> 
> The email threading is broken in [2][3], though, so it may be easiest
> to use a query like "f:lunn" [4] to find those.
> 
> [2] https://lore.kernel.org/rust-for-linux/01010188843258ec-552cca54-4849-4424-b671-7a5bf9b8651a-000000@us-west-2.amazonses.com/
> [3] https://lore.kernel.org/rust-for-linux/01010188a42d5244-fffbd047-446b-4cbf-8a62-9c036d177276-000000@us-west-2.amazonses.com/
> [4] https://lore.kernel.org/rust-for-linux/?q=f%3Alunn
> 
> > My immediate instinct is that I'd rather not merge toy implementations
> > unless someone within the netdev community can vouch for the code.  
> 
> Yes, in general, the goal is that maintainers actually understand what
> is getting merged, get involved, etc. So patch submitters of Rust
> code, at this time, should be expected/ready to explain Rust if
> needed. We can also help from the Rust subsystem side on that.
> 
> But, yeah, knowledgeable people should review the code.

All sounds pretty reasonable, thanks for the pointers.

TBH I was hoping that the code will be more like reading "modern C++"
for a C developer. I can't understand much of what's going on.

Taking an example of what I have randomly on the screen as I'm writing
this email:

+    /// Updates TX stats.
+    pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) {
+        // SAFETY: We have exclusive access to the `rtnl_link_stats64`, so writing to it is okay.
+        unsafe {
+            let inner = Opaque::get(&self.0);
+            (*inner).tx_packets = packets;
+            (*inner).tx_bytes = bytes;
+            (*inner).tx_errors = errors;
+            (*inner).tx_dropped = dropped;
+        }
+    }

What is this supposed to be doing? Who needs to _set_ unrelated
statistics from a function call? Yet no reviewer is complaining
which either means I don't understand, or people aren't really 
paying attention :(

> > You seem to create a rust/net/ directory without adding anything
> > to MAINTAINERS. Are we building a parallel directory structure?
> > Are the maintainers also different?  
> 
> The plan is to split the `kernel` crate and move the files to their
> proper subsystems if the experiment goes well.

I see.

> But, indeed, it is best if a `F:` entry is added wherever you think it
> is best. Some subsystems may just add it to their entry (e.g. KUnit
> wants to do that). Others may decide to split the Rust part into
> another entry, so that maintainers may be a subset (or a different set
> -- sometimes this could be done, for instance, if a new maintainer
> shows up that wants to take care of the Rust abstractions).

I think F: would work for us.

Are there success stories in any subsystem for getting a driver for
real HW supported? I think the best way to focus the effort would be 
to set a target on a relatively simple device.

Actually Andrew is interested, and PHY drivers seem relatively simple..
/me runs away

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Richard Cochran]

On Thu, Jun 15, 2023 at 07:02:52PM -0700, Jakub Kicinski wrote:

> IMO we have too much useless playground stuff in C already, don't get me
> started. Let's get a real device prove that this thing can actually work
> :( We can waste years implementing imaginary device drivers that nobody
> ends up using.

Yes, the long standing policy seems to be not to merge "dummy"
drivers.  The original PTP patch set had one, but at the time the
reviews said firmly "No" to that.

Thanks,
Richard

Re: [PATCH 0/5] Rust abstractions for network device drivers

[FUJITA Tomonori]

Hi,

On Thu, 15 Jun 2023 19:19:31 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> TBH I was hoping that the code will be more like reading "modern C++"
> for a C developer. I can't understand much of what's going on.
> 
> Taking an example of what I have randomly on the screen as I'm writing
> this email:
> 
> +    /// Updates TX stats.
> +    pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) {
> +        // SAFETY: We have exclusive access to the `rtnl_link_stats64`, so writing to it is okay.
> +        unsafe {
> +            let inner = Opaque::get(&self.0);
> +            (*inner).tx_packets = packets;
> +            (*inner).tx_bytes = bytes;
> +            (*inner).tx_errors = errors;
> +            (*inner).tx_dropped = dropped;
> +        }
> +    }
> 
> What is this supposed to be doing? Who needs to _set_ unrelated
> statistics from a function call? Yet no reviewer is complaining
> which either means I don't understand, or people aren't really 
> paying attention :(

Sorry, this function was used in the dummy driver to implement
net_device_ops->ndo_get_stats64:

https://lore.kernel.org/rust-for-linux/01010188a025632b-16a4fb69-5601-4f46-a170-52b5f2921ed2-000000@us-west-2.amazonses.com/T/#m518550baea9c76224524e44ab3ee5a0ecd01b1b9

The old version uses atomic types in xmit path to save tx stats for
ndo_get_stats64. But Andrew said that using atomic types in xmit path
isn't a good idea in even sample driver so I removed that code.


>> But, indeed, it is best if a `F:` entry is added wherever you think it
>> is best. Some subsystems may just add it to their entry (e.g. KUnit
>> wants to do that). Others may decide to split the Rust part into
>> another entry, so that maintainers may be a subset (or a different set
>> -- sometimes this could be done, for instance, if a new maintainer
>> shows up that wants to take care of the Rust abstractions).
> 
> I think F: would work for us.
> 
> Are there success stories in any subsystem for getting a driver for
> real HW supported? I think the best way to focus the effort would be 
> to set a target on a relatively simple device.

As far as I know, no subsystem has accepted Rust bindings yet.

Replacing the existing C driver for real HW with Rust new one doesn't
make sense, right? So a necessary condition of getting Rust bindings
for a subsystem accepted is that a HW verndor implements both a driver
and bindings for their new HW?

thanks,

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Alice Ryhl]

On 6/15/23 08:01, Jakub Kicinski wrote:> I was hoping someone from the 
Rust side is going to review this.
> We try to review stuff within 48h at netdev, and there's no review :S

I'm from the Rust side, and I intend to review this, but I've taken this 
week off, so it wont be until next week.

> What is this supposed to be doing? Who needs to _set_ unrelated
> statistics from a function call? Yet no reviewer is complaining
> which either means I don't understand, or people aren't really 
> paying attention 🙁

It can sometimes be necessary to wrap loads or stores in function calls 
when working over the FFI boundary. However, Rust is very aggressive 
about inlining this kind of thing, so it shouldn't have any actual 
performance cost.

As for this being a single function rather than four functions, that's 
definitely a debatable decision. You would only do that if it makes 
sense to merge them together and if you would always assign all of them 
together. I don't know enough about these fields to say whether it makes 
sense here.

Alice

Re: [PATCH 0/5] Rust abstractions for network device drivers

[FUJITA Tomonori]

Hi,

On Thu, 15 Jun 2023 19:02:52 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 15 Jun 2023 14:51:10 +0200 Andrew Lunn wrote:
>> On Wed, Jun 14, 2023 at 11:01:28PM -0700, Jakub Kicinski wrote:
>> > On Tue, 13 Jun 2023 13:53:21 +0900 FUJITA Tomonori wrote:  
>> > > This patchset adds minimum Rust abstractions for network device
>> > > drivers and an example of a Rust network device driver, a simpler
>> > > version of drivers/net/dummy.c.
>> > > 
>> > > The dummy network device driver doesn't attach any bus such as PCI so
>> > > the dependency is minimum. Hopefully, it would make reviewing easier.
>> > > 
>> > > Thanks a lot for reviewing on RFC patchset at rust-for-linux ml.
>> > > Hopefully, I've addressed all the issues.  
>> > 
>> > First things first, what are the current expectations for subsystems
>> > accepting rust code?
>> > 
>> > I was hoping someone from the Rust side is going to review this.
>> > We try to review stuff within 48h at netdev, and there's no review :S  
>> 
>> As pointed out elsewhere, i've looked the code over. I cannot say
>> anything about the rust, but i don't see anything too obviously wrong
>> with the way it use a few netdev API calls.
> 
> The skb freeing looks shady from functional perspective.
> I'm guessing some form of destructor frees the skb automatically
> in xmit handler(?), but (a) no reason support, (b) kfree_skb_reason()
> is most certainly not safe to call on all xmit paths...

Yeah, I assume that a driver keeps a skb in private data structure
(such as tx ring) then removes the skb from it after the completion of
tx; automatically the drop() method runs (where we need to free the
skb).

I thought that calling dev_kfree_skb() is fine but no? We also need
something different for drivers that use other ways to free the skb
though.

I use kfree_skb_reason() because dev_kfree_skb() is a macro so it
can't be called directly from Rust. But I should have used
dev_kfree_skb() with a helper function.

thanks,

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Andrew Lunn]

> Actually Andrew is interested, and PHY drivers seem relatively simple..
> /me runs away

:-)

I think because they are so simple, there is not much to gain by
implementing them in rust.

	Andrew

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Andrew Lunn]

> I use kfree_skb_reason() because dev_kfree_skb() is a macro so it
> can't be called directly from Rust. But I should have used
> dev_kfree_skb() with a helper function.

I think this is something you need to get addressed at a language
level very soon. Lots of netdev API calls will be to macros. The API
to manipulate skbs is pretty much always used on the hot path, so i
expect that it will have a large number of macros. It is unclear to me
how well it will scale if you need to warp them all?

~/linux/include/linux$ grep inline skbuff.h  | wc
    349    2487   23010

Do you really want to write 300+ wrappers?

   Andrew

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Miguel Ojeda]

On Fri, Jun 16, 2023 at 4:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> TBH I was hoping that the code will be more like reading "modern C++"
> for a C developer. I can't understand much of what's going on.

Yeah, there are some subtleties in the language, especially in
`unsafe` land and dealing with FFI, but please feel free to ask. I
think if one understands the complexities of modern C++, then
high-level Rust can be mapped fairly well.

> Are there success stories in any subsystem for getting a driver for
> real HW supported? I think the best way to focus the effort would be
> to set a target on a relatively simple device.

DRM is looking into start merging things [1] soon, as far as I
understand, and the GPU driver is being used in Asahi Linux.

If we go a bit outside "real HW", but still "non-sample/toy modules",
my understanding is that Binder is also looking into merging the Rust
implementation in the next few months [2] (they are submitting the
abstractions for their dependencies already).

Then there are other efforts like the NVMe driver, which is
progressing nicely and Andreas has shown nice performance results so
far [3]. Though the upstreaming story for some of those efforts may be
less clear (for different reasons).

Of course, I don't speak for any of them -- I am just trying to give a
summary of how things are going, and a positive outlook :) In the end,
if a subsystem is willing to give things a try, we appreciate it,
because it also makes things easier for others in the future, too.

Perhaps you may want to treat it as an experiment (possibly marking
things as experimental/staging/... explicitly if needed) that allows
you to both learn Rust and whether it suits your subsystem, as well as
a way to possibly get new people/future maintainers involved.

[1] https://lore.kernel.org/rust-for-linux/20230307-rust-drm-v1-0-917ff5bc80a8@asahilina.net/
[2] https://rust-for-linux.com/android-binder-driver
[3] https://rust-for-linux.com/nvme-driver

Cheers,
Miguel

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Andrew Lunn]

> As for this being a single function rather than four functions, that's
> definitely a debatable decision. You would only do that if it makes sense to
> merge them together and if you would always assign all of them together. I
> don't know enough about these fields to say whether it makes sense here.

It can actually make sense to do them all together, because the source
of these is likely to be a per CPU data structure protected by a per
CPU sequence lock. You iterate over all CPUs, doing a transaction,
taking the sequence lock, copy the values, and then releasing the
lock. Taking and releases the lock per value is unnecessary expense.

      Andrew

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Miguel Ojeda]

On Fri, Jun 16, 2023 at 2:18 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> As far as I know, no subsystem has accepted Rust bindings yet.

For abstractions in general (see my previous reply for "real HW"
etc.), the KUnit subsystem [1] which is onboard and taking some
patches through their tree / ownership of the code.

[1] https://lore.kernel.org/rust-for-linux/CABVgOSnprvxzi-z42KFjOZsiRUv7u7E2poVGJNmTfS2OU4x4AA@mail.gmail.com/

> Replacing the existing C driver for real HW with Rust new one doesn't
> make sense, right? So a necessary condition of getting Rust bindings
> for a subsystem accepted is that a HW verndor implements both a driver
> and bindings for their new HW?

Not necessarily. It is true that, in general, the kernel does not
want/accept duplicate implementations.

However, this is a bit of a special situation, and there may be some
reasons to allow for it in a given subsystem. For instance:

  - The need to experiment with Rust.

  - To have an actual in-tree user that allows to develop the
abstractions for a subsystem, so that later they are ready to be used
for future, actual new drivers.

  - Pending redesigns: sometimes subsystems may have a
redesign/refactor/experiment that they have wanted to do for a while,
so they may take the chance to also try to write it in Rust anyway. Of
course, that could conflate two experiments, but... :)

  - Security: there may be some modules that have been problematic in
the past (especially if due to memory safety issues / data races), and
the subsystem may be willing to accept a parallel implementation to
see if it would be an improvement thanks to Rust's properties.

Cheers,
Miguel

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Alice Ryhl]

On 6/16/23 15:20, Andrew Lunn wrote:
>> As for this being a single function rather than four functions, that's
>> definitely a debatable decision. You would only do that if it makes sense to
>> merge them together and if you would always assign all of them together. I
>> don't know enough about these fields to say whether it makes sense here.
> 
> It can actually make sense to do them all together, because the source
> of these is likely to be a per CPU data structure protected by a per
> CPU sequence lock. You iterate over all CPUs, doing a transaction,
> taking the sequence lock, copy the values, and then releasing the
> lock. Taking and releases the lock per value is unnecessary expense.

It can probably be split into several methods without introducing a lock 
call for each one, if the API is designed right.

Alice

Re: [PATCH 0/5] Rust abstractions for network device drivers

[FUJITA Tomonori]

Hi,

On Fri, 16 Jun 2023 15:23:01 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Fri, Jun 16, 2023 at 2:18 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> As far as I know, no subsystem has accepted Rust bindings yet.
> 
> For abstractions in general (see my previous reply for "real HW"
> etc.), the KUnit subsystem [1] which is onboard and taking some
> patches through their tree / ownership of the code.
> 
> [1] https://lore.kernel.org/rust-for-linux/CABVgOSnprvxzi-z42KFjOZsiRUv7u7E2poVGJNmTfS2OU4x4AA@mail.gmail.com/
> 
>> Replacing the existing C driver for real HW with Rust new one doesn't
>> make sense, right? So a necessary condition of getting Rust bindings
>> for a subsystem accepted is that a HW verndor implements both a driver
>> and bindings for their new HW?
> 
> Not necessarily. It is true that, in general, the kernel does not
> want/accept duplicate implementations.
> 
> However, this is a bit of a special situation, and there may be some
> reasons to allow for it in a given subsystem. For instance:
> 
>   - The need to experiment with Rust.
> 
>   - To have an actual in-tree user that allows to develop the
> abstractions for a subsystem, so that later they are ready to be used
> for future, actual new drivers.
> 
>   - Pending redesigns: sometimes subsystems may have a
> redesign/refactor/experiment that they have wanted to do for a while,
> so they may take the chance to also try to write it in Rust anyway. Of
> course, that could conflate two experiments, but... :)
> 
>   - Security: there may be some modules that have been problematic in
> the past (especially if due to memory safety issues / data races), and
> the subsystem may be willing to accept a parallel implementation to
> see if it would be an improvement thanks to Rust's properties.

Yeah, I know :) But what I want to know is if there are such reasons
for netdev subsystem, a necessary condition of getting Rust bindings
accepted for the netdev maintainers.

thanks,

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Miguel Ojeda]

On Fri, Jun 16, 2023 at 3:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> I think this is something you need to get addressed at a language
> level very soon. Lots of netdev API calls will be to macros. The API
> to manipulate skbs is pretty much always used on the hot path, so i
> expect that it will have a large number of macros. It is unclear to me
> how well it will scale if you need to warp them all?
>
> ~/linux/include/linux$ grep inline skbuff.h  | wc
>     349    2487   23010
>
> Do you really want to write 300+ wrappers?

It would be very nice if at least `bindgen` (or even the Rust
compiler... :) could cover many of these one-liners. We have discussed
and asked for this in the past, and messages like this reinforce the
need/request for this clearly, so thanks for this.

Since `bindgen` 0.64.0 earlier this year [1] there is an experimental
feature for this (`--wrap-static-fns`), so that is nice -- though we
need to see how well it works. We are upgrading `bindgen` to the
latest version after the merge window, so we can play with this soon.

In particular, given:

    static inline int foo(int a, int b) {
        return a + b;
    }

It generates a C file with e.g.:

    #include "a.h"

    // Static wrappers

    int foo__extern(int a, int b) { return foo(a, b); }

And then in the usual Rust bindings:

    extern "C" {
        #[link_name = "foo__extern"]
        pub fn foo(a: ::std::os::raw::c_int, b: ::std::os::raw::c_int)
-> ::std::os::raw::c_int;
    }

Cheers,
Miguel

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Andrew Lunn]

On Fri, Jun 16, 2023 at 03:48:31PM +0200, Miguel Ojeda wrote:
> On Fri, Jun 16, 2023 at 3:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > I think this is something you need to get addressed at a language
> > level very soon. Lots of netdev API calls will be to macros. The API
> > to manipulate skbs is pretty much always used on the hot path, so i
> > expect that it will have a large number of macros. It is unclear to me
> > how well it will scale if you need to warp them all?
> >
> > ~/linux/include/linux$ grep inline skbuff.h  | wc
> >     349    2487   23010
> >
> > Do you really want to write 300+ wrappers?
> 
> It would be very nice if at least `bindgen` (or even the Rust
> compiler... :) could cover many of these one-liners. We have discussed
> and asked for this in the past, and messages like this reinforce the
> need/request for this clearly, so thanks for this.
> 
> Since `bindgen` 0.64.0 earlier this year [1] there is an experimental
> feature for this (`--wrap-static-fns`), so that is nice -- though we
> need to see how well it works. We are upgrading `bindgen` to the
> latest version after the merge window, so we can play with this soon.
> 
> In particular, given:
> 
>     static inline int foo(int a, int b) {
>         return a + b;
>     }
> 
> It generates a C file with e.g.:
> 
>     #include "a.h"
> 
>     // Static wrappers
> 
>     int foo__extern(int a, int b) { return foo(a, b); }
> 
> And then in the usual Rust bindings:
> 
>     extern "C" {
>         #[link_name = "foo__extern"]
>         pub fn foo(a: ::std::os::raw::c_int, b: ::std::os::raw::c_int)
> -> ::std::os::raw::c_int;
>     }

I said in another email, i don't want to suggest premature
optimisation, before profiling is done. But in C, these functions are
inline for a reason. We don't want the cost of a subroutine call. We
want the compiler to be able to inline the code, and the optimiser to
be able to see it and generate the best code it can.

Can the rust compile inline the binding including the FFI call?

skb are used on the hot path.  For a 10G Ethernet, it is dealing with
nearly 1 million packets per second for big Ethernet frames, or worst
case 14Mpps for small frames. Subroutine calls add up when you only
have around 200 instructions to deal with a Ethernet frame.

     Andrew

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Miguel Ojeda]

On Fri, Jun 16, 2023 at 4:43 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> I said in another email, i don't want to suggest premature
> optimisation, before profiling is done. But in C, these functions are
> inline for a reason. We don't want the cost of a subroutine call. We
> want the compiler to be able to inline the code, and the optimiser to
> be able to see it and generate the best code it can.

See my reply in v2 to your message where I mentioned some of the
options we are considering [1] -- not sure if you saw it:

> Yeah, other use cases will also need that solved, e.g. Andreas for his
> NVMe work.
>
> We discussed reimplementing performance-critical bits in Rust as you
> suggest, as well as cross-language LTO. We also talked about possible
> alternative approaches like "manual local LTO" for the helpers only
> via feeding their LLVM IR to `rustc`, which may recover most of the
> performance without having to go for full LTO and its associated
> kernel link times.

[1] https://lore.kernel.org/rust-for-linux/CANiq72kyUhvmG6KB32X1vuhNzOOJbs7R1JbK+vnPELX4tG73RA@mail.gmail.com/

Cheers,
Miguel

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Andrew Lunn]

On Thu, Jun 15, 2023 at 08:47:43PM -0700, Richard Cochran wrote:
> On Thu, Jun 15, 2023 at 07:02:52PM -0700, Jakub Kicinski wrote:
> 
> > IMO we have too much useless playground stuff in C already, don't get me
> > started. Let's get a real device prove that this thing can actually work
> > :( We can waste years implementing imaginary device drivers that nobody
> > ends up using.
> 
> Yes, the long standing policy seems to be not to merge "dummy"
> drivers.  The original PTP patch set had one, but at the time the
> reviews said firmly "No" to that.

Hi Richard

I think in the modern day, you could probably add code to netdevsim to
aid testing PTP. It should however by accompanied with selftests which
actually make use of what you add.

	Andrew

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Jakub Kicinski]

On Fri, 16 Jun 2023 15:23:01 +0200 Miguel Ojeda wrote:
> Not necessarily. It is true that, in general, the kernel does not
> want/accept duplicate implementations.
> 
> However, this is a bit of a special situation, and there may be some
> reasons to allow for it in a given subsystem. For instance:
> 
>   - The need to experiment with Rust.

Duplicated driver in a new language means nobody has a real incentive
to use it in production. That really mutes the signal we get out of the
experiment. At the same time IIUC building the Rust code is not trivial,
so IDK if we're ready to force people to use it. Ugh.

Do you have any idea how long it will take until one can 
 dnf install $rust
and have that be enough to be build a kernel (for the two major arches)?

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Jakub Kicinski]

On Fri, 16 Jun 2023 15:04:59 +0200 Andrew Lunn wrote:
> > Actually Andrew is interested, and PHY drivers seem relatively simple..
> > /me runs away  
> 
> :-)
> 
> I think because they are so simple, there is not much to gain by
> implementing them in rust.

I see many benefits :) Its a smallish and self-contained piece so it's
achievable. Yet it interacts (to the extent the complexity of the PHY
calls for it) with core netdev structures. Some PHYs even do
timestamping which brings in interactions with skbs etc.

Major benefit number 2 is that Rust is currently missing any real life
bus bindings (AFAIU) and PHYs and the bus they live on are maintained
by the same person 😁️

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Jakub Kicinski]

On Fri, 16 Jun 2023 22:02:20 +0900 (JST) FUJITA Tomonori wrote:
> > The skb freeing looks shady from functional perspective.
> > I'm guessing some form of destructor frees the skb automatically
> > in xmit handler(?), but (a) no reason support, (b) kfree_skb_reason()
> > is most certainly not safe to call on all xmit paths...  
> 
> Yeah, I assume that a driver keeps a skb in private data structure
> (such as tx ring) then removes the skb from it after the completion of
> tx; automatically the drop() method runs (where we need to free the
> skb).
> 
> I thought that calling dev_kfree_skb() is fine but no? We also need
> something different for drivers that use other ways to free the skb
> though.
> 
> I use kfree_skb_reason() because dev_kfree_skb() is a macro so it
> can't be called directly from Rust. But I should have used
> dev_kfree_skb() with a helper function.

skbs (generally) can't be freed in an interrupt context.
dev_kfree_skb_any_reason() is probably the most general implementation.
But then we also have a set of functions used in known contexts for fast
object recycling like napi_consume_skb().
How would complex object destruction rules fit in in the Rust world?

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Alice Ryhl]

On 6/16/23 20:40, Jakub Kicinski wrote:
> On Fri, 16 Jun 2023 22:02:20 +0900 (JST) FUJITA Tomonori wrote:
>>> The skb freeing looks shady from functional perspective.
>>> I'm guessing some form of destructor frees the skb automatically
>>> in xmit handler(?), but (a) no reason support, (b) kfree_skb_reason()
>>> is most certainly not safe to call on all xmit paths...
>>
>> Yeah, I assume that a driver keeps a skb in private data structure
>> (such as tx ring) then removes the skb from it after the completion of
>> tx; automatically the drop() method runs (where we need to free the
>> skb).
>>
>> I thought that calling dev_kfree_skb() is fine but no? We also need
>> something different for drivers that use other ways to free the skb
>> though.
>>
>> I use kfree_skb_reason() because dev_kfree_skb() is a macro so it
>> can't be called directly from Rust. But I should have used
>> dev_kfree_skb() with a helper function.
> 
> skbs (generally) can't be freed in an interrupt context.
> dev_kfree_skb_any_reason() is probably the most general implementation.
> But then we also have a set of functions used in known contexts for fast
> object recycling like napi_consume_skb().
> How would complex object destruction rules fit in in the Rust world?

A Rust method can be defined to take the struct "by value", which 
consumes the struct and prevents you from using it again. This can let 
you provide many different cleanup methods that each clean it up in 
different ways.

However, you cannot force the user to use one of those methods. They 
always have the option of letting the value go out of scope, which calls 
the destructor. And they can do this at any time.

That said, the destructor of the value does not necessarily *have* to 
translate to immediately freeing the value. If the value if refcounted, 
the destructor could just drop the refcount. It would also be possible 
for a destructor to schedule the cleanup operation to a workqueue. Or 
you could do something more clever.

Alice

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Jakub Kicinski]

On Fri, 16 Jun 2023 21:00:36 +0200 Alice Ryhl wrote:
> A Rust method can be defined to take the struct "by value", which 
> consumes the struct and prevents you from using it again. This can let 
> you provide many different cleanup methods that each clean it up in 
> different ways.
> 
> However, you cannot force the user to use one of those methods. They 
> always have the option of letting the value go out of scope, which calls 
> the destructor. And they can do this at any time.
> 
> That said, the destructor of the value does not necessarily *have* to 
> translate to immediately freeing the value. If the value if refcounted, 
> the destructor could just drop the refcount. It would also be possible 
> for a destructor to schedule the cleanup operation to a workqueue. Or 
> you could do something more clever.

Can we put a WARN_ON() in the destructor and expect object to never be
implicitly freed?  skbs represent packets (most of the time) and for
tracking which part of the stack is dropping packets we try to provide
a drop reason along the freed skb. It'd be great if for Rust we could
from the get-go direct everyone towards the APIs with an explicit reason
code.

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Alice Ryhl]

On 6/16/23 21:10, Jakub Kicinski wrote:
> On Fri, 16 Jun 2023 21:00:36 +0200 Alice Ryhl wrote:
>> A Rust method can be defined to take the struct "by value", which
>> consumes the struct and prevents you from using it again. This can let
>> you provide many different cleanup methods that each clean it up in
>> different ways.
>>
>> However, you cannot force the user to use one of those methods. They
>> always have the option of letting the value go out of scope, which calls
>> the destructor. And they can do this at any time.
>>
>> That said, the destructor of the value does not necessarily *have* to
>> translate to immediately freeing the value. If the value if refcounted,
>> the destructor could just drop the refcount. It would also be possible
>> for a destructor to schedule the cleanup operation to a workqueue. Or
>> you could do something more clever.
> 
> Can we put a WARN_ON() in the destructor and expect object to never be
> implicitly freed?  skbs represent packets (most of the time) and for
> tracking which part of the stack is dropping packets we try to provide
> a drop reason along the freed skb. It'd be great if for Rust we could
> from the get-go direct everyone towards the APIs with an explicit reason
> code.

Yes, you can certainly put a WARN_ON in the destructor.

Another possibility is to use a scope to clean up. I don't know anything 
about these skb objects are used, but you could have the user define a 
"process this socket" function that you pass a pointer to the skb, then 
make the return value be something that explains what should be done 
with the packet. Since you must return a value of the right type, this 
forces you to choose.

Of course, this requires that the processing of packets can be expressed 
as a function call, where it only inspects the packet for the duration 
of that function call. (Lifetimes can ensure that the skb pointer does 
not escape the function.)

Would something like that work?

Alice

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Andrew Lunn]

> Yes, you can certainly put a WARN_ON in the destructor.
> 
> Another possibility is to use a scope to clean up. I don't know anything
> about these skb objects are used, but you could have the user define a
> "process this socket" function that you pass a pointer to the skb, then make
> the return value be something that explains what should be done with the
> packet. Since you must return a value of the right type, this forces you to
> choose.
> 
> Of course, this requires that the processing of packets can be expressed as
> a function call, where it only inspects the packet for the duration of that
> function call. (Lifetimes can ensure that the skb pointer does not escape
> the function.)
> 
> Would something like that work?

I don't think so, at least not in the contest of an Rust Ethernet
driver.

There are two main flows.

A packet is received. An skb is allocated and the received packet is
placed into the skb. The Ethernet driver then hands the packet over to
the network stack. The network stack is free to do whatever it wants
with the packet. Things can go wrong within the driver, so at times it
needs to free the skb rather than pass it to the network stack, which
would be a drop.

The second flow is that the network stack has a packet it wants sent
out an Ethernet port, in the form of an skb. The skb gets passed to
the Ethernet driver. The driver will do whatever it needs to do to
pass the contents of the skb to the hardware. Once the hardware has
it, the driver frees the skb. Again, things can go wrong and it needs
to free the skb without sending it, which is a drop.

So the lifetime is not a simple function call.

The drop reason indicates why the packet was dropped. It should give
some indication of what problem occurred which caused the drop. So
ideally we don't want an anonymous drop. The C code does not enforce
that, but it would be nice if the rust wrapper to dispose of an skb
did enforce it.

I would also say that this dummy driver and the C dummy driver is
actually wrong in 'dropping' the frame. Its whole purpose in life is to
be a black hole. It should only drop the packet if for some reason it
cannot throw the packet into the black hole.

       Andrew

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Miguel Ojeda]

On Fri, Jun 16, 2023 at 8:26 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Duplicated driver in a new language means nobody has a real incentive
> to use it in production. That really mutes the signal we get out of the
> experiment.

What I was trying to say is that there may be other incentives for
using the new one, like the ones I gave in the list.

Even if there is no incentive for using it right now, one may still
want to build/design it in order to evaluate Rust and/or prepare
abstractions for future drivers. This was the reason for the NVMe
driver request we got back then, for instance.

> At the same time IIUC building the Rust code is not trivial,
> so IDK if we're ready to force people to use it. Ugh.

I am not sure what you mean -- building works the same way as you have
done now, i.e. your usual build commands with `make`.

If you mean installing the toolchain, then we have a Quick Start guide
[1], and a script that checks for the requirements.

[1] https://docs.kernel.org/rust/quick-start.html

> Do you have any idea how long it will take until one can
>  dnf install $rust
> and have that be enough to be build a kernel (for the two major arches)?

You may do it today if the version matches, which for Fedora it should
e.g. next week or so when we merge the upgrade to 1.70, since Fedora
has now 1.70 [2].

But since they may not always align, I would recommend using `rustup`
(or the standalone installers) until we can establish a minimum
version. Please see the Quick Start guide for details [1].

Other distributions may provide a custom package with the required
dependencies for building Rust kernel code, like Ubuntu does.

[2] https://packages.fedoraproject.org/pkgs/rust/rust/

Cheers,
Miguel

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Alice Ryhl]

On 6/16/23 22:04, Andrew Lunn wrote:
>> Yes, you can certainly put a WARN_ON in the destructor.
>>
>> Another possibility is to use a scope to clean up. I don't know anything
>> about these skb objects are used, but you could have the user define a
>> "process this socket" function that you pass a pointer to the skb, then make
>> the return value be something that explains what should be done with the
>> packet. Since you must return a value of the right type, this forces you to
>> choose.
>>
>> Of course, this requires that the processing of packets can be expressed as
>> a function call, where it only inspects the packet for the duration of that
>> function call. (Lifetimes can ensure that the skb pointer does not escape
>> the function.)
>>
>> Would something like that work?
> 
> I don't think so, at least not in the contest of an Rust Ethernet
> driver.
> 
> There are two main flows.
> 
> A packet is received. An skb is allocated and the received packet is
> placed into the skb. The Ethernet driver then hands the packet over to
> the network stack. The network stack is free to do whatever it wants
> with the packet. Things can go wrong within the driver, so at times it
> needs to free the skb rather than pass it to the network stack, which
> would be a drop.
> 
> The second flow is that the network stack has a packet it wants sent
> out an Ethernet port, in the form of an skb. The skb gets passed to
> the Ethernet driver. The driver will do whatever it needs to do to
> pass the contents of the skb to the hardware. Once the hardware has
> it, the driver frees the skb. Again, things can go wrong and it needs
> to free the skb without sending it, which is a drop.
> 
> So the lifetime is not a simple function call.
> 
> The drop reason indicates why the packet was dropped. It should give
> some indication of what problem occurred which caused the drop. So
> ideally we don't want an anonymous drop. The C code does not enforce
> that, but it would be nice if the rust wrapper to dispose of an skb
> did enforce it.

It sounds like a destructor with WARN_ON is the best approach right now.

Unfortunately, I don't think we can enforce that the destructor is not 
used today. That said, in the future it may be possible to implement a 
linter that detects it - I know that there have already been experiments 
with other custom lints for the kernel (e.g., enforcing that you don't 
sleep while holding a spinlock).

> I would also say that this dummy driver and the C dummy driver is
> actually wrong in 'dropping' the frame. Its whole purpose in life is to
> be a black hole. It should only drop the packet if for some reason it
> cannot throw the packet into the black hole.

Ah, I suppose that we would also need a "by value" cleanup method for 
that case.

Alice

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Greg KH]

On Sat, Jun 17, 2023 at 12:08:26PM +0200, Alice Ryhl wrote:
> On 6/16/23 22:04, Andrew Lunn wrote:
> > > Yes, you can certainly put a WARN_ON in the destructor.
> > > 
> > > Another possibility is to use a scope to clean up. I don't know anything
> > > about these skb objects are used, but you could have the user define a
> > > "process this socket" function that you pass a pointer to the skb, then make
> > > the return value be something that explains what should be done with the
> > > packet. Since you must return a value of the right type, this forces you to
> > > choose.
> > > 
> > > Of course, this requires that the processing of packets can be expressed as
> > > a function call, where it only inspects the packet for the duration of that
> > > function call. (Lifetimes can ensure that the skb pointer does not escape
> > > the function.)
> > > 
> > > Would something like that work?
> > 
> > I don't think so, at least not in the contest of an Rust Ethernet
> > driver.
> > 
> > There are two main flows.
> > 
> > A packet is received. An skb is allocated and the received packet is
> > placed into the skb. The Ethernet driver then hands the packet over to
> > the network stack. The network stack is free to do whatever it wants
> > with the packet. Things can go wrong within the driver, so at times it
> > needs to free the skb rather than pass it to the network stack, which
> > would be a drop.
> > 
> > The second flow is that the network stack has a packet it wants sent
> > out an Ethernet port, in the form of an skb. The skb gets passed to
> > the Ethernet driver. The driver will do whatever it needs to do to
> > pass the contents of the skb to the hardware. Once the hardware has
> > it, the driver frees the skb. Again, things can go wrong and it needs
> > to free the skb without sending it, which is a drop.
> > 
> > So the lifetime is not a simple function call.
> > 
> > The drop reason indicates why the packet was dropped. It should give
> > some indication of what problem occurred which caused the drop. So
> > ideally we don't want an anonymous drop. The C code does not enforce
> > that, but it would be nice if the rust wrapper to dispose of an skb
> > did enforce it.
> 
> It sounds like a destructor with WARN_ON is the best approach right now.

Note, if that WARN_ON fires, you just rebooted the machine as the huge
majority of all Linux systems in the world run with "panic on warn"
enabled :(

So almost NEVER do a WARN* call if at all possible, unless there is no
way back, and you can not recover from the mess, and crashing the system
is your only recourse (instead of locking it up.)

So please, don't do that, recover and report an error to the caller if
possible and let them handle it properly.  Yes, in a descructor that's
almost impossible, but then perhaps if this is a real problem, something
needs to be changed somewhere else in the callpath.

thanks,

greg k-h

Re: [PATCH 0/5] Rust abstractions for network device drivers

[FUJITA Tomonori]

Hi,

On Sat, 17 Jun 2023 12:08:26 +0200
Alice Ryhl <alice@ryhl.io> wrote:

> On 6/16/23 22:04, Andrew Lunn wrote:
>>> Yes, you can certainly put a WARN_ON in the destructor.
>>>
>>> Another possibility is to use a scope to clean up. I don't know
>>> anything
>>> about these skb objects are used, but you could have the user define a
>>> "process this socket" function that you pass a pointer to the skb,
>>> then make
>>> the return value be something that explains what should be done with
>>> the
>>> packet. Since you must return a value of the right type, this forces
>>> you to
>>> choose.
>>>
>>> Of course, this requires that the processing of packets can be
>>> expressed as
>>> a function call, where it only inspects the packet for the duration of
>>> that
>>> function call. (Lifetimes can ensure that the skb pointer does not
>>> escape
>>> the function.)
>>>
>>> Would something like that work?
>> I don't think so, at least not in the contest of an Rust Ethernet
>> driver.
>> There are two main flows.
>> A packet is received. An skb is allocated and the received packet is
>> placed into the skb. The Ethernet driver then hands the packet over to
>> the network stack. The network stack is free to do whatever it wants
>> with the packet. Things can go wrong within the driver, so at times it
>> needs to free the skb rather than pass it to the network stack, which
>> would be a drop.
>> The second flow is that the network stack has a packet it wants sent
>> out an Ethernet port, in the form of an skb. The skb gets passed to
>> the Ethernet driver. The driver will do whatever it needs to do to
>> pass the contents of the skb to the hardware. Once the hardware has
>> it, the driver frees the skb. Again, things can go wrong and it needs
>> to free the skb without sending it, which is a drop.
>> So the lifetime is not a simple function call.
>> The drop reason indicates why the packet was dropped. It should give
>> some indication of what problem occurred which caused the drop. So
>> ideally we don't want an anonymous drop. The C code does not enforce
>> that, but it would be nice if the rust wrapper to dispose of an skb
>> did enforce it.
> 
> It sounds like a destructor with WARN_ON is the best approach right
> now.

Better to simply BUG()? We want to make sure that a device driver
explicity calls a function that consumes a skb object (on tx path,
e.g., napi_consume_skb()). If a device driver doesn't call such, it's
a bug that should be found easily and fixed during the development. It
would be even better if the compiler could find such though.

If Rust bindings for netdev could help device developpers in such way,
it's worth an experiments? because looks like netdev subsystem accepts
more drivers for new hardware than other subsystems.

thanks,

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Greg KH]

On Mon, Jun 19, 2023 at 05:50:03PM +0900, FUJITA Tomonori wrote:
> Hi,
> 
> On Sat, 17 Jun 2023 12:08:26 +0200
> Alice Ryhl <alice@ryhl.io> wrote:
> 
> > On 6/16/23 22:04, Andrew Lunn wrote:
> >>> Yes, you can certainly put a WARN_ON in the destructor.
> >>>
> >>> Another possibility is to use a scope to clean up. I don't know
> >>> anything
> >>> about these skb objects are used, but you could have the user define a
> >>> "process this socket" function that you pass a pointer to the skb,
> >>> then make
> >>> the return value be something that explains what should be done with
> >>> the
> >>> packet. Since you must return a value of the right type, this forces
> >>> you to
> >>> choose.
> >>>
> >>> Of course, this requires that the processing of packets can be
> >>> expressed as
> >>> a function call, where it only inspects the packet for the duration of
> >>> that
> >>> function call. (Lifetimes can ensure that the skb pointer does not
> >>> escape
> >>> the function.)
> >>>
> >>> Would something like that work?
> >> I don't think so, at least not in the contest of an Rust Ethernet
> >> driver.
> >> There are two main flows.
> >> A packet is received. An skb is allocated and the received packet is
> >> placed into the skb. The Ethernet driver then hands the packet over to
> >> the network stack. The network stack is free to do whatever it wants
> >> with the packet. Things can go wrong within the driver, so at times it
> >> needs to free the skb rather than pass it to the network stack, which
> >> would be a drop.
> >> The second flow is that the network stack has a packet it wants sent
> >> out an Ethernet port, in the form of an skb. The skb gets passed to
> >> the Ethernet driver. The driver will do whatever it needs to do to
> >> pass the contents of the skb to the hardware. Once the hardware has
> >> it, the driver frees the skb. Again, things can go wrong and it needs
> >> to free the skb without sending it, which is a drop.
> >> So the lifetime is not a simple function call.
> >> The drop reason indicates why the packet was dropped. It should give
> >> some indication of what problem occurred which caused the drop. So
> >> ideally we don't want an anonymous drop. The C code does not enforce
> >> that, but it would be nice if the rust wrapper to dispose of an skb
> >> did enforce it.
> > 
> > It sounds like a destructor with WARN_ON is the best approach right
> > now.
> 
> Better to simply BUG()? We want to make sure that a device driver
> explicity calls a function that consumes a skb object (on tx path,
> e.g., napi_consume_skb()). If a device driver doesn't call such, it's
> a bug that should be found easily and fixed during the development. It
> would be even better if the compiler could find such though.

No, BUG() means "I have given up all hope here because the hardware is
broken and beyond repair so the machine will now crash and take all of
your data with it because I don't know how to properly recover".  That
should NEVER happen in a device driver, as that's very presumptious of
it, and means the driver itself is broken.

Report the error back up the chain and handle it properly, that's the
correct thing to do.

> If Rust bindings for netdev could help device developpers in such way,
> it's worth an experiments? because looks like netdev subsystem accepts
> more drivers for new hardware than other subsystems.

Have you looked at the IIO subsystem?  :)

thanks,

greg k-h

Re: [PATCH 0/5] Rust abstractions for network device drivers

[FUJITA Tomonori]

Hi,

On Mon, 19 Jun 2023 11:46:38 +0200
Greg KH <greg@kroah.com> wrote:

> On Mon, Jun 19, 2023 at 05:50:03PM +0900, FUJITA Tomonori wrote:
>> Hi,
>> 
>> On Sat, 17 Jun 2023 12:08:26 +0200
>> Alice Ryhl <alice@ryhl.io> wrote:
>> 
>> > On 6/16/23 22:04, Andrew Lunn wrote:
>> >>> Yes, you can certainly put a WARN_ON in the destructor.
>> >>>
>> >>> Another possibility is to use a scope to clean up. I don't know
>> >>> anything
>> >>> about these skb objects are used, but you could have the user define a
>> >>> "process this socket" function that you pass a pointer to the skb,
>> >>> then make
>> >>> the return value be something that explains what should be done with
>> >>> the
>> >>> packet. Since you must return a value of the right type, this forces
>> >>> you to
>> >>> choose.
>> >>>
>> >>> Of course, this requires that the processing of packets can be
>> >>> expressed as
>> >>> a function call, where it only inspects the packet for the duration of
>> >>> that
>> >>> function call. (Lifetimes can ensure that the skb pointer does not
>> >>> escape
>> >>> the function.)
>> >>>
>> >>> Would something like that work?
>> >> I don't think so, at least not in the contest of an Rust Ethernet
>> >> driver.
>> >> There are two main flows.
>> >> A packet is received. An skb is allocated and the received packet is
>> >> placed into the skb. The Ethernet driver then hands the packet over to
>> >> the network stack. The network stack is free to do whatever it wants
>> >> with the packet. Things can go wrong within the driver, so at times it
>> >> needs to free the skb rather than pass it to the network stack, which
>> >> would be a drop.
>> >> The second flow is that the network stack has a packet it wants sent
>> >> out an Ethernet port, in the form of an skb. The skb gets passed to
>> >> the Ethernet driver. The driver will do whatever it needs to do to
>> >> pass the contents of the skb to the hardware. Once the hardware has
>> >> it, the driver frees the skb. Again, things can go wrong and it needs
>> >> to free the skb without sending it, which is a drop.
>> >> So the lifetime is not a simple function call.
>> >> The drop reason indicates why the packet was dropped. It should give
>> >> some indication of what problem occurred which caused the drop. So
>> >> ideally we don't want an anonymous drop. The C code does not enforce
>> >> that, but it would be nice if the rust wrapper to dispose of an skb
>> >> did enforce it.
>> > 
>> > It sounds like a destructor with WARN_ON is the best approach right
>> > now.
>> 
>> Better to simply BUG()? We want to make sure that a device driver
>> explicity calls a function that consumes a skb object (on tx path,
>> e.g., napi_consume_skb()). If a device driver doesn't call such, it's
>> a bug that should be found easily and fixed during the development. It
>> would be even better if the compiler could find such though.
> 
> No, BUG() means "I have given up all hope here because the hardware is
> broken and beyond repair so the machine will now crash and take all of
> your data with it because I don't know how to properly recover".  That
> should NEVER happen in a device driver, as that's very presumptious of
> it, and means the driver itself is broken.
> 
> Report the error back up the chain and handle it properly, that's the
> correct thing to do.

I see. Then netdev_warn() should be used instead.

Is it possible to handle the case where a device driver wrongly
doesn't consume a skb object?


>> If Rust bindings for netdev could help device developpers in such way,
>> it's worth an experiments? because looks like netdev subsystem accepts
>> more drivers for new hardware than other subsystems.
> 
> Have you looked at the IIO subsystem?  :)

No, I've not. Are there possible drivers that Rust could be useful
for?

thanks,

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Greg KH]

On Mon, Jun 19, 2023 at 08:05:59PM +0900, FUJITA Tomonori wrote:
> On Mon, 19 Jun 2023 11:46:38 +0200
> Greg KH <greg@kroah.com> wrote:
> 
> > On Mon, Jun 19, 2023 at 05:50:03PM +0900, FUJITA Tomonori wrote:
> >> Hi,
> >> 
> >> On Sat, 17 Jun 2023 12:08:26 +0200
> >> Alice Ryhl <alice@ryhl.io> wrote:
> >> 
> >> > On 6/16/23 22:04, Andrew Lunn wrote:
> >> >>> Yes, you can certainly put a WARN_ON in the destructor.
> >> >>>
> >> >>> Another possibility is to use a scope to clean up. I don't know
> >> >>> anything
> >> >>> about these skb objects are used, but you could have the user define a
> >> >>> "process this socket" function that you pass a pointer to the skb,
> >> >>> then make
> >> >>> the return value be something that explains what should be done with
> >> >>> the
> >> >>> packet. Since you must return a value of the right type, this forces
> >> >>> you to
> >> >>> choose.
> >> >>>
> >> >>> Of course, this requires that the processing of packets can be
> >> >>> expressed as
> >> >>> a function call, where it only inspects the packet for the duration of
> >> >>> that
> >> >>> function call. (Lifetimes can ensure that the skb pointer does not
> >> >>> escape
> >> >>> the function.)
> >> >>>
> >> >>> Would something like that work?
> >> >> I don't think so, at least not in the contest of an Rust Ethernet
> >> >> driver.
> >> >> There are two main flows.
> >> >> A packet is received. An skb is allocated and the received packet is
> >> >> placed into the skb. The Ethernet driver then hands the packet over to
> >> >> the network stack. The network stack is free to do whatever it wants
> >> >> with the packet. Things can go wrong within the driver, so at times it
> >> >> needs to free the skb rather than pass it to the network stack, which
> >> >> would be a drop.
> >> >> The second flow is that the network stack has a packet it wants sent
> >> >> out an Ethernet port, in the form of an skb. The skb gets passed to
> >> >> the Ethernet driver. The driver will do whatever it needs to do to
> >> >> pass the contents of the skb to the hardware. Once the hardware has
> >> >> it, the driver frees the skb. Again, things can go wrong and it needs
> >> >> to free the skb without sending it, which is a drop.
> >> >> So the lifetime is not a simple function call.
> >> >> The drop reason indicates why the packet was dropped. It should give
> >> >> some indication of what problem occurred which caused the drop. So
> >> >> ideally we don't want an anonymous drop. The C code does not enforce
> >> >> that, but it would be nice if the rust wrapper to dispose of an skb
> >> >> did enforce it.
> >> > 
> >> > It sounds like a destructor with WARN_ON is the best approach right
> >> > now.
> >> 
> >> Better to simply BUG()? We want to make sure that a device driver
> >> explicity calls a function that consumes a skb object (on tx path,
> >> e.g., napi_consume_skb()). If a device driver doesn't call such, it's
> >> a bug that should be found easily and fixed during the development. It
> >> would be even better if the compiler could find such though.
> > 
> > No, BUG() means "I have given up all hope here because the hardware is
> > broken and beyond repair so the machine will now crash and take all of
> > your data with it because I don't know how to properly recover".  That
> > should NEVER happen in a device driver, as that's very presumptious of
> > it, and means the driver itself is broken.
> > 
> > Report the error back up the chain and handle it properly, that's the
> > correct thing to do.
> 
> I see. Then netdev_warn() should be used instead.

Yes.

> Is it possible to handle the case where a device driver wrongly
> doesn't consume a skb object?

I have no idea as I do not know the network driver stack, sorry.

> >> If Rust bindings for netdev could help device developpers in such way,
> >> it's worth an experiments? because looks like netdev subsystem accepts
> >> more drivers for new hardware than other subsystems.
> > 
> > Have you looked at the IIO subsystem?  :)
> 
> No, I've not. Are there possible drivers that Rust could be useful
> for?

Who knows, you said you were looking for subsystems with lots and lots
of new drivers, and the IIO subsystem has over a thousand of them, all
just added in the past few years.

good luck!

greg k-h

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Emilio Cobos Álvarez]

Hi Andrew, Miguel,

On 6/16/23 16:43, Andrew Lunn wrote:
> I said in another email, i don't want to suggest premature
> optimisation, before profiling is done. But in C, these functions are
> inline for a reason. We don't want the cost of a subroutine call. We
> want the compiler to be able to inline the code, and the optimiser to
> be able to see it and generate the best code it can.
> 
> Can the rust compile inline the binding including the FFI call?

This is possible, with cross-language LTO, see:

   https://blog.llvm.org/2019/09/closing-gap-cross-language-lto-between.html

There are some requirements that need to happen for that to work 
(mainly, I believe, that the LLVM version used by rustc and clang agree).

But in general it is possible. We use it extensively on Firefox. Of 
course the requirements of Firefox and the kernel might be different.

I think we rely heavily on PGO instrumentation to make the linker inline 
ffi functions, but there might be other ways of forcing the linker to 
inline particular calls that bindgen could generate or what not.

Cheers,

  -- Emilio

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Andrew Lunn]

> > Report the error back up the chain and handle it properly, that's the
> > correct thing to do.
> 
> I see. Then netdev_warn() should be used instead.
> 
> Is it possible to handle the case where a device driver wrongly
> doesn't consume a skb object?

The skb is then likely leaked. After a while, you will consume all the
memory, the OOM handler will start killing processes, until eventually
the machine dies.

This is unlikely to happen on the main path, it would be a rather dumb
developer who messed up something so obvious. Leaks are more likely to
happen in the error paths, and that tends to happen less often, and so
the machine is going to live for longer.

But we are talking about different things here. Jakub wanted to ensure
that when the driver does drop an skb, it includes a reason code
indicating why it dropped it. If the driver tries to drop the frame
without providing a reason, we want to know the code path, so it can
be fixed. Even better if this can be done at compile time.

Not releasing the frame at all is a different problem, and probably
not easy to fix. There is some degree of handover of ownership of the
skb. When asked to transmit it, the driver should eventually release
the skb. However, that is often sometime in the future after the
hardware has confirmed it has DMAed a copy of the frame into its own
memory. On the receive side, in the normal path the driver could
allocate an skb, setup the DMA to copy the frame into it, and then
wait for an indication the DMA is complete. Then it passes it to the
network stack, at which point the network stack becomes the owner.

But there are no simple scope rules to detect an skb has been leaked.

    Andrew

RE: [PATCH 0/5] Rust abstractions for network device drivers

[David Laight]

....
> Not releasing the frame at all is a different problem, and probably
> not easy to fix. There is some degree of handover of ownership of the
> skb. When asked to transmit it, the driver should eventually release
> the skb. However, that is often sometime in the future after the
> hardware has confirmed it has DMAed a copy of the frame into its own
> memory. On the receive side, in the normal path the driver could
> allocate an skb, setup the DMA to copy the frame into it, and then
> wait for an indication the DMA is complete. Then it passes it to the
> network stack, at which point the network stack becomes the owner.
> 
> But there are no simple scope rules to detect an skb has been leaked.

You can require/enforce that the pointer the driver has is set to
NULL when the skb is successfully passed on.
But that tends to require passing the pointer by reference.
Ok for long-lived items but a likely performance hit for skb.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Jakub Kicinski]

On Sat, 17 Jun 2023 12:08:26 +0200 Alice Ryhl wrote:
> > The drop reason indicates why the packet was dropped. It should give
> > some indication of what problem occurred which caused the drop. So
> > ideally we don't want an anonymous drop. The C code does not enforce
> > that, but it would be nice if the rust wrapper to dispose of an skb
> > did enforce it.  
> 
> It sounds like a destructor with WARN_ON is the best approach right now.
> 
> Unfortunately, I don't think we can enforce that the destructor is not 
> used today. That said, in the future it may be possible to implement a 
> linter that detects it - I know that there have already been experiments 
> with other custom lints for the kernel (e.g., enforcing that you don't 
> sleep while holding a spinlock).

Can we do something to hide the destructor from the linker?

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Alice Ryhl]

On 6/20/23 17:47, Jakub Kicinski wrote:
> On Sat, 17 Jun 2023 12:08:26 +0200 Alice Ryhl wrote:
>>> The drop reason indicates why the packet was dropped. It should give
>>> some indication of what problem occurred which caused the drop. So
>>> ideally we don't want an anonymous drop. The C code does not enforce
>>> that, but it would be nice if the rust wrapper to dispose of an skb
>>> did enforce it.
>>
>> It sounds like a destructor with WARN_ON is the best approach right now.
>>
>> Unfortunately, I don't think we can enforce that the destructor is not
>> used today. That said, in the future it may be possible to implement a
>> linter that detects it - I know that there have already been experiments
>> with other custom lints for the kernel (e.g., enforcing that you don't
>> sleep while holding a spinlock).
> 
> Can we do something to hide the destructor from the linker?

We could probably have the destructor call some method that the linker 
wont be able to find, then mark the destructor with #[inline] so that 
dead-code elimination removes it if unused.

(At least, in godbolt the inline marker was necessary for the destructor 
to get removed when not used.)

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Miguel Ojeda]

On Tue, Jun 20, 2023 at 6:55 PM Alice Ryhl <alice@ryhl.io> wrote:
>
> We could probably have the destructor call some method that the linker
> wont be able to find, then mark the destructor with #[inline] so that
> dead-code elimination removes it if unused.
>
> (At least, in godbolt the inline marker was necessary for the destructor
> to get removed when not used.)

Yeah, and we could use `build_assert!(false);` to ensure we don't ever
call it (by users, or by the the custom destructor methods) -- it
seems to work [1], but I am Cc'ing Gary in case he has some concerns
(IIRC we discussed this in the past; I may be forgetting an issue with
this).

Cheers,
Miguel

[1]

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index c20b37e88ab2..7c313c75ff14 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -261,3 +261,28 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
     // instead of `!`. See
<https://github.com/rust-lang/rust-bindgen/issues/2094>.
     loop {}
 }
+
+/// Custom destructor example.
+pub struct S;
+
+impl S {
+    /// Custom destructor 1.
+    pub fn close1(self) {
+        pr_info!("close1");
+        core::mem::forget(self); // If commented out, it is a build error.
+    }
+
+    /// Custom destructor 2.
+    pub fn close2(self) {
+        pr_info!("close2");
+        core::mem::forget(self); // If commented out, it is a build error.
+    }
+}
+
+impl Drop for S {
+    #[inline(always)]
+    fn drop(&mut self) {
+        // This should never be called.
+        build_assert!(false);
+    }
+}
diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs
index 54de32b78cec..f07064ff6341 100644
--- a/samples/rust/rust_minimal.rs
+++ b/samples/rust/rust_minimal.rs
@@ -3,6 +3,7 @@
 //! Rust minimal sample.

 use kernel::prelude::*;
+use kernel::S;

 module! {
     type: RustMinimal,
@@ -26,6 +27,12 @@ impl kernel::Module for RustMinimal {
         numbers.try_push(108)?;
         numbers.try_push(200)?;

+        let _s = S;
+        _s.close1(); // If commented out, it is a build error.
+
+        let _s = S;
+        _s.close2(); // If commented out, it is a build error.
+
         Ok(RustMinimal { numbers })
     }
 }

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Miguel Ojeda]

On Tue, Jun 20, 2023 at 7:44 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Yeah, and we could use `build_assert!(false);` to ensure we don't ever
> call it (by users, or by the the custom destructor methods) -- it

i.e. `build_assert!` is a similar trick we have to produce a linker
error like Alice suggested. It was "nicely packaged" by Gary a while
ago :)

Actually, it should be `build_error!("Normal destructor should never
be called");` -- same thing (I just forgot we had that one).

Cheers,
Miguel

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Miguel Ojeda]

On Mon, Jun 19, 2023 at 1:27 PM Emilio Cobos Álvarez <emilio@crisal.io> wrote:
>
> Hi Andrew, Miguel,
>
> On 6/16/23 16:43, Andrew Lunn wrote:
> > I said in another email, i don't want to suggest premature
> > optimisation, before profiling is done. But in C, these functions are
> > inline for a reason. We don't want the cost of a subroutine call. We
> > want the compiler to be able to inline the code, and the optimiser to
> > be able to see it and generate the best code it can.
> >
> > Can the rust compile inline the binding including the FFI call?
>
> This is possible, with cross-language LTO, see:
>
>    https://blog.llvm.org/2019/09/closing-gap-cross-language-lto-between.html
>
> There are some requirements that need to happen for that to work
> (mainly, I believe, that the LLVM version used by rustc and clang agree).
>
> But in general it is possible. We use it extensively on Firefox. Of
> course the requirements of Firefox and the kernel might be different.
>
> I think we rely heavily on PGO instrumentation to make the linker inline
> ffi functions, but there might be other ways of forcing the linker to
> inline particular calls that bindgen could generate or what not.

Thanks Emilio! It is nice to hear cross-language LTO is working well
for Firefox.

Andreas took a look at cross-language LTO some weeks ago, if I
remember correctly (Cc'd).

I am not sure about the latest status on kernel PGO, though.

Cheers,
Miguel

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Andreas Hindborg (Samsung)]

Hi All,

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:

> On Mon, Jun 19, 2023 at 1:27 PM Emilio Cobos Álvarez <emilio@crisal.io> wrote:
>>
>> Hi Andrew, Miguel,
>>
>> On 6/16/23 16:43, Andrew Lunn wrote:
>> > I said in another email, i don't want to suggest premature
>> > optimisation, before profiling is done. But in C, these functions are
>> > inline for a reason. We don't want the cost of a subroutine call. We
>> > want the compiler to be able to inline the code, and the optimiser to
>> > be able to see it and generate the best code it can.
>> >
>> > Can the rust compile inline the binding including the FFI call?
>>
>> This is possible, with cross-language LTO, see:
>>
>>    https://blog.llvm.org/2019/09/closing-gap-cross-language-lto-between.html
>>
>> There are some requirements that need to happen for that to work
>> (mainly, I believe, that the LLVM version used by rustc and clang agree).
>>
>> But in general it is possible. We use it extensively on Firefox. Of
>> course the requirements of Firefox and the kernel might be different.
>>
>> I think we rely heavily on PGO instrumentation to make the linker inline
>> ffi functions, but there might be other ways of forcing the linker to
>> inline particular calls that bindgen could generate or what not.
>
> Thanks Emilio! It is nice to hear cross-language LTO is working well
> for Firefox.
>
> Andreas took a look at cross-language LTO some weeks ago, if I
> remember correctly (Cc'd).
>
> I am not sure about the latest status on kernel PGO, though.

I hacked it to work a while back for the NVMe and null_blk drivers. You
need to build the C and Rust parts of the kernel with compatible
clang/rustc compilers (same major version I believe), and then pass the
right compiler flags to get rustc and clang to emit llvm bitcode instead
of ELF files with machine code. As far as I recall, some infrastructure
is present in kbuild, but I had to add some bits to make it build.

Also, I had some issues with LLVM not doing the inline properly and I
had to use `llvm-link` on the bitcode of my module to link in the
bitcode of the C code I wanted to inline. Without that step, inlining
did not happen.

I was able to build and execute in qemu like this, but I was not able to
boot on bare metal. I think the LTO breaks something in C land. I did
not investigate that further.

Eventually I paused the work because I did not observe conclusive
speedups in my benchmarks. I plan to resume the work at some point and
do more rigorous benchmarking to see if I can observe a statistically
significant speedup. Not sure when that will happen though.

Best regards,
Andreas

Re: [PATCH 0/5] Rust abstractions for network device drivers

[Andreas Hindborg (Samsung)]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:

> On Fri, Jun 16, 2023 at 3:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> I think this is something you need to get addressed at a language
>> level very soon. Lots of netdev API calls will be to macros. The API
>> to manipulate skbs is pretty much always used on the hot path, so i
>> expect that it will have a large number of macros. It is unclear to me
>> how well it will scale if you need to warp them all?
>>
>> ~/linux/include/linux$ grep inline skbuff.h  | wc
>>     349    2487   23010
>>
>> Do you really want to write 300+ wrappers?
>
> It would be very nice if at least `bindgen` (or even the Rust
> compiler... :) could cover many of these one-liners. We have discussed
> and asked for this in the past, and messages like this reinforce the
> need/request for this clearly, so thanks for this.
>
> Since `bindgen` 0.64.0 earlier this year [1] there is an experimental
> feature for this (`--wrap-static-fns`), so that is nice -- though we
> need to see how well it works. We are upgrading `bindgen` to the
> latest version after the merge window, so we can play with this soon.
>
> In particular, given:
>
>     static inline int foo(int a, int b) {
>         return a + b;
>     }
>
> It generates a C file with e.g.:
>
>     #include "a.h"
>
>     // Static wrappers
>
>     int foo__extern(int a, int b) { return foo(a, b); }
>
> And then in the usual Rust bindings:
>
>     extern "C" {
>         #[link_name = "foo__extern"]
>         pub fn foo(a: ::std::os::raw::c_int, b: ::std::os::raw::c_int)
> -> ::std::os::raw::c_int;
>     }

This is nice! It would be awesome if we could have something similar for
macros. I am not sure if it is possible though. For the null_block
demonstrator I had to move some C macros to Rust, for instance to
implement iterators of bvec [1]. In the particular case of
`bvec_iter_bvec()` it is not possible to wrap the macro in a function
because the macro operates on a value, not a reference. We would have to
pass an argument by value to the wrapping function in order to invoke
the macro on this stack local variable, and then return the value again.
Not really efficient.

[1] https://lore.kernel.org/rust-for-linux/20230503090708.2524310-5-nmi@metaspace.dk/

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[FUJITA Tomonori]

Hi,
Thanks for reviewing.

On Thu, 15 Jun 2023 13:01:50 +0000
Benno Lossin <benno.lossin@proton.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@gmail.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.

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[Boqun Feng]

On Tue, Jun 13, 2023 at 01:53:22PM +0900, 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@gmail.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) }
> +    }
> +}
> +
> +// 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;
> +}
> +
> +/// 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());
> +            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.
> +        let ptr = from_err_ptr(unsafe {
> +            bindings::alloc_etherdev_mqs(
> +                core::mem::size_of::<*const c_void>() as i32,
> +                tx_queue_size,
> +                rx_queue_size,
> +            )
> +        })?;
> +
> +        // SAFETY: `ptr` is valid and non-null since `alloc_etherdev_mqs()`
> +        // returned a valid pointer which was null-checked.

Hmm.. neither alloc_etherdev_mqs() nor `from_err_ptr` would do the
null-check IIUC, so you may get a `ptr` whose values is null here.

Regards,
Boqun

> +        let dev = unsafe { Device::from_ptr(ptr) };
> +        // SAFETY: It's safe to write an address to the returned pointer
> +        // from `netdev_priv()` because `alloc_etherdev_mqs()` allocates
> +        // contiguous memory for `struct net_device` and a pointer.
> +        unsafe {
> +            let priv_ptr = bindings::netdev_priv(ptr) as *mut *const c_void;
> +            core::ptr::write(priv_ptr, data.into_foreign());
> +        }
> +        Ok(Registration {
> +            dev,
> +            is_registered: false,
> +            _p: PhantomData,
> +        })
> +    }
> +
[...]

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[FUJITA Tomonori]

Hi,

On Wed, 21 Jun 2023 15:44:42 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Tue, Jun 13, 2023 at 01:53:22PM +0900, FUJITA Tomonori wrote:
>> +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.
>> +        let ptr = from_err_ptr(unsafe {
>> +            bindings::alloc_etherdev_mqs(
>> +                core::mem::size_of::<*const c_void>() as i32,
>> +                tx_queue_size,
>> +                rx_queue_size,
>> +            )
>> +        })?;
>> +
>> +        // SAFETY: `ptr` is valid and non-null since `alloc_etherdev_mqs()`
>> +        // returned a valid pointer which was null-checked.
> 
> Hmm.. neither alloc_etherdev_mqs() nor `from_err_ptr` would do the
> null-check IIUC, so you may get a `ptr` whose values is null here.

Oops, `ptr.is_null()` should be used here instead.

Thanks a lot!

Re: [PATCH 5/5] samples: rust: add dummy network driver

[FUJITA Tomonori]

Hi,

On Thu, 15 Jun 2023 13:08:39 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 6/13/23 06:53, FUJITA Tomonori wrote:
>> This is a simpler version of drivers/net/dummy.c.
>> 
>> This demonstrates the usage of abstractions for network device drivers.
>> 
>> Allows allocator_api feature for Box::try_new();
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> ---
>>  samples/rust/Kconfig           | 12 +++++
>>  samples/rust/Makefile          |  1 +
>>  samples/rust/rust_net_dummy.rs | 81 ++++++++++++++++++++++++++++++++++
>>  scripts/Makefile.build         |  2 +-
>>  4 files changed, 95 insertions(+), 1 deletion(-)
>>  create mode 100644 samples/rust/rust_net_dummy.rs
>> 
>> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
>> index b0f74a81c8f9..8b52ba620ae3 100644
>> --- a/samples/rust/Kconfig
>> +++ b/samples/rust/Kconfig
>> @@ -30,6 +30,18 @@ config SAMPLE_RUST_PRINT
>> 
>>  	 If unsure, say N.
>> 
>> +config SAMPLE_RUST_NET_DUMMY
>> +	tristate "Dummy network driver"
>> +	depends on NET
>> +	help
>> +	 This is the simpler version of drivers/net/dummy.c. No intention to replace it.
>> +	 This provides educational information for Rust abstractions for network drivers.
>> +
>> +	 To compile this as a module, choose M here:
>> +	 the module will be called rust_minimal.
> 
> The module is not called `rust_minimal` :)

Oops, I'll fix it in the next version.

thanks,

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[Benno Lossin]

On 6/21/23 15:13, FUJITA Tomonori wrote:
> Hi,
> Thanks for reviewing.
> 
> On Thu, 15 Jun 2023 13:01:50 +0000
> Benno Lossin <benno.lossin@proton.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@gmail.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 think this should be encoded in the type invariants of `Device`. 
Because the safety comment needs some justification.

> 
>> 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
>              ))
>          }
>      }
> }

LGTM (+ adding a Safety comment).

> 
> 
>>> +// 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)
> 

In the example both structs are empty so I am not really sure why it has 
to be two types. Can't we do this:
```
pub struct MyDriver {
     // Just some random fields...
     pub access_count: Cell<usize>,
}


impl DriverData for Box<MyDriver> {}

// And then we could make `DeviceOperations: DriverData`.
// Users would then do this:

#[vtable]
impl DeviceOperations for Box<MyDriver> {
     fn init(_dev: Device, data: &MyDriver) -> Result {
         data.access_count.set(0);
         Ok(())
     }

     fn open(_dev: Device, data: &MyDriver) -> Result {
         data.access_count.set(data.access_count.get() + 1);
         Ok(())
     }
}
```

I think this would simplify things, because you do not have to carry the 
extra associated type around (and have to spell out
`<D::Data as ForeignOwnable>` everywhere).

> 
>>> +/// 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.
> 
> ?

Yes you should just write that.

> 
>>> +    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.

Can't you just use `&Self::DEVICE_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).

Could you point me to those other things that use `DriverData`?

> 
> 
>>> +    /// 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

I do not really see a reason why it should be `&mut Device` over just 
`Device`.

-- 
Cheers,
Benno

> 
>>> +/// 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.
> 

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[FUJITA Tomonori]

Hi,

On Sun, 25 Jun 2023 09:52:53 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 6/21/23 15:13, FUJITA Tomonori wrote:
>> Hi,
>> Thanks for reviewing.
>> 
>> On Thu, 15 Jun 2023 13:01:50 +0000
>> Benno Lossin <benno.lossin@proton.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@gmail.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 think this should be encoded in the type invariants of `Device`. 
> Because the safety comment needs some justification.
> 
>> 
>>> 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
>>              ))
>>          }
>>      }
>> }
> 
> LGTM (+ adding a Safety comment).
> 
>> 
>> 
>>>> +// 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)
>> 
> 
> In the example both structs are empty so I am not really sure why it has 
> to be two types. Can't we do this:
> ```
> pub struct MyDriver {
>      // Just some random fields...
>      pub access_count: Cell<usize>,
> }
> 
> 
> impl DriverData for Box<MyDriver> {}
> 
> // And then we could make `DeviceOperations: DriverData`.
> // Users would then do this:
> 
> #[vtable]
> impl DeviceOperations for Box<MyDriver> {
>      fn init(_dev: Device, data: &MyDriver) -> Result {
>          data.access_count.set(0);
>          Ok(())
>      }
> 
>      fn open(_dev: Device, data: &MyDriver) -> Result {
>          data.access_count.set(data.access_count.get() + 1);
>          Ok(())
>      }
> }
> ```
> 
> I think this would simplify things, because you do not have to carry the 
> extra associated type around (and have to spell out
> `<D::Data as ForeignOwnable>` everywhere).

I'm still not sure if I correctly understand what you try to do.

If I define DeviceOperations in dev.rs like the following:

#[vtable]
pub trait DeviceOperations<D: ForeignOwnable + Send + Sync> {
    /// Corresponds to `ndo_init` in `struct net_device_ops`.
        fn init(_dev: &mut Device, _data: D::Borrowed<'_>) -> Result {
	        Ok(())
        }
}

And the driver implmeents DeviceOperations like the folloing: 

#[vtable]
impl<D: ForeignOwnable + Send + Sync> DeviceOperations<D> for Box<DriverData> {
    fn init(_dev: &mut Device, _data: &DriverData) -> Result {
            Ok(())
    }
}

I got the following error:

error[E0053]: method `init` has an incompatible type for trait
  --> samples/rust/rust_net_dummy.rs:24:39
   |
24 |     fn init(_dev: &mut Device, _data: &DriverData) -> Result {
   |                                       ^^^^^^^^^^^
   |                                       |
   |                                       expected associated type, found `&DriverData`
   |                                       help: change the parameter type to match the trait: `<D as ForeignOwnable>::Borrowed<'_>`
   |
   = note: expected signature `fn(&mut Device, <D as ForeignOwnable>::Borrowed<'_>) -> core::result::Result<_, _>`
              found signature `fn(&mut Device, &DriverData) -> core::result::Result<_, _>`


>>>> +/// 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.
>> 
>> ?
> 
> Yes you should just write that.
> 
>> 
>>>> +    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.
> 
> Can't you just use `&Self::DEVICE_OPS`?

I think that it didn't work in the past but seems that it works
now. I'll fix.


>>>> +
>>>> +    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).
> 
> Could you point me to those other things that use `DriverData`?

net_device struct has some like tlsdev_ops, rtnl_link_ops.. A device
driver might need to access to the private data via net_device in
these operations.


thanks,

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[Benno Lossin]

On 25.06.23 16:27, FUJITA Tomonori wrote:
> Hi,
>
> On Sun, 25 Jun 2023 09:52:53 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>
>>>>> +/// 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)
>>>
>>
>> In the example both structs are empty so I am not really sure why it has
>> to be two types. Can't we do this:
>> ```
>> pub struct MyDriver {
>>       // Just some random fields...
>>       pub access_count: Cell<usize>,
>> }
>>
>>
>> impl DriverData for Box<MyDriver> {}
>>
>> // And then we could make `DeviceOperations: DriverData`.
>> // Users would then do this:
>>
>> #[vtable]
>> impl DeviceOperations for Box<MyDriver> {
>>       fn init(_dev: Device, data: &MyDriver) -> Result {
>>           data.access_count.set(0);
>>           Ok(())
>>       }
>>
>>       fn open(_dev: Device, data: &MyDriver) -> Result {
>>           data.access_count.set(data.access_count.get() + 1);
>>           Ok(())
>>       }
>> }
>> ```
>>
>> I think this would simplify things, because you do not have to carry the
>> extra associated type around (and have to spell out
>> `<D::Data as ForeignOwnable>` everywhere).
>
> I'm still not sure if I correctly understand what you try to do.
>
> If I define DeviceOperations in dev.rs like the following:
>
> #[vtable]
> pub trait DeviceOperations<D: ForeignOwnable + Send + Sync> {
>      /// Corresponds to `ndo_init` in `struct net_device_ops`.
>          fn init(_dev: &mut Device, _data: D::Borrowed<'_>) -> Result {
> 	        Ok(())
>          }
> }
>
> And the driver implmeents DeviceOperations like the folloing:
>
> #[vtable]
> impl<D: ForeignOwnable + Send + Sync> DeviceOperations<D> for Box<DriverData> {
>      fn init(_dev: &mut Device, _data: &DriverData) -> Result {
>              Ok(())
>      }
> }
>
> I got the following error:
>
> error[E0053]: method `init` has an incompatible type for trait
>    --> samples/rust/rust_net_dummy.rs:24:39
>     |
> 24 |     fn init(_dev: &mut Device, _data: &DriverData) -> Result {
>     |                                       ^^^^^^^^^^^
>     |                                       |
>     |                                       expected associated type, found `&DriverData`
>     |                                       help: change the parameter type to match the trait: `<D as ForeignOwnable>::Borrowed<'_>`
>     |
>     = note: expected signature `fn(&mut Device, <D as ForeignOwnable>::Borrowed<'_>) -> core::result::Result<_, _>`
>                found signature `fn(&mut Device, &DriverData) -> core::result::Result<_, _>`
>

I thought you could do this:
```
#[vtable]
pub trait DeviceOperations: ForeignOwnable + Send + Sync {
     /// Corresponds to `ndo_init` in `struct net_device_ops`.
     fn init(_dev: &mut Device, _data: Self::Borrowed<'_>) -> Result {
         Ok(())
     }
}

#[vtable]
impl DeviceOperations<D> for Box<DriverData> {
     fn init(_dev: &mut Device, _data: &DriverData) -> Result {
         Ok(())
     }
}
```

>>>>> +    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.
>>
>> Can't you just use `&Self::DEVICE_OPS`?
>
> I think that it didn't work in the past but seems that it works
> now. I'll fix.
>
>
>>>>> +/// 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).
>>
>> Could you point me to those other things that use `DriverData`?
>
> net_device struct has some like tlsdev_ops, rtnl_link_ops.. A device
> driver might need to access to the private data via net_device in
> these operations.

In my mental model you can just implement the `TLSOperations` trait
alongside the `DeviceOperations` trait. But I would have to see the
API that will be used for that to be sure. I do not think that you need
to have different private data for each of those operations traits.

--
Cheers,
Benno

>
>
> thanks,