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

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

[FUJITA Tomonori]

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

The major change is a way to drop an skb (1/5 patch); a driver needs
to explicitly call a function to drop a skb. The code to let a skb
go out of scope can't be compiled.

I dropped get_stats64 support patch that the current sample driver
doesn't use. Instead I added a patch to update the NETWORKING DRIVERS
entry in MAINTAINERS.

Changes since v1 [1]:
- a driver must explicitly call a function to drop a skb.
- simplify the code (thanks to Benno Lossin).
- update MAINTAINERS file.

[1] https://lwn.net/ml/netdev/20230613045326.3938283-1-fujita.tomonori@gmail.com/

FUJITA Tomonori (5):
  rust: core abstractions for network device drivers
  rust: add support for ethernet operations
  rust: add methods for configure net_device
  samples: rust: add dummy network driver
  MAINTAINERS: add Rust network abstractions files to the NETWORKING
    DRIVERS entry

 MAINTAINERS                     |   2 +
 rust/bindings/bindings_helper.h |   3 +
 rust/helpers.c                  |  23 ++
 rust/kernel/lib.rs              |   3 +
 rust/kernel/net.rs              |   5 +
 rust/kernel/net/dev.rs          | 598 ++++++++++++++++++++++++++++++++
 samples/rust/Kconfig            |  13 +
 samples/rust/Makefile           |   1 +
 samples/rust/rust_net_dummy.rs  |  75 ++++
 scripts/Makefile.build          |   2 +-
 10 files changed, 724 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/dev.rs
 create mode 100644 samples/rust/rust_net_dummy.rs


base-commit: d2e3115d717197cb2bc020dd1f06b06538474ac3
-- 
2.34.1

[PATCH 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
> 

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

[FUJITA Tomonori]

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

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

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

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

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 468bf606f174..6446ff764980 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -8,6 +8,7 @@
 
 #include <linux/errname.h>
 #include <linux/etherdevice.h>
+#include <linux/ethtool.h>
 #include <linux/netdevice.h>
 #include <linux/slab.h>
 #include <linux/refcount.h>
diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
index fe20616668a9..ff00616e4fef 100644
--- a/rust/kernel/net/dev.rs
+++ b/rust/kernel/net/dev.rs
@@ -142,7 +142,7 @@ 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.
+        // SAFETY: The type invariants of `Device` guarantee that `self.dev.ptr` is valid.
         let ret = unsafe {
             (*self.dev.ptr).netdev_ops = &Self::DEVICE_OPS;
             bindings::register_netdev(self.dev.ptr)
@@ -155,6 +155,18 @@ pub fn register(&mut self) -> Result {
         }
     }
 
+    /// Sets `ethtool_ops` of `net_device`.
+    pub fn set_ether_operations<E: EtherOperations>(&mut self) -> Result {
+        if self.is_registered {
+            return Err(code::EINVAL);
+        }
+        // SAFETY: The type invariants of `Device` guarantee that `self.dev.ptr` is valid.
+        unsafe {
+            (*(self.dev.ptr)).ethtool_ops = &EtherOperationsAdapter::<E>::ETHER_OPS;
+        }
+        Ok(())
+    }
+
     const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
         ndo_init: if <T>::HAS_INIT {
             Some(Self::init_callback)
@@ -328,3 +340,76 @@ fn drop(&mut self) {
         build_error!("skb must be released explicitly");
     }
 }
+
+/// Builds the kernel's `struct ethtool_ops`.
+struct EtherOperationsAdapter<E: EtherOperations> {
+    _p: PhantomData<E>,
+}
+
+impl<E: EtherOperations> EtherOperationsAdapter<E> {
+    unsafe extern "C" fn get_ts_info_callback(
+        netdev: *mut bindings::net_device,
+        info: *mut bindings::ethtool_ts_info,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `netdev` is valid while this function is running.
+            let dev = unsafe { Device::from_ptr(netdev) };
+            // SAFETY: The C API guarantees that `info` is valid while this function is running.
+            let info = unsafe { EthtoolTsInfo::from_ptr(info) };
+            E::get_ts_info(dev, info)?;
+            Ok(0)
+        })
+    }
+
+    const ETHER_OPS: bindings::ethtool_ops = bindings::ethtool_ops {
+        get_ts_info: if <E>::HAS_GET_TS_INFO {
+            Some(Self::get_ts_info_callback)
+        } else {
+            None
+        },
+        // SAFETY: The rest is zeroed out to initialize `struct ethtool_ops`,
+        // set `Option<&F>` to be `None`.
+        ..unsafe { core::mem::MaybeUninit::<bindings::ethtool_ops>::zeroed().assume_init() }
+    };
+}
+
+/// Corresponds to the kernel's `struct ethtool_ops`.
+#[vtable]
+pub trait EtherOperations: ForeignOwnable + Send + Sync {
+    /// Corresponds to `get_ts_info` in `struct ethtool_ops`.
+    fn get_ts_info(_dev: Device<Self>, _info: EthtoolTsInfo) -> Result {
+        Err(Error::from_errno(bindings::EOPNOTSUPP as i32))
+    }
+}
+
+/// Corresponds to the kernel's `struct ethtool_ts_info`.
+///
+/// # Invariants
+///
+/// The pointer is valid.
+pub struct EthtoolTsInfo(*mut bindings::ethtool_ts_info);
+
+impl EthtoolTsInfo {
+    /// Creates a new `EthtoolTsInfo' instance.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` must be valid.
+    unsafe fn from_ptr(ptr: *mut bindings::ethtool_ts_info) -> Self {
+        // INVARIANT: The safety requirements ensure the invariant.
+        Self(ptr)
+    }
+
+    /// Sets up the info for software timestamping.
+    pub fn ethtool_op_get_ts_info<D: ForeignOwnable + Send + Sync>(
+        dev: &Device<D>,
+        info: &mut EthtoolTsInfo,
+    ) -> Result {
+        // SAFETY: The type invariants of `Device` guarantee that `dev.ptr` are valid.
+        // The type invariants guarantee that `info.0` are valid.
+        unsafe {
+            bindings::ethtool_op_get_ts_info(dev.ptr, info.0);
+        }
+        Ok(())
+    }
+}
-- 
2.34.1

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

