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

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

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

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.

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          | 292 ++++++++++++++++++++++++++++++++
 5 files changed, 318 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 50e7a76d5455..e3ebaaf9ab4f 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -6,6 +6,8 @@
  * Sorted alphabetically.
  */
 
+#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 81e80261d597..a91d2a99035b 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -23,10 +23,26 @@
 #include <linux/err.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..407d7d5bff2c
--- /dev/null
+++ b/rust/kernel/net/dev.rs
@@ -0,0 +1,292 @@
+// 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`.
+///
+/// # Safety
+///
+/// The kernel uses a `net_device` object with various functions like `struct net_device_ops`.
+/// This object is passed to Rust callbacks while these functions are running.
+/// The C API guarantees that `net_device` isn't released while this function is running.
+pub struct Device(*mut bindings::net_device);
+
+impl Device {
+    /// Gets a pointer to network device private data.
+    ///
+    /// A driver in C uses contiguous memory `struct net_device` and its private data.
+    /// A driver in Rust contiguous memory `struct net_device` and a pointer to its private data.
+    /// So a driver in Rust pays extra cost to access to its private data.
+    /// A device driver passes an properly initialized private data and the kernel saves its pointer.
+    fn priv_data_ptr(netdev: *mut bindings::net_device) -> *const c_void {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { core::ptr::read(bindings::netdev_priv(&mut *netdev) as *const *const c_void) }
+    }
+}
+
+// SAFETY: `Device` exposes the state, `D::Data` object across threads
+// but that type is required to be Send and Sync.
+unsafe impl Send for Device {}
+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.
+///
+/// # Safety
+///
+/// The pointer to the `net_device` object is guaranteed to be valid until
+/// the registration object is dropped.
+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: `dev` was allocated during initialization and guaranteed to be valid.
+        unsafe {
+            if self.is_registered {
+                bindings::unregister_netdev(self.dev.0);
+            }
+            let _ = D::Data::from_foreign(Device::priv_data_ptr(self.dev.0));
+            bindings::free_netdev(self.dev.0);
+        }
+    }
+}
+
+impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> {
+    /// Creates new instance of registration.
+    pub fn try_new(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: the kernel allocated the space for 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: Device(ptr),
+            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: `dev` was allocated during initialization and is guaranteed to be 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
+        },
+        ..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 `net_device` isn't released while this function is running.
+            let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+            let mut dev = Device(netdev);
+            T::init(&mut dev, data)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn uninit_callback(netdev: *mut bindings::net_device) {
+        // SAFETY: The C API guarantees that `net_device` isn't released while this function is running.
+        let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+        let mut dev = Device(netdev);
+        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 `net_device` isn't released while this function is running.
+            let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+            let mut dev = Device(netdev);
+            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 `net_device` isn't released while this function is running.
+            let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+            let mut dev = Device(netdev);
+            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 `net_device` isn't released while this function is running.
+        let mut dev = Device(netdev);
+        let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+        // SAFETY: The C API guarantees that `sk_buff` isn't released while this function is running.
+        let skb = SkBuff(skb);
+        T::start_xmit(&mut dev, data, skb) as bindings::netdev_tx_t
+    }
+}
+
+// SAFETY: `Registration` exposes its state, `D::Data` object across threads
+// but that type is required to be Send and Sync.
+unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {}
+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.
+pub struct SkBuff(*mut bindings::sk_buff);
+
+impl SkBuff {
+    /// Provides a time stamp.
+    pub fn tx_timestamp(&mut self) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            bindings::skb_tx_timestamp(self.0);
+        }
+    }
+}
+
+impl Drop for SkBuff {
+    fn drop(&mut self) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            bindings::kfree_skb_reason(
+                self.0,
+                bindings::skb_drop_reason_SKB_DROP_REASON_NOT_SPECIFIED,
+            )
+        }
+    }
+}
-- 
2.34.1

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

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

This demonstrates the usage of abstractions for network device drivers.

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

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9..d631419b0d2a 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -30,6 +30,17 @@ config SAMPLE_RUST_PRINT
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_NET_DUMMY
+	tristate "Dummy network driver"
+	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..b5278e26e447
--- /dev/null
+++ b/samples/rust/rust_net_dummy.rs
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+//! Rust dummy netdev.
+
+use kernel::{
+    bindings, c_str,
+    net::dev::{
+        ethtool_op_get_ts_info, Device, DeviceOperations, DriverData, EtherOperations,
+        EthtoolTsInfo, Registration, RtnlLinkStats64, SkBuff, TxCode,
+    },
+    prelude::*,
+};
+
+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) {}
+}
+
+// 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(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()
+                | bindings::net_device_flags_IFF_NOARP & !bindings::net_device_flags_IFF_MULTICAST,
+        );
+        netdev.set_priv_flags(
+            netdev.get_priv_flags()
+                | bindings::netdev_priv_flags_IFF_LIVE_ADDR_CHANGE
+                | bindings::netdev_priv_flags_IFF_NO_QUEUE,
+        );
+        netdev.set_random_eth_hw_addr();
+        netdev.set_min_mtu(0);
+        netdev.set_max_mtu(0);
+
+        r.register()?;
+
+        pr_info!("Hello Rust dummy netdev!");
+        Ok(DummyNetdev { _r: r })
+    }
+}
+
+module! {
+    type: DummyNetdev,
+    name: "rust_net_dummy",
+    author: "Rust for Linux Contributors",
+    description: "Rust dummy netdev",
+    license: "GPL v2",
+}
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

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

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

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 thats uses the
feature.

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

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index e3ebaaf9ab4f..71db7ab4d92d 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -7,6 +7,7 @@
  */
 
 #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 407d7d5bff2c..278171c0026e 100644
--- a/rust/kernel/net/dev.rs
+++ b/rust/kernel/net/dev.rs
@@ -129,6 +129,19 @@ 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 {
+            Err(code::EINVAL)
+        } else {
+            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)
@@ -290,3 +303,80 @@ 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 `net_device` isn't released while this function is running.
+            let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+            let mut dev = Device(netdev);
+            let mut info = EthtoolTsInfo(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 safety requirement ensures that the pointer 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`.
+pub struct EthtoolTsInfo(*mut bindings::ethtool_ts_info);
+
+/// Sets up the info for software timestamping.
+///
+/// This can be [`Device`] method, but there are non ethernet devices.
+pub fn ethtool_op_get_ts_info(dev: &mut Device, info: &mut EthtoolTsInfo) -> Result {
+    // SAFETY: The safety requirement ensures that the pointer is valid.
+    unsafe {
+        bindings::ethtool_op_get_ts_info(dev.0, info.0);
+    }
+    Ok(())
+}
-- 
2.34.1

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

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

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 | 76 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/rust/helpers.c b/rust/helpers.c
index a91d2a99035b..7c280c552d64 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -21,6 +21,7 @@
 #include <linux/bug.h>
 #include <linux/build_bug.h>
 #include <linux/err.h>
+#include <linux/etherdevice.h>
 #include <linux/refcount.h>
 #include <linux/mutex.h>
 #include <linux/netdevice.h>
@@ -30,6 +31,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 4217051e70dc..a4628d270fe5 100644
--- a/rust/kernel/net/dev.rs
+++ b/rust/kernel/net/dev.rs
@@ -12,6 +12,7 @@
     bindings,
     error::*,
     prelude::vtable,
+    str::CStr,
     types::{ForeignOwnable, Opaque},
 };
 use {core::ffi::c_void, core::marker::PhantomData};
@@ -36,6 +37,81 @@ fn priv_data_ptr(netdev: *mut bindings::net_device) -> *const c_void {
         // SAFETY: The safety requirement ensures that the pointer is valid.
         unsafe { core::ptr::read(bindings::netdev_priv(&mut *netdev) 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` exposes the state, `D::Data` object across threads
-- 
2.34.1

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

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

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 | 58 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
index 278171c0026e..4217051e70dc 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`.
@@ -168,6 +173,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
+        },
         ..unsafe { core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() }
     };
 
@@ -223,6 +233,18 @@ const fn build_device_ops() -> &'static bindings::net_device_ops {
         let skb = SkBuff(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 `net_device` isn't released while this function is running.
+        let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+        let mut dev = Device(netdev);
+        // SAFETY: for writing and nobody else will read or write to it.
+        let stats = unsafe { RtnlLinkStats64::from_raw(stats) };
+        T::get_stats64(&mut dev, data, stats);
+    }
 }
 
 // SAFETY: `Registration` exposes its state, `D::Data` object across threads
@@ -270,6 +292,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`.
@@ -304,6 +334,32 @@ 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 {
+    /// # 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.
+    pub unsafe fn from_raw<'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

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

[Andrew Lunn]

> diff --git a/rust/helpers.c b/rust/helpers.c
> index 81e80261d597..a91d2a99035b 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -23,10 +23,26 @@
>  #include <linux/err.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);

This question is probably due to me not knowing about rust. Why have
these helpers? They don't appear to don't do anything.

> +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> {
> +    /// Creates new instance of registration.
> +    pub fn try_new(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,
> +            )

In the future, there might be some naming issues there. Not every
network device in Linux is an Ethernet network device. Some network
drivers will call alloc_netdev, or some other specialised
allocator. 

	   Andrew

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

[Andrew Lunn]

> +/// Corresponds to the kernel's `struct ethtool_ts_info`.
> +pub struct EthtoolTsInfo(*mut bindings::ethtool_ts_info);
> +
> +/// Sets up the info for software timestamping.
> +///
> +/// This can be [`Device`] method, but there are non ethernet devices.
> +pub fn ethtool_op_get_ts_info(dev: &mut Device, info: &mut EthtoolTsInfo) -> Result {

Just an FYI: Despite it being called ethtool, it is not limited to
ethernet devices. e.g. CAN devices make use of ethtool.

	 Andrew

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

[Andrew Lunn]

> +        pr_info!("Hello Rust dummy netdev!");

In general, we discourage the use of pr_info(). netdev_info(ndev, ...)
is what should be used, once the device is registered. The output then
includes which device is says "hello world", which can be useful when
you have more than one of them. netdev_info() can be called before the
device is registered, but since the name is not fixed at that point,
the output is a bit more ambiguous.

I would not say this is a blocked for this patchset, but i would add
wrapping netdev_{dbg|warn|info|error}() high on your todo list, they
are pretty fundamental to network drivers.

    Andrew

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

[Miguel Ojeda]

On Sat, Jun 10, 2023 at 9:51 AM FUJITA Tomonori <tomo@exabit.dev> wrote:
>
> +config SAMPLE_RUST_NET_DUMMY
> +       tristate "Dummy network driver"
> +       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.

Long line.

Also, should this depend on `NET`?

> +use kernel::{
> +    bindings, c_str,

We should avoid using `bindings` outside the `kernel` crate (or, in
the future, subsystem crates). Ideally this may be a hard error in the
future (we have to see if we need flexibility in some places).

> +// device driver specific information
> +struct Stats {}

Please follow the documentation style used elsewhere.

> +module! {
> +    type: DummyNetdev,
> +    name: "rust_net_dummy",
> +    author: "Rust for Linux Contributors",
> +    description: "Rust dummy netdev",
> +    license: "GPL v2",
> +}

Please move this to the top like in the other samples -- it is the
convention we are following so far (even though it is different from
the C side, and could change, but we should remain consistent).

>  # Compile Rust sources (.rs)
>  # ---------------------------------------------------------------------------
>
> -rust_allowed_features := new_uninit
> +rust_allowed_features := allocator_api,new_uninit

Please add a note that you are adding this in the commit message, and
which functionality we need from it.

Thanks!

Cheers,
Miguel

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

[Miguel Ojeda]

On Sat, Jun 10, 2023 at 9:27 AM FUJITA Tomonori <tomo@exabit.dev> wrote:
>
> +    /// Sets `ethtool_ops` of `net_device`.
> +    pub fn set_ether_operations<U>(&mut self) -> Result
> +    where
> +        U: EtherOperations<D>,
> +    {
> +        if self.is_registered {
> +            Err(code::EINVAL)
> +        } else {
> +            EtherOperationsAdapter::<U, D>::new().set_ether_ops(&mut self.dev);
> +            Ok(())
> +        }

You may want to use early return style, like in C, i.e. `return Err(...);`.

> +            T::get_ts_info(&mut dev, data, &mut info)?;
> +            Ok(0)

Can this be simplified to:

    T::get_ts_info(&mut dev, data, &mut info)

?

> +    fn set_ether_ops(&self, dev: &mut Device) {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe {
> +            (*dev.0).ethtool_ops = Self::build_ether_ops();
> +        }
> +    }

Which safety requirement? Normally when we say "safety requirement",
we are referring to the `# Safety` section on the current function
(i.e. which would be `unsafe`). But this one is a safe function.

If you are referring to `Device`'s, then that is a `struct`, so it
would have invariants, not safety requirements (I will leave a quick
note there too).

> +/// Sets up the info for software timestamping.
> +///
> +/// This can be [`Device`] method, but there are non ethernet devices.
> +pub fn ethtool_op_get_ts_info(dev: &mut Device, info: &mut EthtoolTsInfo) -> Result {
> +    // SAFETY: The safety requirement ensures that the pointer is valid.
> +    unsafe {
> +        bindings::ethtool_op_get_ts_info(dev.0, info.0);
> +    }

Ditto here: there are no safety requirements in this function.

Cheers,
Miguel

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

[Miguel Ojeda]

On Sat, Jun 10, 2023 at 9:35 AM FUJITA Tomonori <tomo@exabit.dev> wrote:
>
> +/// Corresponds to the kernel's `struct net_device`.
> +///
> +/// # Safety
> +///
> +/// The kernel uses a `net_device` object with various functions like `struct net_device_ops`.
> +/// This object is passed to Rust callbacks while these functions are running.
> +/// The C API guarantees that `net_device` isn't released while this function is running.
> +pub struct Device(*mut bindings::net_device);

This is not a function :) So "while this function is running" does not
make too much sense here.

Also, this is a `struct`, so we do not use `# Safety` sections.
Instead, you may want to give this struct a  `# Invariants` section,
and explain what is guaranteed by this type.

Similarly for `SkBuff` below.

> +    fn priv_data_ptr(netdev: *mut bindings::net_device) -> *const c_void {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe { core::ptr::read(bindings::netdev_priv(&mut *netdev) as *const *const c_void) }
> +    }

Like in the other patch, there is no safety requirement in this
function, so you can't use that to justify it.

Even if this is a private function, it would be best if this is
`unsafe`, then you can require callers to pass you a valid pointer --
you already have a `SAFETY` comment in the callers anyway, and those
are the ones that can actually claim that the pointer is valid.

> +// SAFETY: `Device` exposes the state, `D::Data` object across threads
> +// but that type is required to be Send and Sync.
> +unsafe impl Send for Device {}
> +unsafe impl Sync for Device {}

Please provide a `SAFETY` comment for each. See e.g. the ones we have
for `ARef`.

In addition, what is `D::Data` here? I guess you are referring to
`DriverData::Data` somehow, but it is unclear what is `D` here.

Also, please use Markdown in comments, not just documentation, to be
consistent, e.g.

    ... to be `Send` and `Sync`.

> +/// # Safety
> +///
> +/// The pointer to the `net_device` object is guaranteed to be valid until
> +/// the registration object is dropped.
> +pub struct Registration<T: DeviceOperations<D>, D: DriverData> {
> +    dev: Device,
> +    is_registered: bool,
> +    _p: PhantomData<(D, T)>,
> +}

Ditto about this being a `struct` and `# Safety`.

Also, if it is valid until dropped, then it is "always" valid (as far
as someone having such an object is concerned), so there is no need to
say so.

> +    fn drop(&mut self) {
> +        // SAFETY: `dev` was allocated during initialization and guaranteed to be valid.

Do you mean `dev.0`?

> +        // SAFETY: the kernel allocated the space for a pointer.

Please capitalize to be consistent.

> +        unsafe {
> +            let priv_ptr = bindings::netdev_priv(ptr) as *mut *const c_void;
> +            core::ptr::write(priv_ptr, data.into_foreign());
> +        }

> +        ..unsafe { core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() }

A comment on this would be nice. Also, missing `SAFETY` comment, even
if it is a `const`.

Thanks!

Cheers,
Miguel

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

[Alice Ryhl]

On 6/10/23 16:11, Andrew Lunn wrote:
>> diff --git a/rust/helpers.c b/rust/helpers.c
>> index 81e80261d597..a91d2a99035b 100644
>> --- a/rust/helpers.c
>> +++ b/rust/helpers.c
>> @@ -23,10 +23,26 @@
>>   #include <linux/err.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);
> 
> This question is probably due to me not knowing about rust. Why have
> these helpers? They don't appear to don't do anything.

It's because we can't call inline C functions directly, so we need a 
non-inline version of it. (Similarly for #defines.)

Alice

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

[Andrew Lunn]

On Sun, Jun 11, 2023 at 10:03:39AM +0200, Alice Ryhl wrote:
> On 6/10/23 16:11, Andrew Lunn wrote:
> > > diff --git a/rust/helpers.c b/rust/helpers.c
> > > index 81e80261d597..a91d2a99035b 100644
> > > --- a/rust/helpers.c
> > > +++ b/rust/helpers.c
> > > @@ -23,10 +23,26 @@
> > >   #include <linux/err.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);
> > 
> > This question is probably due to me not knowing about rust. Why have
> > these helpers? They don't appear to don't do anything.
> 
> It's because we can't call inline C functions directly, so we need a
> non-inline version of it. (Similarly for #defines.)

Ah, thanks for the explanation.

This is going to be a problem for networking. The hot path has a lot
of inline functions, because a function call is expensive. So there
are going to be a lot of little wrappers like this. I don't want to
encourage early optimisation without proper profiling, but at some
point you might want to replace these wrappers with Rust, using
whatever its equivalent of inline is.

	 Andrew

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

[Miguel Ojeda]

On Sun, Jun 11, 2023 at 6:01 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Ah, thanks for the explanation.
>
> This is going to be a problem for networking. The hot path has a lot
> of inline functions, because a function call is expensive. So there
> are going to be a lot of little wrappers like this. I don't want to
> encourage early optimisation without proper profiling, but at some
> point you might want to replace these wrappers with Rust, using
> whatever its equivalent of inline is.

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.

Cheers,
Miguel

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

[FUJITA Tomonori]

Hi,
Thanks a lot for reviewing!

On Sat, 10 Jun 2023 21:49:50 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Sat, Jun 10, 2023 at 9:35 AM FUJITA Tomonori <tomo@exabit.dev> wrote:
>>
>> +/// Corresponds to the kernel's `struct net_device`.
>> +///
>> +/// # Safety
>> +///
>> +/// The kernel uses a `net_device` object with various functions like `struct net_device_ops`.
>> +/// This object is passed to Rust callbacks while these functions are running.
>> +/// The C API guarantees that `net_device` isn't released while this function is running.
>> +pub struct Device(*mut bindings::net_device);
> 
> This is not a function :) So "while this function is running" does not
> make too much sense here.
> 
> Also, this is a `struct`, so we do not use `# Safety` sections.
> Instead, you may want to give this struct a  `# Invariants` section,
> and explain what is guaranteed by this type.
> 
> Similarly for `SkBuff` below.

Sorry, obviously, I didn't understand comments about safety. Aded `#
Invariants` section to both structures.

I've updated the comments and put the fixed version at the end. Can
you check again? If it looks fine, I'll update the comments in the
rest of patches.


>> +    fn priv_data_ptr(netdev: *mut bindings::net_device) -> *const c_void {
>> +        // SAFETY: The safety requirement ensures that the pointer is valid.
>> +        unsafe { core::ptr::read(bindings::netdev_priv(&mut *netdev) as *const *const c_void) }
>> +    }
> 
> Like in the other patch, there is no safety requirement in this
> function, so you can't use that to justify it.
> 
> Even if this is a private function, it would be best if this is
> `unsafe`, then you can require callers to pass you a valid pointer --
> you already have a `SAFETY` comment in the callers anyway, and those
> are the ones that can actually claim that the pointer is valid.

Fixed.

>> +// SAFETY: `Device` exposes the state, `D::Data` object across threads
>> +// but that type is required to be Send and Sync.
>> +unsafe impl Send for Device {}
>> +unsafe impl Sync for Device {}
> 
> Please provide a `SAFETY` comment for each. See e.g. the ones we have
> for `ARef`.
> 
> In addition, what is `D::Data` here? I guess you are referring to
> `DriverData::Data` somehow, but it is unclear what is `D` here.
> 
> Also, please use Markdown in comments, not just documentation, to be
> consistent, e.g.
> 
>     ... to be `Send` and `Sync`.

Fixed both.

>> +/// # Safety
>> +///
>> +/// The pointer to the `net_device` object is guaranteed to be valid until
>> +/// the registration object is dropped.
>> +pub struct Registration<T: DeviceOperations<D>, D: DriverData> {
>> +    dev: Device,
>> +    is_registered: bool,
>> +    _p: PhantomData<(D, T)>,
>> +}
> 
> Ditto about this being a `struct` and `# Safety`.
>
> Also, if it is valid until dropped, then it is "always" valid (as far
> as someone having such an object is concerned), so there is no need to
> say so.

Fixed.


>> +    fn drop(&mut self) {
>> +        // SAFETY: `dev` was allocated during initialization and guaranteed to be valid.
> 
> Do you mean `dev.0`?
> 
>> +        // SAFETY: the kernel allocated the space for a pointer.
> 
> Please capitalize to be consistent.

Fixed both.

>> +        unsafe {
>> +            let priv_ptr = bindings::netdev_priv(ptr) as *mut *const c_void;
>> +            core::ptr::write(priv_ptr, data.into_foreign());
>> +        }
> 
>> +        ..unsafe { core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() }
> 
> A comment on this would be nice. Also, missing `SAFETY` comment, even
> if it is a `const`.

Added.

Here's a fixed version.


rust: core abstractions for network device drivers

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          | 340 ++++++++++++++++++++++++++++++++
 5 files changed, 366 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 50e7a76d5455..e3ebaaf9ab4f 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -6,6 +6,8 @@
  * Sorted alphabetically.
  */
 
+#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 81e80261d597..a91d2a99035b 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -23,10 +23,26 @@
 #include <linux/err.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..86f525b02479
--- /dev/null
+++ b/rust/kernel/net/dev.rs
@@ -0,0 +1,340 @@
+// 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.
+    ///
+    /// A driver in C uses contiguous memory `struct net_device` and its private data.
+    /// A driver in Rust contiguous memory `struct net_device` and a pointer to its private data.
+    /// So a driver in Rust pays extra cost to access to its private data.
+    /// A device driver passes an properly initialized private data and the kernel saves
+    /// its pointer.
+    fn priv_data_ptr(&self) -> *const c_void {
+        // SAFETY: The type invariants guarantee that `self.0` is valid.
+        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.
+    pub fn try_new(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 {
+            let priv_ptr = bindings::netdev_priv(ptr) as *mut *const c_void;
+            core::ptr::write(priv_ptr, data.into_foreign());
+            Device::from_ptr(ptr)
+        };
+        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<&T>` 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 its state, `Device` object across threads
+// but that type is `Sync`.
+unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {}
+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

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

[FUJITA Tomonori]

Hi,

On Sat, 10 Jun 2023 16:11:41 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> {
>> +    /// Creates new instance of registration.
>> +    pub fn try_new(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,
>> +            )
> 
> In the future, there might be some naming issues there. Not every
> network device in Linux is an Ethernet network device. Some network
> drivers will call alloc_netdev, or some other specialised
> allocator. 

Yeah, I thought about it. Actually dummy.c uses alloc_netdev (then
calls ether_setup so does the same things that alloc_etherdev_mqs).

Wrapping alloc_etherdev_mqs is simpler because of a function pointer
argument in alloc_netdev so I used alloc_etherdev_mqs.

Once someone takes up the issue, I guess that we could add new
functions for other allocators.

thanks,

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

[FUJITA Tomonori]

Hi,

On Sat, 10 Jun 2023 18:48:00 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +/// Corresponds to the kernel's `struct ethtool_ts_info`.
>> +pub struct EthtoolTsInfo(*mut bindings::ethtool_ts_info);
>> +
>> +/// Sets up the info for software timestamping.
>> +///
>> +/// This can be [`Device`] method, but there are non ethernet devices.
>> +pub fn ethtool_op_get_ts_info(dev: &mut Device, info: &mut EthtoolTsInfo) -> Result {
> 
> Just an FYI: Despite it being called ethtool, it is not limited to
> ethernet devices. e.g. CAN devices make use of ethtool.

I see, I'll remove the comment in the next version.

thanks,

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

[FUJITA Tomonori]

Hi,

On Sat, 10 Jun 2023 18:59:52 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +        pr_info!("Hello Rust dummy netdev!");
> 
> In general, we discourage the use of pr_info(). netdev_info(ndev, ...)
> is what should be used, once the device is registered. The output then
> includes which device is says "hello world", which can be useful when
> you have more than one of them. netdev_info() can be called before the
> device is registered, but since the name is not fixed at that point,
> the output is a bit more ambiguous.
> 
> I would not say this is a blocked for this patchset, but i would add
> wrapping netdev_{dbg|warn|info|error}() high on your todo list, they
> are pretty fundamental to network drivers.

Sure, I'll add it to my after-initial-upstreaming TODO list with
per_cpu stuff.

For now, I'll add 'TODO' comment in the next version.

thanks,

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

[FUJITA Tomonori]

Hi,

On Sat, 10 Jun 2023 21:14:37 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Sat, Jun 10, 2023 at 9:27 AM FUJITA Tomonori <tomo@exabit.dev> wrote:
>>
>> +    /// Sets `ethtool_ops` of `net_device`.
>> +    pub fn set_ether_operations<U>(&mut self) -> Result
>> +    where
>> +        U: EtherOperations<D>,
>> +    {
>> +        if self.is_registered {
>> +            Err(code::EINVAL)
>> +        } else {
>> +            EtherOperationsAdapter::<U, D>::new().set_ether_ops(&mut self.dev);
>> +            Ok(())
>> +        }
> 
> You may want to use early return style, like in C, i.e. `return Err(...);`.

Sure, I'll update the code in that way.

>> +            T::get_ts_info(&mut dev, data, &mut info)?;
>> +            Ok(0)
> 
> Can this be simplified to:
> 
>     T::get_ts_info(&mut dev, data, &mut info)
> 
> ?

We could do:

T::get_ts_info(&mut dev, data, &mut info).map(|_| 0)

thanks,

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

[Andrew Lunn]

> Once someone takes up the issue, I guess that we could add new
> functions for other allocators.

Then it would be good to name this function to make it clear it
allocates and ethernet netdev, so leaving the namespace clean for
other allocators.

      Andrew

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

[Miguel Ojeda]

On Mon, Jun 12, 2023 at 7:04 AM FUJITA Tomonori <tomo@exabit.dev> wrote:
>
> Sorry, obviously, I didn't understand comments about safety. Aded `#
> Invariants` section to both structures.
>
> I've updated the comments and put the fixed version at the end. Can
> you check again? If it looks fine, I'll update the comments in the
> rest of patches.

Looks better now (without having checked if they are correct w.r.t.
what the C side guarantees) -- thanks! A couple comments below.

> +    /// Gets a pointer to network device private data.
> +    ///
> +    /// A driver in C uses contiguous memory `struct net_device` and its private data.
> +    /// A driver in Rust contiguous memory `struct net_device` and a pointer to its private data.
> +    /// So a driver in Rust pays extra cost to access to its private data.
> +    /// A device driver passes an properly initialized private data and the kernel saves
> +    /// its pointer.
> +    fn priv_data_ptr(&self) -> *const c_void {
> +        // SAFETY: The type invariants guarantee that `self.0` is valid.
> +        unsafe { core::ptr::read(bindings::netdev_priv(self.0) as *const *const c_void) }
> +    }

Part of the docs of this function should probably be moved to the
`SAFETY` comment, since it is needed to justify the `ptr::read` (and
it is an implementation detail): `self.0` is valid here, but we need
to justify why reading from the returned pointer from `netdev_priv` is
OK, and for that one needs to know the layout explained in the docs
above.

Also, in general, please always aim to minimize what `unsafe` blocks
cover. For instance, here there should be two blocks: one for the FFI
call, and the other for the pointer read. Splitting such blocks makes
it very clear that there are two different proofs needed here.

> +        // SAFETY: `ptr` is valid and non-null since `alloc_etherdev_mqs()`
> +        // returned a valid pointer which was null-checked.
> +        let dev = unsafe {
> +            let priv_ptr = bindings::netdev_priv(ptr) as *mut *const c_void;
> +            core::ptr::write(priv_ptr, data.into_foreign());
> +            Device::from_ptr(ptr)
> +        };

Similarly here, i.e. the `SAFETY` comment does not mention why is
`priv_ptr` valid for the `ptr::write`.

Cheers,
Miguel

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

[Miguel Ojeda]

On Mon, Jun 12, 2023 at 10:51 AM FUJITA Tomonori <tomo@exabit.dev> wrote:
>
> We could do:
>
> T::get_ts_info(&mut dev, data, &mut info).map(|_| 0)

Ah, yeah, then the one you had originally is probably better.

Cheers,
Miguel

[PATCH v2 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          | 330 ++++++++++++++++++++++++++++++++
 5 files changed, 356 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..fe20616668a9
--- /dev/null
+++ b/rust/kernel/net/dev.rs
@@ -0,0 +1,330 @@
+// 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, build_error, error::*, prelude::vtable, types::ForeignOwnable};
+use {core::ffi::c_void, core::marker::PhantomData};
+
+/// Corresponds to the kernel's `struct net_device`.
+///
+/// # Invariants
+///
+/// The `ptr` points to the contiguous memory for `struct net_device` and a pointer,
+/// which stores an address returned by `ForeignOwnable::into_foreign()`.
+pub struct Device<D: ForeignOwnable + Send + Sync> {
+    ptr: *mut bindings::net_device,
+    _p: PhantomData<D>,
+}
+
+impl<D: ForeignOwnable + Send + Sync> Device<D> {
+    /// Creates a new [`Device`] instance.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` must point to the contiguous memory
+    /// for `struct net_device` and a pointer, which stores an address returned
+    /// by `ForeignOwnable::into_foreign()`.
+    unsafe fn from_ptr(ptr: *mut bindings::net_device) -> Self {
+        // INVARIANT: The safety requirements ensure the invariant.
+        Self {
+            ptr,
+            _p: PhantomData,
+        }
+    }
+
+    /// Gets the private data of a device driver.
+    pub fn drv_priv_data(&self) -> D::Borrowed<'_> {
+        // SAFETY: The type invariants guarantee that self.ptr is valid and
+        // bindings::netdev_priv(self.ptr) returns a pointer that stores an address
+        // returned by `ForeignOwnable::into_foreign()`.
+        unsafe {
+            D::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 an object, which is `Sync`
+// so it's safe to sharing its pointer.
+unsafe impl<D: ForeignOwnable + Send + Sync> Send for Device<D> {}
+// 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 an object, which is `Sync`,
+// can be used from any thread too.
+unsafe impl<D: ForeignOwnable + Send + Sync> Sync for Device<D> {}
+
+/// 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> {
+    dev: Device<T>,
+    is_registered: bool,
+    _p: PhantomData<T>,
+}
+
+impl<T: DeviceOperations> Drop for Registration<T> {
+    fn drop(&mut self) {
+        // SAFETY: The type invariants of `Device` guarantee that `self.dev.ptr` is valid and
+        // bindings::netdev_priv(self.ptr) returns a pointer that stores an address
+        // returned by `ForeignOwnable::into_foreign()`.
+        unsafe {
+            let _ = T::from_foreign(core::ptr::read(
+                bindings::netdev_priv(self.dev.ptr) as *const *const c_void
+            ));
+        }
+        // SAFETY: The type invariants of `Device` guarantee that `self.dev.ptr` is valid.
+        unsafe {
+            if self.is_registered {
+                bindings::unregister_netdev(self.dev.ptr);
+            }
+            bindings::free_netdev(self.dev.ptr);
+        }
+    }
+}
+
+impl<T: DeviceOperations> Registration<T> {
+    /// 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: T) -> Result<Self> {
+        // SAFETY: Just an FFI call with no additional safety requirements.
+        let ptr = unsafe {
+            bindings::alloc_etherdev_mqs(
+                core::mem::size_of::<*const c_void>() as i32,
+                tx_queue_size,
+                rx_queue_size,
+            )
+        };
+        if ptr.is_null() {
+            return Err(code::ENOMEM);
+        }
+
+        // SAFETY: It's safe to write an address 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());
+        }
+
+        // SAFETY: `ptr` points to contiguous memory for `struct net_device` and a pointer,
+        // which stores an address returned by `ForeignOwnable::into_foreign()`.
+        let dev = unsafe { Device::from_ptr(ptr) };
+        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<T> {
+        &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.ptr` is valid.
+        let ret = unsafe {
+            (*self.dev.ptr).netdev_ops = &Self::DEVICE_OPS;
+            bindings::register_netdev(self.dev.ptr)
+        };
+        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() }
+    };
+
+    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 dev = unsafe { Device::from_ptr(netdev) };
+            T::init(dev)?;
+            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 dev = unsafe { Device::from_ptr(netdev) };
+        T::uninit(dev);
+    }
+
+    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 dev = unsafe { Device::from_ptr(netdev) };
+            T::open(dev)?;
+            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 dev = unsafe { Device::from_ptr(netdev) };
+            T::stop(dev)?;
+            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 dev = unsafe { Device::from_ptr(netdev) };
+        // SAFETY: The C API guarantees that `skb` is valid until a driver releases the skb.
+        let skb = unsafe { SkBuff::from_ptr(skb) };
+        T::start_xmit(dev, skb) as bindings::netdev_tx_t
+    }
+}
+
+// SAFETY: `Registration` exposes only `Device` object which can be used from any thread.
+unsafe impl<T: DeviceOperations> Send for Registration<T> {}
+// SAFETY: `Registration` exposes only `Device` object which can be used from any thread.
+unsafe impl<T: DeviceOperations> Sync for Registration<T> {}
+
+/// 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: ForeignOwnable + Send + Sync {
+    /// Corresponds to `ndo_init` in `struct net_device_ops`.
+    fn init(_dev: Device<Self>) -> Result {
+        Ok(())
+    }
+
+    /// Corresponds to `ndo_uninit` in `struct net_device_ops`.
+    fn uninit(_dev: Device<Self>) {}
+
+    /// Corresponds to `ndo_open` in `struct net_device_ops`.
+    fn open(_dev: Device<Self>) -> Result {
+        Ok(())
+    }
+
+    /// Corresponds to `ndo_stop` in `struct net_device_ops`.
+    fn stop(_dev: Device<Self>) -> Result {
+        Ok(())
+    }
+
+    /// Corresponds to `ndo_start_xmit` in `struct net_device_ops`.
+    fn start_xmit(_dev: Device<Self>, _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.
+///
+/// A driver must explicitly call a function to drop a `sk_buff` object.
+/// The code to let a `SkBuff` object go out of scope can't be compiled.
+///
+/// # 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);
+        }
+    }
+
+    /// Consumes a [`sk_buff`] object.
+    pub fn consume(self) {
+        // SAFETY: The type invariants guarantee that `self.0` is valid.
+        unsafe {
+            bindings::kfree_skb_reason(self.0, bindings::skb_drop_reason_SKB_CONSUMED);
+        }
+        core::mem::forget(self);
+    }
+}
+
+impl Drop for SkBuff {
+    #[inline(always)]
+    fn drop(&mut self) {
+        build_error!("skb must be released explicitly");
+    }
+}
-- 
2.34.1

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

[Benno Lossin]

> 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          | 330 ++++++++++++++++++++++++++++++++
>  5 files changed, 356 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..fe20616668a9
> --- /dev/null
> +++ b/rust/kernel/net/dev.rs
> @@ -0,0 +1,330 @@
> +// 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, build_error, error::*, prelude::vtable, types::ForeignOwnable};
> +use {core::ffi::c_void, core::marker::PhantomData};
> +
> +/// Corresponds to the kernel's `struct net_device`.
> +///
> +/// # Invariants
> +///
> +/// The `ptr` points to the contiguous memory for `struct net_device` and a pointer,
> +/// which stores an address returned by `ForeignOwnable::into_foreign()`.
> +pub struct Device<D: ForeignOwnable + Send + Sync> {
> +    ptr: *mut bindings::net_device,
> +    _p: PhantomData<D>,
> +}
> +
> +impl<D: ForeignOwnable + Send + Sync> Device<D> {
> +    /// Creates a new [`Device`] instance.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `ptr` must point to the contiguous memory
> +    /// for `struct net_device` and a pointer, which stores an address returned
> +    /// by `ForeignOwnable::into_foreign()`.
> +    unsafe fn from_ptr(ptr: *mut bindings::net_device) -> Self {
> +        // INVARIANT: The safety requirements ensure the invariant.
> +        Self {
> +            ptr,
> +            _p: PhantomData,
> +        }
> +    }
> +
> +    /// Gets the private data of a device driver.
> +    pub fn drv_priv_data(&self) -> D::Borrowed<'_> {
> +        // SAFETY: The type invariants guarantee that self.ptr is valid and
> +        // bindings::netdev_priv(self.ptr) returns a pointer that stores an address
> +        // returned by `ForeignOwnable::into_foreign()`.
> +        unsafe {
> +            D::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 an object, which is `Sync`
> +// so it's safe to sharing its pointer.
> +unsafe impl<D: ForeignOwnable + Send + Sync> Send for Device<D> {}
> +// 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 an object, which is `Sync`,
> +// can be used from any thread too.
> +unsafe impl<D: ForeignOwnable + Send + Sync> Sync for Device<D> {}
> +
> +/// 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> {
> +    dev: Device<T>,
> +    is_registered: bool,
> +    _p: PhantomData<T>,
> +}
> +
> +impl<T: DeviceOperations> Drop for Registration<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: The type invariants of `Device` guarantee that `self.dev.ptr` is valid and
> +        // bindings::netdev_priv(self.ptr) returns a pointer that stores an address
> +        // returned by `ForeignOwnable::into_foreign()`.
> +        unsafe {
> +            let _ = T::from_foreign(core::ptr::read(
> +                bindings::netdev_priv(self.dev.ptr) as *const *const c_void
> +            ));
> +        }
> +        // SAFETY: The type invariants of `Device` guarantee that `self.dev.ptr` is valid.
> +        unsafe {
> +            if self.is_registered {
> +                bindings::unregister_netdev(self.dev.ptr);
> +            }
> +            bindings::free_netdev(self.dev.ptr);
> +        }
> +    }
> +}
> +
> +impl<T: DeviceOperations> Registration<T> {
> +    /// 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: T) -> Result<Self> {
> +        // SAFETY: Just an FFI call with no additional safety requirements.
> +        let ptr = unsafe {
> +            bindings::alloc_etherdev_mqs(
> +                core::mem::size_of::<*const c_void>() as i32,
> +                tx_queue_size,
> +                rx_queue_size,
> +            )
> +        };
> +        if ptr.is_null() {
> +            return Err(code::ENOMEM);
> +        }
> +
> +        // SAFETY: It's safe to write an address 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());
> +        }
> +
> +        // SAFETY: `ptr` points to contiguous memory for `struct net_device` and a pointer,
> +        // which stores an address returned by `ForeignOwnable::into_foreign()`.
> +        let dev = unsafe { Device::from_ptr(ptr) };
> +        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<T> {
> +        &mut self.dev
> +    }

I think you could instead implement `AsMut`.

> +
> +    /// 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.ptr` is valid.
> +        let ret = unsafe {
> +            (*self.dev.ptr).netdev_ops = &Self::DEVICE_OPS;
> +            bindings::register_netdev(self.dev.ptr)
> +        };
> +        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() }
> +    };
> +
> +    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 dev = unsafe { Device::from_ptr(netdev) };
> +            T::init(dev)?;
> +            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 dev = unsafe { Device::from_ptr(netdev) };
> +        T::uninit(dev);
> +    }
> +
> +    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 dev = unsafe { Device::from_ptr(netdev) };
> +            T::open(dev)?;
> +            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 dev = unsafe { Device::from_ptr(netdev) };
> +            T::stop(dev)?;
> +            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 dev = unsafe { Device::from_ptr(netdev) };
> +        // SAFETY: The C API guarantees that `skb` is valid until a driver releases the skb.
> +        let skb = unsafe { SkBuff::from_ptr(skb) };
> +        T::start_xmit(dev, skb) as bindings::netdev_tx_t
> +    }
> +}
> +
> +// SAFETY: `Registration` exposes only `Device` object which can be used from any thread.
> +unsafe impl<T: DeviceOperations> Send for Registration<T> {}
> +// SAFETY: `Registration` exposes only `Device` object which can be used from any thread.
> +unsafe impl<T: DeviceOperations> Sync for Registration<T> {}
> +
> +/// 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,
> +}

Is it really necessary that this has the same representation as the
C constants? Would a function that converts be sufficient? I think we
should let the compiler decide the layout when we have no concrete
requirements.

> +
> +/// 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: ForeignOwnable + Send + Sync {
> +    /// Corresponds to `ndo_init` in `struct net_device_ops`.
> +    fn init(_dev: Device<Self>) -> Result {
> +        Ok(())
> +    }
> +
> +    /// Corresponds to `ndo_uninit` in `struct net_device_ops`.
> +    fn uninit(_dev: Device<Self>) {}
> +
> +    /// Corresponds to `ndo_open` in `struct net_device_ops`.
> +    fn open(_dev: Device<Self>) -> Result {
> +        Ok(())
> +    }
> +
> +    /// Corresponds to `ndo_stop` in `struct net_device_ops`.
> +    fn stop(_dev: Device<Self>) -> Result {
> +        Ok(())
> +    }
> +
> +    /// Corresponds to `ndo_start_xmit` in `struct net_device_ops`.
> +    fn start_xmit(_dev: Device<Self>, _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.
> +///
> +/// A driver must explicitly call a function to drop a `sk_buff` object.
> +/// The code to let a `SkBuff` object go out of scope can't be compiled.
> +///
> +/// # 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);
> +        }
> +    }
> +
> +    /// Consumes a [`sk_buff`] object.
> +    pub fn consume(self) {
> +        // SAFETY: The type invariants guarantee that `self.0` is valid.
> +        unsafe {
> +            bindings::kfree_skb_reason(self.0, bindings::skb_drop_reason_SKB_CONSUMED);
> +        }
> +        core::mem::forget(self);

I read the prior discussion and I just wanted to make one thing sure:
dropping the `sk_buff` is not required for the safety of a program (of course
it still is a leak and that is not good), so it does not mean UB can occur
at some later point.

If leaking a `sk_buff` is fine, then I have no complaints, but we need to
keep this in mind when reviewing code that uses `sk_buff`, since there
using `forget` or other ways of leaking objects should not happen.

--
Cheers,
Benno

> +    }
> +}
> +
> +impl Drop for SkBuff {
> +    #[inline(always)]
> +    fn drop(&mut self) {
> +        build_error!("skb must be released explicitly");
> +    }
> +}
> --
> 2.34.1
>