[Benno Lossin]

> 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_device_ops.
> 
> Currently, only get_ts_info operation is supported. The following
> patch adds the Rust version of the dummy network driver, which uses
> the operation.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/bindings/bindings_helper.h |  1 +
>  rust/kernel/net/dev.rs          | 87 ++++++++++++++++++++++++++++++++-
>  2 files changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 468bf606f174..6446ff764980 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -8,6 +8,7 @@
> 
>  #include <linux/errname.h>
>  #include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
>  #include <linux/netdevice.h>
>  #include <linux/slab.h>
>  #include <linux/refcount.h>
> diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
> index fe20616668a9..ff00616e4fef 100644
> --- a/rust/kernel/net/dev.rs
> +++ b/rust/kernel/net/dev.rs
> @@ -142,7 +142,7 @@ 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.
> +        // SAFETY: The type invariants of `Device` guarantee that `self.dev.ptr` is valid.

Shouldn't this be squashed into patch 1?

>          let ret = unsafe {
>              (*self.dev.ptr).netdev_ops = &Self::DEVICE_OPS;
>              bindings::register_netdev(self.dev.ptr)
> @@ -155,6 +155,18 @@ pub fn register(&mut self) -> Result {
>          }
>      }
> 
> +    /// Sets `ethtool_ops` of `net_device`.
> +    pub fn set_ether_operations<E: EtherOperations>(&mut self) -> Result {
> +        if self.is_registered {
> +            return Err(code::EINVAL);
> +        }
> +        // SAFETY: The type invariants of `Device` guarantee that `self.dev.ptr` is valid.
> +        unsafe {
> +            (*(self.dev.ptr)).ethtool_ops = &EtherOperationsAdapter::<E>::ETHER_OPS;
> +        }
> +        Ok(())
> +    }
> +

I think it would make sense to also add

```
pub fn setup_ether_operations(&mut self) -> Result
where
    T: EtherOperations
{
    self.set_ether_operations::<T>();
}
```

Since normally you would implement `EtherOperations` for the same type
that also implement `DeviceOperations`, right?

>      const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
>          ndo_init: if <T>::HAS_INIT {
>              Some(Self::init_callback)
> @@ -328,3 +340,76 @@ fn drop(&mut self) {
>          build_error!("skb must be released explicitly");
>      }
>  }
> +
> +/// Builds the kernel's `struct ethtool_ops`.
> +struct EtherOperationsAdapter<E: EtherOperations> {
> +    _p: PhantomData<E>,
> +}
> +
> +impl<E: EtherOperations> EtherOperationsAdapter<E> {
> +    unsafe extern "C" fn get_ts_info_callback(
> +        netdev: *mut bindings::net_device,
> +        info: *mut bindings::ethtool_ts_info,
> +    ) -> core::ffi::c_int {
> +        from_result(|| {
> +            // SAFETY: The C API guarantees that `netdev` is valid while this function is running.
> +            let dev = unsafe { Device::from_ptr(netdev) };
> +            // SAFETY: The C API guarantees that `info` is valid while this function is running.
> +            let info = unsafe { EthtoolTsInfo::from_ptr(info) };
> +            E::get_ts_info(dev, info)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    const ETHER_OPS: bindings::ethtool_ops = bindings::ethtool_ops {
> +        get_ts_info: if <E>::HAS_GET_TS_INFO {
> +            Some(Self::get_ts_info_callback)
> +        } else {
> +            None
> +        },
> +        // SAFETY: The rest is zeroed out to initialize `struct ethtool_ops`,
> +        // set `Option<&F>` to be `None`.
> +        ..unsafe { core::mem::MaybeUninit::<bindings::ethtool_ops>::zeroed().assume_init() }
> +    };
> +}
> +
> +/// Corresponds to the kernel's `struct ethtool_ops`.
> +#[vtable]
> +pub trait EtherOperations: ForeignOwnable + Send + Sync {
> +    /// Corresponds to `get_ts_info` in `struct ethtool_ops`.
> +    fn get_ts_info(_dev: Device<Self>, _info: EthtoolTsInfo) -> Result {
> +        Err(Error::from_errno(bindings::EOPNOTSUPP as i32))

Note that you have to use `-(EOPNOTSUPP as i32)`. Maybe just add this
in `error.rs` via the `declare_err` macro.

--
Cheers,
Benno

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

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

[FUJITA Tomonori]

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

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

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

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

[Benno Lossin]

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

Just to make sure, the `name` field in `net_device` does not need to
be nul-terminated, right?

--
Cheers,
Benno

> +        Ok(())
> +    }
> +
> +    /// Sets carrier.
> +    pub fn netif_carrier_on(&mut self) {
> +        // SAFETY: The type invariants guarantee that `self.ptr` is valid.
> +        unsafe { bindings::netif_carrier_on(self.ptr) }
> +    }
> +
> +    /// Clears carrier.
> +    pub fn netif_carrier_off(&mut self) {
> +        // SAFETY: The type invariants guarantee that `self.ptr` is valid.
> +        unsafe { bindings::netif_carrier_off(self.ptr) }
> +    }
> +
> +    /// Sets the max mtu of the device.
> +    pub fn set_max_mtu(&mut self, max_mtu: u32) {
> +        // SAFETY: The type invariants guarantee that `self.ptr` is valid.
> +        unsafe {
> +            (*self.ptr).max_mtu = max_mtu;
> +        }
> +    }
> +
> +    /// Sets the minimum mtu of the device.
> +    pub fn set_min_mtu(&mut self, min_mtu: u32) {
> +        // SAFETY: The type invariants guarantee that `self.ptr` is valid.
> +        unsafe {
> +            (*self.ptr).min_mtu = min_mtu;
> +        }
> +    }
> +
> +    /// Returns the flags of the device.
> +    pub fn get_flags(&self) -> u32 {
> +        // SAFETY: The type invariants guarantee that `self.ptr` is valid.
> +        unsafe { (*self.ptr).flags }
> +    }
> +
> +    /// Sets the flags of the device.
> +    pub fn set_flags(&mut self, flags: u32) {
> +        // SAFETY: The type invariants guarantee that `self.ptr` is valid.
> +        unsafe {
> +            (*self.ptr).flags = flags;
> +        }
> +    }
> +
> +    /// Returns the priv_flags of the device.
> +    pub fn get_priv_flags(&self) -> u64 {
> +        // SAFETY: The type invariants guarantee that `self.ptr` is valid.
> +        unsafe { (*self.ptr).priv_flags }
> +    }
> +
> +    /// Sets the priv_flags of the device.
> +    pub fn set_priv_flags(&mut self, flags: u64) {
> +        // SAFETY: The type invariants guarantee that `self.ptr` is valid.
> +        unsafe { (*self.ptr).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 type invariants guarantee that `self.ptr` is valid.
> +        unsafe { bindings::eth_hw_addr_random(self.ptr) }
> +    }
>  }
> 
>  // SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used
> --
> 2.34.1
> 

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

[FUJITA Tomonori]

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

This demonstrates the usage of abstractions for network device drivers.

Allows allocator_api feature for Box::try_new();

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

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9..02bda7bdf722 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -30,6 +30,19 @@ config SAMPLE_RUST_PRINT
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_NET_DUMMY
+	tristate "Dummy network driver"
+	depends on NET
+	help
+	  This is the simpler version of drivers/net/dummy.c.
+	  No intention to replace it. This provides educational information
+	  for Rust abstractions for network device drivers.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_net_dummy.
+
+	  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..78f8f3321fd2
--- /dev/null
+++ b/samples/rust/rust_net_dummy.rs
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+//! Rust dummy netdev.
+
+//use kernel::types::ForeignOwnable;
+use kernel::{
+    c_str,
+    net::dev::{
+        flags, priv_flags, Device, DeviceOperations, EtherOperations, EthtoolTsInfo, Registration,
+        SkBuff, TxCode,
+    },
+    prelude::*,
+};
+
+module! {
+    type: DummyNetdev,
+    name: "rust_net_dummy",
+    author: "Rust for Linux Contributors",
+    description: "Rust dummy netdev",
+    license: "GPL v2",
+}
+
+#[vtable]
+impl DeviceOperations for Box<DriverData> {
+    fn init(dev: Device<Box<DriverData>>) -> Result {
+        // how to access to the driver private data.
+        let _ = dev.drv_priv_data();
+        Ok(())
+    }
+
+    fn start_xmit(_dev: Device<Box<DriverData>>, mut skb: SkBuff) -> TxCode {
+        skb.tx_timestamp();
+        skb.consume();
+        TxCode::Ok
+    }
+}
+
+/// For device driver specific information.
+struct DriverData {}
+
+struct DummyNetdev {
+    _r: Registration<Box<DriverData>>,
+}
+
+#[vtable]
+impl EtherOperations for Box<DriverData> {
+    fn get_ts_info(dev: Device<Box<DriverData>>, mut info: EthtoolTsInfo) -> Result {
+        EthtoolTsInfo::ethtool_op_get_ts_info(&dev, &mut info)
+    }
+}
+
+impl kernel::Module for DummyNetdev {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        let data = Box::try_new(DriverData {})?;
+        let mut r = Registration::<Box<DriverData>>::try_new_ether(1, 1, data)?;
+        r.set_ether_operations::<Box<DriverData>>()?;
+
+        let netdev = r.dev_get();
+        netdev.set_name(c_str!("dummy%d"))?;
+
+        netdev.set_flags(netdev.get_flags() | flags::IFF_NOARP & !flags::IFF_MULTICAST);
+        netdev.set_priv_flags(
+            netdev.get_priv_flags() | priv_flags::IFF_LIVE_ADDR_CHANGE | priv_flags::IFF_NO_QUEUE,
+        );
+        netdev.set_random_eth_hw_addr();
+        netdev.set_min_mtu(0);
+        netdev.set_max_mtu(0);
+
+        r.register()?;
+
+        // TODO: Replaces pr_info with the wrapper of netdev_info().
+        pr_info!("Hello Rust dummy netdev!");
+        Ok(DummyNetdev { _r: r })
+    }
+}
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 78175231c969..1404967e908e 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -277,7 +277,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
 # Compile Rust sources (.rs)
 # ---------------------------------------------------------------------------
 
-rust_allowed_features := new_uninit
+rust_allowed_features := allocator_api,new_uninit
 
 rust_common_cmd = \
 	RUST_MODFILE=$(modfile) $(RUSTC_OR_CLIPPY) $(rust_flags) \
-- 
2.34.1

[PATCH v2 5/5] MAINTAINERS: add Rust network abstractions files to the NETWORKING DRIVERS entry

[FUJITA Tomonori]

The files are placed at rust/kernel/ directory for now but the files
are likely to be moved to net/ directory if things go well.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 250518fc70ff..66b8e43b05a2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14581,6 +14581,8 @@ F:	include/linux/inetdevice.h
 F:	include/linux/netdevice.h
 F:	include/uapi/linux/if_*
 F:	include/uapi/linux/netdevice.h
+F:	rust/kernel/net.rs
+F:	rust/kernel/net/
 
 NETWORKING DRIVERS (WIRELESS)
 M:	Kalle Valo <kvalo@kernel.org>
-- 
2.34.1

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

[Jakub Kicinski]

On Mon, 10 Jul 2023 16:36:58 +0900 FUJITA Tomonori wrote:
> This patchset adds minimum Rust abstractions for network device
> drivers and an example of a Rust network device driver, a simpler
> version of drivers/net/dummy.c.
> 
> The major change is a way to drop an skb (1/5 patch); a driver needs
> to explicitly call a function to drop a skb. The code to let a skb
> go out of scope can't be compiled.
> 
> I dropped get_stats64 support patch that the current sample driver
> doesn't use. Instead I added a patch to update the NETWORKING DRIVERS
> entry in MAINTAINERS.

I'd like to double down on my suggestion to try to implement a real
PHY driver. Most of the bindings in patch 3 will never be used by
drivers. (Re)implementing a real driver will guide you towards useful
stuff and real problems.

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

[Greg KH]

On Mon, Jul 10, 2023 at 11:29:52AM -0700, Jakub Kicinski wrote:
> On Mon, 10 Jul 2023 16:36:58 +0900 FUJITA Tomonori wrote:
> > This patchset adds minimum Rust abstractions for network device
> > drivers and an example of a Rust network device driver, a simpler
> > version of drivers/net/dummy.c.
> > 
> > The major change is a way to drop an skb (1/5 patch); a driver needs
> > to explicitly call a function to drop a skb. The code to let a skb
> > go out of scope can't be compiled.
> > 
> > I dropped get_stats64 support patch that the current sample driver
> > doesn't use. Instead I added a patch to update the NETWORKING DRIVERS
> > entry in MAINTAINERS.
> 
> I'd like to double down on my suggestion to try to implement a real
> PHY driver. Most of the bindings in patch 3 will never be used by
> drivers. (Re)implementing a real driver will guide you towards useful
> stuff and real problems.

And I'd recommend that we not take any more bindings without real users,
as there seems to be just a collection of these and it's hard to
actually review them to see how they are used...

thanks,

greg k-h

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

[FUJITA Tomonori]

Hi,

On Mon, 10 Jul 2023 11:29:52 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Mon, 10 Jul 2023 16:36:58 +0900 FUJITA Tomonori wrote:
>> This patchset adds minimum Rust abstractions for network device
>> drivers and an example of a Rust network device driver, a simpler
>> version of drivers/net/dummy.c.
>> 
>> The major change is a way to drop an skb (1/5 patch); a driver needs
>> to explicitly call a function to drop a skb. The code to let a skb
>> go out of scope can't be compiled.
>> 
>> I dropped get_stats64 support patch that the current sample driver
>> doesn't use. Instead I added a patch to update the NETWORKING DRIVERS
>> entry in MAINTAINERS.
> 
> I'd like to double down on my suggestion to try to implement a real
> PHY driver. Most of the bindings in patch 3 will never be used by
> drivers. (Re)implementing a real driver will guide you towards useful
> stuff and real problems.

You meant reimplementing one of drivers in drivers/net/phy in Rust
(with Rust abstractions for PHY drivers)?

Even the approach, we would get hit the same problem? Replacing an
existing working driver in C doesn't make sense much thus the
abstractions cannot be merged until someone want to implement a driver
in Rust for new PHY hardware.

Or you think that PHY drivers (and probably the abstractions) are
relatively simple so merging the abstractions for them is acceptable
without a real driver (we could put a real drivers under
samples/rust/)?

thanks,

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

[Andrew Lunn]

> Or you think that PHY drivers (and probably the abstractions) are
> relatively simple so merging the abstractions for them is acceptable
> without a real driver (we could put a real drivers under
> samples/rust/)?

PHY drivers are much simpler than Ethernet drivers. But more
importantly, the API to the rest of network stack is much much
smaller. So a binding for a sample driver is going to cover a large
part of that API, unlike your sample Ethernet driver binding which
covers a tiny part of the API. The PHY binding is then actually
useful, unlike the binding for Ethernet.

As for reimplementing an existing driver, vs a new driver for hardware
which is currently unsupported, that would depend on you. You could
reach out to some vendors and see if they have devices which are
missing mainline drivers. See if they will donate an RDK and the
datasheet in return for a free driver written in Rust. Whole new
drivers do come along reasonably frequently, so there is probably
scope for a new driver. Automotive is a big source of new code and
devices at the moment.

	Andrew

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

[FUJITA Tomonori]

Hi,

On Tue, 11 Jul 2023 15:17:33 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> Or you think that PHY drivers (and probably the abstractions) are
>> relatively simple so merging the abstractions for them is acceptable
>> without a real driver (we could put a real drivers under
>> samples/rust/)?
> 
> PHY drivers are much simpler than Ethernet drivers. But more
> importantly, the API to the rest of network stack is much much
> smaller. So a binding for a sample driver is going to cover a large
> part of that API, unlike your sample Ethernet driver binding which
> covers a tiny part of the API. The PHY binding is then actually
> useful, unlike the binding for Ethernet.

Point taken, I work on PHY drivers first.

> As for reimplementing an existing driver, vs a new driver for hardware
> which is currently unsupported, that would depend on you. You could
> reach out to some vendors and see if they have devices which are
> missing mainline drivers. See if they will donate an RDK and the
> datasheet in return for a free driver written in Rust. Whole new
> drivers do come along reasonably frequently, so there is probably
> scope for a new driver. Automotive is a big source of new code and
> devices at the moment.

Understood. Let me reseach the existing drivers to think about that.

Thanks,

[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

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 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,

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

[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

[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