| Message ID | 20230613045326.3938283-2-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | Rust abstractions for network device drivers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch, async |
On 6/13/23 06:53, FUJITA Tomonori wrote: > This patch adds very basic abstractions to implement network device > drivers, corresponds to the kernel's net_device and net_device_ops > structs with support for register_netdev/unregister_netdev functions. > > allows the const_maybe_uninit_zeroed feature for > core::mem::MaybeUinit::<T>::zeroed() in const function. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > --- > rust/bindings/bindings_helper.h | 2 + > rust/helpers.c | 16 ++ > rust/kernel/lib.rs | 3 + > rust/kernel/net.rs | 5 + > rust/kernel/net/dev.rs | 344 ++++++++++++++++++++++++++++++++ > 5 files changed, 370 insertions(+) > create mode 100644 rust/kernel/net.rs > create mode 100644 rust/kernel/net/dev.rs > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index 3e601ce2548d..468bf606f174 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -7,6 +7,8 @@ > */ > > #include <linux/errname.h> > +#include <linux/etherdevice.h> > +#include <linux/netdevice.h> > #include <linux/slab.h> > #include <linux/refcount.h> > #include <linux/wait.h> > diff --git a/rust/helpers.c b/rust/helpers.c > index bb594da56137..70d50767ff4e 100644 > --- a/rust/helpers.c > +++ b/rust/helpers.c > @@ -24,10 +24,26 @@ > #include <linux/errname.h> > #include <linux/refcount.h> > #include <linux/mutex.h> > +#include <linux/netdevice.h> > +#include <linux/skbuff.h> > #include <linux/spinlock.h> > #include <linux/sched/signal.h> > #include <linux/wait.h> > > +#ifdef CONFIG_NET > +void *rust_helper_netdev_priv(const struct net_device *dev) > +{ > + return netdev_priv(dev); > +} > +EXPORT_SYMBOL_GPL(rust_helper_netdev_priv); > + > +void rust_helper_skb_tx_timestamp(struct sk_buff *skb) > +{ > + skb_tx_timestamp(skb); > +} > +EXPORT_SYMBOL_GPL(rust_helper_skb_tx_timestamp); > +#endif > + > __noreturn void rust_helper_BUG(void) > { > BUG(); > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 85b261209977..fc7d048d359d 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -13,6 +13,7 @@ > > #![no_std] > #![feature(allocator_api)] > +#![feature(const_maybe_uninit_zeroed)] > #![feature(coerce_unsized)] > #![feature(dispatch_from_dyn)] > #![feature(new_uninit)] > @@ -34,6 +35,8 @@ > pub mod error; > pub mod init; > pub mod ioctl; > +#[cfg(CONFIG_NET)] > +pub mod net; > pub mod prelude; > pub mod print; > mod static_assert; > diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs > new file mode 100644 > index 000000000000..28fe8f398463 > --- /dev/null > +++ b/rust/kernel/net.rs > @@ -0,0 +1,5 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Networking core. > + > +pub mod dev; > diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs > new file mode 100644 > index 000000000000..d072c81f99ce > --- /dev/null > +++ b/rust/kernel/net/dev.rs > @@ -0,0 +1,344 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Network device. > +//! > +//! C headers: [`include/linux/etherdevice.h`](../../../../include/linux/etherdevice.h), > +//! [`include/linux/ethtool.h`](../../../../include/linux/ethtool.h), > +//! [`include/linux/netdevice.h`](../../../../include/linux/netdevice.h), > +//! [`include/linux/skbuff.h`](../../../../include/linux/skbuff.h), > +//! [`include/uapi/linux/if_link.h`](../../../../include/uapi/linux/if_link.h). > + > +use crate::{bindings, error::*, prelude::vtable, types::ForeignOwnable}; > +use {core::ffi::c_void, core::marker::PhantomData}; > + > +/// Corresponds to the kernel's `struct net_device`. > +/// > +/// # Invariants > +/// > +/// The pointer is valid. > +pub struct Device(*mut bindings::net_device); > + > +impl Device { > + /// Creates a new [`Device`] instance. > + /// > + /// # Safety > + /// > + /// Callers must ensure that `ptr` must be valid. > + unsafe fn from_ptr(ptr: *mut bindings::net_device) -> Self { > + // INVARIANT: The safety requirements ensure the invariant. > + Self(ptr) > + } > + > + /// Gets a pointer to network device private data. > + fn priv_data_ptr(&self) -> *const c_void { > + // SAFETY: The type invariants guarantee that `self.0` is valid. > + // During the initialization of `Registration` instance, the kernel allocates > + // contiguous memory for `struct net_device` and a pointer to its private data. > + // So it's safe to read an address from the returned address from `netdev_priv()`. > + unsafe { core::ptr::read(bindings::netdev_priv(self.0) as *const *const c_void) } Why are at least `size_of::<*const c_void>` bytes allocated? Why is it a `*const c_void` pointer? This function does not give any guarantees about this pointer, is it valid? I know that you are allocating exactly this amount in `Registration`, but `Device` does not know about that. Should this be a type invariant? It might be a good idea to make `Driver` generic over `D`, the data that is stored behind this pointer. You could then return `D::Borrowed` instead. > + } > +} > + > +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used > +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync` > +// so it's safe to sharing its pointer. > +unsafe impl Send for Device {} > +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used > +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync`, > +// can be used from any thread too. > +unsafe impl Sync for Device {} > + > +/// Trait for device driver specific information. > +/// > +/// This data structure is passed to a driver with the operations for `struct net_device` > +/// like `struct net_device_ops`, `struct ethtool_ops`, `struct rtnl_link_ops`, etc. > +pub trait DriverData { > + /// The object are stored in C object, `struct net_device`. > + type Data: ForeignOwnable + Send + Sync; Why is this an associated type? Could you not use `D: ForeignOwnable + Send + Sync` everywhere instead? I think this should be possible, since `DriverData` does not define anything else. > +} > + > +/// Registration structure for a network device driver. > +/// > +/// This allocates and owns a `struct net_device` object. > +/// Once the `net_device` object is registered via `register_netdev` function, > +/// the kernel calls various functions such as `struct net_device_ops` operations with > +/// the `net_device` object. > +/// > +/// A driver must implement `struct net_device_ops` so the trait for it is tied. > +/// Other operations like `struct ethtool_ops` are optional. > +pub struct Registration<T: DeviceOperations<D>, D: DriverData> { > + dev: Device, > + is_registered: bool, > + _p: PhantomData<(D, T)>, > +} > + > +impl<D: DriverData, T: DeviceOperations<D>> Drop for Registration<T, D> { > + fn drop(&mut self) { > + // SAFETY: The type invariants guarantee that `self.dev.0` is valid. > + unsafe { > + let _ = D::Data::from_foreign(self.dev.priv_data_ptr()); Why is `self.dev.priv_data_ptr()` a valid pointer? This `unsafe` block should be split to better explain the different safety requirements. > + if self.is_registered { > + bindings::unregister_netdev(self.dev.0); > + } > + bindings::free_netdev(self.dev.0); > + } > + } > +} > + > +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> { > + /// Creates a new [`Registration`] instance for ethernet device. > + /// > + /// A device driver can pass private data. > + pub fn try_new_ether(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> { > + // SAFETY: FFI call. If this FFI call has no safety requirements then say so. > + let ptr = from_err_ptr(unsafe { > + bindings::alloc_etherdev_mqs( > + core::mem::size_of::<*const c_void>() as i32, > + tx_queue_size, > + rx_queue_size, > + ) > + })?; > + > + // SAFETY: `ptr` is valid and non-null since `alloc_etherdev_mqs()` > + // returned a valid pointer which was null-checked. > + let dev = unsafe { Device::from_ptr(ptr) }; > + // SAFETY: It's safe to write an address to the returned pointer > + // from `netdev_priv()` because `alloc_etherdev_mqs()` allocates > + // contiguous memory for `struct net_device` and a pointer. > + unsafe { > + let priv_ptr = bindings::netdev_priv(ptr) as *mut *const c_void; > + core::ptr::write(priv_ptr, data.into_foreign()); > + } > + Ok(Registration { > + dev, > + is_registered: false, > + _p: PhantomData, > + }) > + } > + > + /// Returns a network device. > + /// > + /// A device driver normally configures the device before registration. > + pub fn dev_get(&mut self) -> &mut Device { > + &mut self.dev > + } > + > + /// Registers a network device. > + pub fn register(&mut self) -> Result { > + if self.is_registered { > + return Err(code::EINVAL); > + } > + // SAFETY: The type invariants guarantee that `self.dev.0` is valid. > + let ret = unsafe { > + (*self.dev.0).netdev_ops = Self::build_device_ops(); > + bindings::register_netdev(self.dev.0) > + }; > + if ret != 0 { > + Err(Error::from_errno(ret)) > + } else { > + self.is_registered = true; > + Ok(()) > + } > + } > + > + const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops { > + ndo_init: if <T>::HAS_INIT { > + Some(Self::init_callback) > + } else { > + None > + }, > + ndo_uninit: if <T>::HAS_UNINIT { > + Some(Self::uninit_callback) > + } else { > + None > + }, > + ndo_open: if <T>::HAS_OPEN { > + Some(Self::open_callback) > + } else { > + None > + }, > + ndo_stop: if <T>::HAS_STOP { > + Some(Self::stop_callback) > + } else { > + None > + }, > + ndo_start_xmit: if <T>::HAS_START_XMIT { > + Some(Self::start_xmit_callback) > + } else { > + None > + }, > + // SAFETY: The rest is zeroed out to initialize `struct net_device_ops`, > + // set `Option<&F>` to be `None`. > + ..unsafe { core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() } > + }; > + > + const fn build_device_ops() -> &'static bindings::net_device_ops { > + &Self::DEVICE_OPS > + } Why does this function exist? > + > + unsafe extern "C" fn init_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { > + from_result(|| { Since you are the first user of `from_result`, you can remove the `#[allow(dead_code)]` attribute. @Reviewers/Maintainers: Or would we prefer to make that change ourselves? > + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. > + let mut dev = unsafe { Device::from_ptr(netdev) }; > + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when > + // `Registration` object was created. > + // `D::Data::from_foreign` is only called by the object was released. > + // So we know `data` is valid while this function is running. This should be a type invariant of `Registration`. > + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; > + T::init(&mut dev, data)?; > + Ok(0) > + }) > + } > + > + unsafe extern "C" fn uninit_callback(netdev: *mut bindings::net_device) { > + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. > + let mut dev = unsafe { Device::from_ptr(netdev) }; > + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when > + // `Registration` object was created. > + // `D::Data::from_foreign` is only called by the object was released. > + // So we know `data` is valid while this function is running. > + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; > + T::uninit(&mut dev, data); > + } > + > + unsafe extern "C" fn open_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { > + from_result(|| { > + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. > + let mut dev = unsafe { Device::from_ptr(netdev) }; > + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when > + // `Registration` object was created. > + // `D::Data::from_foreign` is only called by the object was released. > + // So we know `data` is valid while this function is running. > + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; > + T::open(&mut dev, data)?; > + Ok(0) > + }) > + } > + > + unsafe extern "C" fn stop_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { > + from_result(|| { > + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. > + let mut dev = unsafe { Device::from_ptr(netdev) }; > + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when > + // `Registration` object was created. > + // `D::Data::from_foreign` is only called by the object was released. > + // So we know `data` is valid while this function is running. > + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; > + T::stop(&mut dev, data)?; > + Ok(0) > + }) > + } > + > + unsafe extern "C" fn start_xmit_callback( > + skb: *mut bindings::sk_buff, > + netdev: *mut bindings::net_device, > + ) -> bindings::netdev_tx_t { > + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. > + let mut dev = unsafe { Device::from_ptr(netdev) }; > + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when > + // `Registration` object was created. > + // `D::Data::from_foreign` is only called by the object was released. > + // So we know `data` is valid while this function is running. > + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; > + // SAFETY: The C API guarantees that `skb` is valid while this function is running. > + let skb = unsafe { SkBuff::from_ptr(skb) }; > + T::start_xmit(&mut dev, data, skb) as bindings::netdev_tx_t > + } > +} > + > +// SAFETY: `Registration` exposes only `Device` object which can be used from > +// any thread. > +unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {} > +// SAFETY: `Registration` exposes only `Device` object which can be used from > +// any thread. > +unsafe impl<D: DriverData, T: DeviceOperations<D>> Sync for Registration<T, D> {} > + > +/// Corresponds to the kernel's `enum netdev_tx`. > +#[repr(i32)] > +pub enum TxCode { > + /// Driver took care of packet. > + Ok = bindings::netdev_tx_NETDEV_TX_OK, > + /// Driver tx path was busy. > + Busy = bindings::netdev_tx_NETDEV_TX_BUSY, > +} > + > +/// Corresponds to the kernel's `struct net_device_ops`. > +/// > +/// A device driver must implement this. Only very basic operations are supported for now. > +#[vtable] > +pub trait DeviceOperations<D: DriverData> { Why is this trait generic over `D`? Why is this not `Self` or an associated type? > + /// Corresponds to `ndo_init` in `struct net_device_ops`. > + fn init(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result { Why do all of these functions take a `&mut Device`? `Device` already is a pointer, so why the double indirection? > + Ok(()) > + } > + > + /// Corresponds to `ndo_uninit` in `struct net_device_ops`. > + fn uninit(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) {} > + > + /// Corresponds to `ndo_open` in `struct net_device_ops`. > + fn open(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result { > + Ok(()) > + } > + > + /// Corresponds to `ndo_stop` in `struct net_device_ops`. > + fn stop(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result { > + Ok(()) > + } > + > + /// Corresponds to `ndo_start_xmit` in `struct net_device_ops`. > + fn start_xmit( > + _dev: &mut Device, > + _data: <D::Data as ForeignOwnable>::Borrowed<'_>, > + _skb: SkBuff, > + ) -> TxCode { > + TxCode::Busy > + } > +} > + > +/// Corresponds to the kernel's `struct sk_buff`. > +/// > +/// A driver manages `struct sk_buff` in two ways. In both ways, the ownership is transferred > +/// between C and Rust. The allocation and release are done asymmetrically. > +/// > +/// On the tx side (`ndo_start_xmit` operation in `struct net_device_ops`), the kernel allocates > +/// a `sk_buff' object and passes it to the driver. The driver is responsible for the release > +/// after transmission. > +/// On the rx side, the driver allocates a `sk_buff` object then passes it to the kernel > +/// after receiving data. > +/// > +/// # Invariants > +/// > +/// The pointer is valid. > +pub struct SkBuff(*mut bindings::sk_buff); > + > +impl SkBuff { > + /// Creates a new [`SkBuff`] instance. > + /// > + /// # Safety > + /// > + /// Callers must ensure that `ptr` must be valid. > + unsafe fn from_ptr(ptr: *mut bindings::sk_buff) -> Self { > + // INVARIANT: The safety requirements ensure the invariant. > + Self(ptr) > + } > + > + /// Provides a time stamp. > + pub fn tx_timestamp(&mut self) { > + // SAFETY: The type invariants guarantee that `self.0` is valid. > + unsafe { > + bindings::skb_tx_timestamp(self.0); > + } > + } > +} > + > +impl Drop for SkBuff { > + fn drop(&mut self) { > + // SAFETY: The type invariants guarantee that `self.0` is valid. > + unsafe { > + bindings::kfree_skb_reason( > + self.0, > + bindings::skb_drop_reason_SKB_DROP_REASON_NOT_SPECIFIED, > + ) AFAICT this function frees the `struct sk_buff`, why is this safe? This function also has as a requirement that all other pointers to this struct are never used again. How do you guarantee this? You mentioned above that there are two us cases for an SkBuff, in one case the kernel frees it and in another the driver. How do we know that we can free it here? -- Cheers, Benno > + } > + } > +} > -- > 2.34.1 >
Hi, Thanks for reviewing. On Thu, 15 Jun 2023 13:01:50 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 6/13/23 06:53, FUJITA Tomonori wrote: >> This patch adds very basic abstractions to implement network device >> drivers, corresponds to the kernel's net_device and net_device_ops >> structs with support for register_netdev/unregister_netdev functions. >> >> allows the const_maybe_uninit_zeroed feature for >> core::mem::MaybeUinit::<T>::zeroed() in const function. >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >> --- >> rust/bindings/bindings_helper.h | 2 + >> rust/helpers.c | 16 ++ >> rust/kernel/lib.rs | 3 + >> rust/kernel/net.rs | 5 + >> rust/kernel/net/dev.rs | 344 ++++++++++++++++++++++++++++++++ >> 5 files changed, 370 insertions(+) >> create mode 100644 rust/kernel/net.rs >> create mode 100644 rust/kernel/net/dev.rs >> >> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h >> index 3e601ce2548d..468bf606f174 100644 >> --- a/rust/bindings/bindings_helper.h >> +++ b/rust/bindings/bindings_helper.h >> @@ -7,6 +7,8 @@ >> */ >> >> #include <linux/errname.h> >> +#include <linux/etherdevice.h> >> +#include <linux/netdevice.h> >> #include <linux/slab.h> >> #include <linux/refcount.h> >> #include <linux/wait.h> >> diff --git a/rust/helpers.c b/rust/helpers.c >> index bb594da56137..70d50767ff4e 100644 >> --- a/rust/helpers.c >> +++ b/rust/helpers.c >> @@ -24,10 +24,26 @@ >> #include <linux/errname.h> >> #include <linux/refcount.h> >> #include <linux/mutex.h> >> +#include <linux/netdevice.h> >> +#include <linux/skbuff.h> >> #include <linux/spinlock.h> >> #include <linux/sched/signal.h> >> #include <linux/wait.h> >> >> +#ifdef CONFIG_NET >> +void *rust_helper_netdev_priv(const struct net_device *dev) >> +{ >> + return netdev_priv(dev); >> +} >> +EXPORT_SYMBOL_GPL(rust_helper_netdev_priv); >> + >> +void rust_helper_skb_tx_timestamp(struct sk_buff *skb) >> +{ >> + skb_tx_timestamp(skb); >> +} >> +EXPORT_SYMBOL_GPL(rust_helper_skb_tx_timestamp); >> +#endif >> + >> __noreturn void rust_helper_BUG(void) >> { >> BUG(); >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs >> index 85b261209977..fc7d048d359d 100644 >> --- a/rust/kernel/lib.rs >> +++ b/rust/kernel/lib.rs >> @@ -13,6 +13,7 @@ >> >> #![no_std] >> #![feature(allocator_api)] >> +#![feature(const_maybe_uninit_zeroed)] >> #![feature(coerce_unsized)] >> #![feature(dispatch_from_dyn)] >> #![feature(new_uninit)] >> @@ -34,6 +35,8 @@ >> pub mod error; >> pub mod init; >> pub mod ioctl; >> +#[cfg(CONFIG_NET)] >> +pub mod net; >> pub mod prelude; >> pub mod print; >> mod static_assert; >> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs >> new file mode 100644 >> index 000000000000..28fe8f398463 >> --- /dev/null >> +++ b/rust/kernel/net.rs >> @@ -0,0 +1,5 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +//! Networking core. >> + >> +pub mod dev; >> diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs >> new file mode 100644 >> index 000000000000..d072c81f99ce >> --- /dev/null >> +++ b/rust/kernel/net/dev.rs >> @@ -0,0 +1,344 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +//! Network device. >> +//! >> +//! C headers: [`include/linux/etherdevice.h`](../../../../include/linux/etherdevice.h), >> +//! [`include/linux/ethtool.h`](../../../../include/linux/ethtool.h), >> +//! [`include/linux/netdevice.h`](../../../../include/linux/netdevice.h), >> +//! [`include/linux/skbuff.h`](../../../../include/linux/skbuff.h), >> +//! [`include/uapi/linux/if_link.h`](../../../../include/uapi/linux/if_link.h). >> + >> +use crate::{bindings, error::*, prelude::vtable, types::ForeignOwnable}; >> +use {core::ffi::c_void, core::marker::PhantomData}; >> + >> +/// Corresponds to the kernel's `struct net_device`. >> +/// >> +/// # Invariants >> +/// >> +/// The pointer is valid. >> +pub struct Device(*mut bindings::net_device); >> + >> +impl Device { >> + /// Creates a new [`Device`] instance. >> + /// >> + /// # Safety >> + /// >> + /// Callers must ensure that `ptr` must be valid. >> + unsafe fn from_ptr(ptr: *mut bindings::net_device) -> Self { >> + // INVARIANT: The safety requirements ensure the invariant. >> + Self(ptr) >> + } >> + >> + /// Gets a pointer to network device private data. >> + fn priv_data_ptr(&self) -> *const c_void { >> + // SAFETY: The type invariants guarantee that `self.0` is valid. >> + // During the initialization of `Registration` instance, the kernel allocates >> + // contiguous memory for `struct net_device` and a pointer to its private data. >> + // So it's safe to read an address from the returned address from `netdev_priv()`. >> + unsafe { core::ptr::read(bindings::netdev_priv(self.0) as *const *const c_void) } > > Why are at least `size_of::<*const c_void>` bytes allocated? Why is it a > `*const c_void` pointer? This function does not give any guarantees about > this pointer, is it valid? The reason is a device driver needs its data structure. It needs to access to it via a pointer to bindings::net_device struct. The space for the pointer is allocated during initialization of Registration and it's valid until the Registration object is dropped. > I know that you are allocating exactly this amount in `Registration`, but > `Device` does not know about that. Should this be a type invariant? > It might be a good idea to make `Driver` generic over `D`, the data that is > stored behind this pointer. You could then return `D::Borrowed` instead. We could do: impl<D: DriverData> Device<D> { ... /// Gets the private data of a device driver. pub fn drv_priv_data(&self) -> <D::Data as ForeignOwnable>::Borrowed<'_> { unsafe { D::Data::borrow(core::ptr::read( bindings::netdev_priv(self.ptr) as *const *const c_void )) } } } >> +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used >> +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync` >> +// so it's safe to sharing its pointer. >> +unsafe impl Send for Device {} >> +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used >> +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync`, >> +// can be used from any thread too. >> +unsafe impl Sync for Device {} >> + >> +/// Trait for device driver specific information. >> +/// >> +/// This data structure is passed to a driver with the operations for `struct net_device` >> +/// like `struct net_device_ops`, `struct ethtool_ops`, `struct rtnl_link_ops`, etc. >> +pub trait DriverData { >> + /// The object are stored in C object, `struct net_device`. >> + type Data: ForeignOwnable + Send + Sync; > > Why is this an associated type? Could you not use > `D: ForeignOwnable + Send + Sync` everywhere instead? > I think this should be possible, since `DriverData` does not define > anything else. With that approach, is it possible to allow a device driver to define own data structure and functions taking the structure as aurgument (like DevOps structutre in the 5th patch) >> +/// Registration structure for a network device driver. >> +/// >> +/// This allocates and owns a `struct net_device` object. >> +/// Once the `net_device` object is registered via `register_netdev` function, >> +/// the kernel calls various functions such as `struct net_device_ops` operations with >> +/// the `net_device` object. >> +/// >> +/// A driver must implement `struct net_device_ops` so the trait for it is tied. >> +/// Other operations like `struct ethtool_ops` are optional. >> +pub struct Registration<T: DeviceOperations<D>, D: DriverData> { >> + dev: Device, >> + is_registered: bool, >> + _p: PhantomData<(D, T)>, >> +} >> + >> +impl<D: DriverData, T: DeviceOperations<D>> Drop for Registration<T, D> { >> + fn drop(&mut self) { >> + // SAFETY: The type invariants guarantee that `self.dev.0` is valid. >> + unsafe { >> + let _ = D::Data::from_foreign(self.dev.priv_data_ptr()); > > Why is `self.dev.priv_data_ptr()` a valid pointer? > This `unsafe` block should be split to better explain the different safety > requirements. Explained above. >> + if self.is_registered { >> + bindings::unregister_netdev(self.dev.0); >> + } >> + bindings::free_netdev(self.dev.0); >> + } >> + } >> +} >> + >> +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> { >> + /// Creates a new [`Registration`] instance for ethernet device. >> + /// >> + /// A device driver can pass private data. >> + pub fn try_new_ether(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> { >> + // SAFETY: FFI call. > > If this FFI call has no safety requirements then say so. SAFETY: FFI call has no safety requirements. ? >> + const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops { >> + ndo_init: if <T>::HAS_INIT { >> + Some(Self::init_callback) >> + } else { >> + None >> + }, >> + ndo_uninit: if <T>::HAS_UNINIT { >> + Some(Self::uninit_callback) >> + } else { >> + None >> + }, >> + ndo_open: if <T>::HAS_OPEN { >> + Some(Self::open_callback) >> + } else { >> + None >> + }, >> + ndo_stop: if <T>::HAS_STOP { >> + Some(Self::stop_callback) >> + } else { >> + None >> + }, >> + ndo_start_xmit: if <T>::HAS_START_XMIT { >> + Some(Self::start_xmit_callback) >> + } else { >> + None >> + }, >> + // SAFETY: The rest is zeroed out to initialize `struct net_device_ops`, >> + // set `Option<&F>` to be `None`. >> + ..unsafe { core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() } >> + }; >> + >> + const fn build_device_ops() -> &'static bindings::net_device_ops { >> + &Self::DEVICE_OPS >> + } > > Why does this function exist? To get const struct net_device_ops *netdev_ops. >> + >> + unsafe extern "C" fn init_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { >> + from_result(|| { > > Since you are the first user of `from_result`, you can remove the > `#[allow(dead_code)]` attribute. > > @Reviewers/Maintainers: Or would we prefer to make that change ourselves? Ah, either is fine by me. >> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >> + let mut dev = unsafe { Device::from_ptr(netdev) }; >> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >> + // `Registration` object was created. >> + // `D::Data::from_foreign` is only called by the object was released. >> + // So we know `data` is valid while this function is running. > > This should be a type invariant of `Registration`. Understood. >> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >> + T::init(&mut dev, data)?; >> + Ok(0) >> + }) >> + } >> + >> + unsafe extern "C" fn uninit_callback(netdev: *mut bindings::net_device) { >> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >> + let mut dev = unsafe { Device::from_ptr(netdev) }; >> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >> + // `Registration` object was created. >> + // `D::Data::from_foreign` is only called by the object was released. >> + // So we know `data` is valid while this function is running. >> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >> + T::uninit(&mut dev, data); >> + } >> + >> + unsafe extern "C" fn open_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { >> + from_result(|| { >> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >> + let mut dev = unsafe { Device::from_ptr(netdev) }; >> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >> + // `Registration` object was created. >> + // `D::Data::from_foreign` is only called by the object was released. >> + // So we know `data` is valid while this function is running. >> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >> + T::open(&mut dev, data)?; >> + Ok(0) >> + }) >> + } >> + >> + unsafe extern "C" fn stop_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { >> + from_result(|| { >> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >> + let mut dev = unsafe { Device::from_ptr(netdev) }; >> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >> + // `Registration` object was created. >> + // `D::Data::from_foreign` is only called by the object was released. >> + // So we know `data` is valid while this function is running. >> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >> + T::stop(&mut dev, data)?; >> + Ok(0) >> + }) >> + } >> + >> + unsafe extern "C" fn start_xmit_callback( >> + skb: *mut bindings::sk_buff, >> + netdev: *mut bindings::net_device, >> + ) -> bindings::netdev_tx_t { >> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >> + let mut dev = unsafe { Device::from_ptr(netdev) }; >> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >> + // `Registration` object was created. >> + // `D::Data::from_foreign` is only called by the object was released. >> + // So we know `data` is valid while this function is running. >> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >> + // SAFETY: The C API guarantees that `skb` is valid while this function is running. >> + let skb = unsafe { SkBuff::from_ptr(skb) }; >> + T::start_xmit(&mut dev, data, skb) as bindings::netdev_tx_t >> + } >> +} >> + >> +// SAFETY: `Registration` exposes only `Device` object which can be used from >> +// any thread. >> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {} >> +// SAFETY: `Registration` exposes only `Device` object which can be used from >> +// any thread. >> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Sync for Registration<T, D> {} >> + >> +/// Corresponds to the kernel's `enum netdev_tx`. >> +#[repr(i32)] >> +pub enum TxCode { >> + /// Driver took care of packet. >> + Ok = bindings::netdev_tx_NETDEV_TX_OK, >> + /// Driver tx path was busy. >> + Busy = bindings::netdev_tx_NETDEV_TX_BUSY, >> +} >> + >> +/// Corresponds to the kernel's `struct net_device_ops`. >> +/// >> +/// A device driver must implement this. Only very basic operations are supported for now. >> +#[vtable] >> +pub trait DeviceOperations<D: DriverData> { > > Why is this trait generic over `D`? Why is this not `Self` or an associated > type? DriverData also used in EtherOperationsAdapter (the second patch) and there are other operations that uses DriverData (not in this patchset). >> + /// Corresponds to `ndo_init` in `struct net_device_ops`. >> + fn init(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result { > > Why do all of these functions take a `&mut Device`? `Device` already is a > pointer, so why the double indirection? I guess that I follow the existing code like https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/amba.rs >> +/// Corresponds to the kernel's `struct sk_buff`. >> +/// >> +/// A driver manages `struct sk_buff` in two ways. In both ways, the ownership is transferred >> +/// between C and Rust. The allocation and release are done asymmetrically. >> +/// >> +/// On the tx side (`ndo_start_xmit` operation in `struct net_device_ops`), the kernel allocates >> +/// a `sk_buff' object and passes it to the driver. The driver is responsible for the release >> +/// after transmission. >> +/// On the rx side, the driver allocates a `sk_buff` object then passes it to the kernel >> +/// after receiving data. >> +/// >> +/// # Invariants >> +/// >> +/// The pointer is valid. >> +pub struct SkBuff(*mut bindings::sk_buff); >> + >> +impl SkBuff { >> + /// Creates a new [`SkBuff`] instance. >> + /// >> + /// # Safety >> + /// >> + /// Callers must ensure that `ptr` must be valid. >> + unsafe fn from_ptr(ptr: *mut bindings::sk_buff) -> Self { >> + // INVARIANT: The safety requirements ensure the invariant. >> + Self(ptr) >> + } >> + >> + /// Provides a time stamp. >> + pub fn tx_timestamp(&mut self) { >> + // SAFETY: The type invariants guarantee that `self.0` is valid. >> + unsafe { >> + bindings::skb_tx_timestamp(self.0); >> + } >> + } >> +} >> + >> +impl Drop for SkBuff { >> + fn drop(&mut self) { >> + // SAFETY: The type invariants guarantee that `self.0` is valid. >> + unsafe { >> + bindings::kfree_skb_reason( >> + self.0, >> + bindings::skb_drop_reason_SKB_DROP_REASON_NOT_SPECIFIED, >> + ) > > AFAICT this function frees the `struct sk_buff`, why is this safe? This > function also has as a requirement that all other pointers to this struct > are never used again. How do you guarantee this? > You mentioned above that there are two us cases for an SkBuff, in one case > the kernel frees it and in another the driver. How do we know that we can > free it here? This can handle only the tx case. As you can see, we had a good discussion on this and seems that found a solution. It'll be fixed.
On Tue, Jun 13, 2023 at 01:53:22PM +0900, FUJITA Tomonori wrote: > This patch adds very basic abstractions to implement network device > drivers, corresponds to the kernel's net_device and net_device_ops > structs with support for register_netdev/unregister_netdev functions. > > allows the const_maybe_uninit_zeroed feature for > core::mem::MaybeUinit::<T>::zeroed() in const function. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > --- > rust/bindings/bindings_helper.h | 2 + > rust/helpers.c | 16 ++ > rust/kernel/lib.rs | 3 + > rust/kernel/net.rs | 5 + > rust/kernel/net/dev.rs | 344 ++++++++++++++++++++++++++++++++ > 5 files changed, 370 insertions(+) > create mode 100644 rust/kernel/net.rs > create mode 100644 rust/kernel/net/dev.rs > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index 3e601ce2548d..468bf606f174 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -7,6 +7,8 @@ > */ > > #include <linux/errname.h> > +#include <linux/etherdevice.h> > +#include <linux/netdevice.h> > #include <linux/slab.h> > #include <linux/refcount.h> > #include <linux/wait.h> > diff --git a/rust/helpers.c b/rust/helpers.c > index bb594da56137..70d50767ff4e 100644 > --- a/rust/helpers.c > +++ b/rust/helpers.c > @@ -24,10 +24,26 @@ > #include <linux/errname.h> > #include <linux/refcount.h> > #include <linux/mutex.h> > +#include <linux/netdevice.h> > +#include <linux/skbuff.h> > #include <linux/spinlock.h> > #include <linux/sched/signal.h> > #include <linux/wait.h> > > +#ifdef CONFIG_NET > +void *rust_helper_netdev_priv(const struct net_device *dev) > +{ > + return netdev_priv(dev); > +} > +EXPORT_SYMBOL_GPL(rust_helper_netdev_priv); > + > +void rust_helper_skb_tx_timestamp(struct sk_buff *skb) > +{ > + skb_tx_timestamp(skb); > +} > +EXPORT_SYMBOL_GPL(rust_helper_skb_tx_timestamp); > +#endif > + > __noreturn void rust_helper_BUG(void) > { > BUG(); > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 85b261209977..fc7d048d359d 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -13,6 +13,7 @@ > > #![no_std] > #![feature(allocator_api)] > +#![feature(const_maybe_uninit_zeroed)] > #![feature(coerce_unsized)] > #![feature(dispatch_from_dyn)] > #![feature(new_uninit)] > @@ -34,6 +35,8 @@ > pub mod error; > pub mod init; > pub mod ioctl; > +#[cfg(CONFIG_NET)] > +pub mod net; > pub mod prelude; > pub mod print; > mod static_assert; > diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs > new file mode 100644 > index 000000000000..28fe8f398463 > --- /dev/null > +++ b/rust/kernel/net.rs > @@ -0,0 +1,5 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Networking core. > + > +pub mod dev; > diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs > new file mode 100644 > index 000000000000..d072c81f99ce > --- /dev/null > +++ b/rust/kernel/net/dev.rs > @@ -0,0 +1,344 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Network device. > +//! > +//! C headers: [`include/linux/etherdevice.h`](../../../../include/linux/etherdevice.h), > +//! [`include/linux/ethtool.h`](../../../../include/linux/ethtool.h), > +//! [`include/linux/netdevice.h`](../../../../include/linux/netdevice.h), > +//! [`include/linux/skbuff.h`](../../../../include/linux/skbuff.h), > +//! [`include/uapi/linux/if_link.h`](../../../../include/uapi/linux/if_link.h). > + > +use crate::{bindings, error::*, prelude::vtable, types::ForeignOwnable}; > +use {core::ffi::c_void, core::marker::PhantomData}; > + > +/// Corresponds to the kernel's `struct net_device`. > +/// > +/// # Invariants > +/// > +/// The pointer is valid. > +pub struct Device(*mut bindings::net_device); > + > +impl Device { > + /// Creates a new [`Device`] instance. > + /// > + /// # Safety > + /// > + /// Callers must ensure that `ptr` must be valid. > + unsafe fn from_ptr(ptr: *mut bindings::net_device) -> Self { > + // INVARIANT: The safety requirements ensure the invariant. > + Self(ptr) > + } > + > + /// Gets a pointer to network device private data. > + fn priv_data_ptr(&self) -> *const c_void { > + // SAFETY: The type invariants guarantee that `self.0` is valid. > + // During the initialization of `Registration` instance, the kernel allocates > + // contiguous memory for `struct net_device` and a pointer to its private data. > + // So it's safe to read an address from the returned address from `netdev_priv()`. > + unsafe { core::ptr::read(bindings::netdev_priv(self.0) as *const *const c_void) } > + } > +} > + > +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used > +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync` > +// so it's safe to sharing its pointer. > +unsafe impl Send for Device {} > +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used > +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync`, > +// can be used from any thread too. > +unsafe impl Sync for Device {} > + > +/// Trait for device driver specific information. > +/// > +/// This data structure is passed to a driver with the operations for `struct net_device` > +/// like `struct net_device_ops`, `struct ethtool_ops`, `struct rtnl_link_ops`, etc. > +pub trait DriverData { > + /// The object are stored in C object, `struct net_device`. > + type Data: ForeignOwnable + Send + Sync; > +} > + > +/// Registration structure for a network device driver. > +/// > +/// This allocates and owns a `struct net_device` object. > +/// Once the `net_device` object is registered via `register_netdev` function, > +/// the kernel calls various functions such as `struct net_device_ops` operations with > +/// the `net_device` object. > +/// > +/// A driver must implement `struct net_device_ops` so the trait for it is tied. > +/// Other operations like `struct ethtool_ops` are optional. > +pub struct Registration<T: DeviceOperations<D>, D: DriverData> { > + dev: Device, > + is_registered: bool, > + _p: PhantomData<(D, T)>, > +} > + > +impl<D: DriverData, T: DeviceOperations<D>> Drop for Registration<T, D> { > + fn drop(&mut self) { > + // SAFETY: The type invariants guarantee that `self.dev.0` is valid. > + unsafe { > + let _ = D::Data::from_foreign(self.dev.priv_data_ptr()); > + if self.is_registered { > + bindings::unregister_netdev(self.dev.0); > + } > + bindings::free_netdev(self.dev.0); > + } > + } > +} > + > +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> { > + /// Creates a new [`Registration`] instance for ethernet device. > + /// > + /// A device driver can pass private data. > + pub fn try_new_ether(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> { > + // SAFETY: FFI call. > + let ptr = from_err_ptr(unsafe { > + bindings::alloc_etherdev_mqs( > + core::mem::size_of::<*const c_void>() as i32, > + tx_queue_size, > + rx_queue_size, > + ) > + })?; > + > + // SAFETY: `ptr` is valid and non-null since `alloc_etherdev_mqs()` > + // returned a valid pointer which was null-checked. Hmm.. neither alloc_etherdev_mqs() nor `from_err_ptr` would do the null-check IIUC, so you may get a `ptr` whose values is null here. Regards, Boqun > + let dev = unsafe { Device::from_ptr(ptr) }; > + // SAFETY: It's safe to write an address to the returned pointer > + // from `netdev_priv()` because `alloc_etherdev_mqs()` allocates > + // contiguous memory for `struct net_device` and a pointer. > + unsafe { > + let priv_ptr = bindings::netdev_priv(ptr) as *mut *const c_void; > + core::ptr::write(priv_ptr, data.into_foreign()); > + } > + Ok(Registration { > + dev, > + is_registered: false, > + _p: PhantomData, > + }) > + } > + [...]
Hi, On Wed, 21 Jun 2023 15:44:42 -0700 Boqun Feng <boqun.feng@gmail.com> wrote: > On Tue, Jun 13, 2023 at 01:53:22PM +0900, FUJITA Tomonori wrote: >> +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> { >> + /// Creates a new [`Registration`] instance for ethernet device. >> + /// >> + /// A device driver can pass private data. >> + pub fn try_new_ether(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> { >> + // SAFETY: FFI call. >> + let ptr = from_err_ptr(unsafe { >> + bindings::alloc_etherdev_mqs( >> + core::mem::size_of::<*const c_void>() as i32, >> + tx_queue_size, >> + rx_queue_size, >> + ) >> + })?; >> + >> + // SAFETY: `ptr` is valid and non-null since `alloc_etherdev_mqs()` >> + // returned a valid pointer which was null-checked. > > Hmm.. neither alloc_etherdev_mqs() nor `from_err_ptr` would do the > null-check IIUC, so you may get a `ptr` whose values is null here. Oops, `ptr.is_null()` should be used here instead. Thanks a lot!
On 6/21/23 15:13, FUJITA Tomonori wrote: > Hi, > Thanks for reviewing. > > On Thu, 15 Jun 2023 13:01:50 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > >> On 6/13/23 06:53, FUJITA Tomonori wrote: >>> This patch adds very basic abstractions to implement network device >>> drivers, corresponds to the kernel's net_device and net_device_ops >>> structs with support for register_netdev/unregister_netdev functions. >>> >>> allows the const_maybe_uninit_zeroed feature for >>> core::mem::MaybeUinit::<T>::zeroed() in const function. >>> >>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >>> --- >>> rust/bindings/bindings_helper.h | 2 + >>> rust/helpers.c | 16 ++ >>> rust/kernel/lib.rs | 3 + >>> rust/kernel/net.rs | 5 + >>> rust/kernel/net/dev.rs | 344 ++++++++++++++++++++++++++++++++ >>> 5 files changed, 370 insertions(+) >>> create mode 100644 rust/kernel/net.rs >>> create mode 100644 rust/kernel/net/dev.rs >>> >>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h >>> index 3e601ce2548d..468bf606f174 100644 >>> --- a/rust/bindings/bindings_helper.h >>> +++ b/rust/bindings/bindings_helper.h >>> @@ -7,6 +7,8 @@ >>> */ >>> >>> #include <linux/errname.h> >>> +#include <linux/etherdevice.h> >>> +#include <linux/netdevice.h> >>> #include <linux/slab.h> >>> #include <linux/refcount.h> >>> #include <linux/wait.h> >>> diff --git a/rust/helpers.c b/rust/helpers.c >>> index bb594da56137..70d50767ff4e 100644 >>> --- a/rust/helpers.c >>> +++ b/rust/helpers.c >>> @@ -24,10 +24,26 @@ >>> #include <linux/errname.h> >>> #include <linux/refcount.h> >>> #include <linux/mutex.h> >>> +#include <linux/netdevice.h> >>> +#include <linux/skbuff.h> >>> #include <linux/spinlock.h> >>> #include <linux/sched/signal.h> >>> #include <linux/wait.h> >>> >>> +#ifdef CONFIG_NET >>> +void *rust_helper_netdev_priv(const struct net_device *dev) >>> +{ >>> + return netdev_priv(dev); >>> +} >>> +EXPORT_SYMBOL_GPL(rust_helper_netdev_priv); >>> + >>> +void rust_helper_skb_tx_timestamp(struct sk_buff *skb) >>> +{ >>> + skb_tx_timestamp(skb); >>> +} >>> +EXPORT_SYMBOL_GPL(rust_helper_skb_tx_timestamp); >>> +#endif >>> + >>> __noreturn void rust_helper_BUG(void) >>> { >>> BUG(); >>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs >>> index 85b261209977..fc7d048d359d 100644 >>> --- a/rust/kernel/lib.rs >>> +++ b/rust/kernel/lib.rs >>> @@ -13,6 +13,7 @@ >>> >>> #![no_std] >>> #![feature(allocator_api)] >>> +#![feature(const_maybe_uninit_zeroed)] >>> #![feature(coerce_unsized)] >>> #![feature(dispatch_from_dyn)] >>> #![feature(new_uninit)] >>> @@ -34,6 +35,8 @@ >>> pub mod error; >>> pub mod init; >>> pub mod ioctl; >>> +#[cfg(CONFIG_NET)] >>> +pub mod net; >>> pub mod prelude; >>> pub mod print; >>> mod static_assert; >>> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs >>> new file mode 100644 >>> index 000000000000..28fe8f398463 >>> --- /dev/null >>> +++ b/rust/kernel/net.rs >>> @@ -0,0 +1,5 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> + >>> +//! Networking core. >>> + >>> +pub mod dev; >>> diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs >>> new file mode 100644 >>> index 000000000000..d072c81f99ce >>> --- /dev/null >>> +++ b/rust/kernel/net/dev.rs >>> @@ -0,0 +1,344 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> + >>> +//! Network device. >>> +//! >>> +//! C headers: [`include/linux/etherdevice.h`](../../../../include/linux/etherdevice.h), >>> +//! [`include/linux/ethtool.h`](../../../../include/linux/ethtool.h), >>> +//! [`include/linux/netdevice.h`](../../../../include/linux/netdevice.h), >>> +//! [`include/linux/skbuff.h`](../../../../include/linux/skbuff.h), >>> +//! [`include/uapi/linux/if_link.h`](../../../../include/uapi/linux/if_link.h). >>> + >>> +use crate::{bindings, error::*, prelude::vtable, types::ForeignOwnable}; >>> +use {core::ffi::c_void, core::marker::PhantomData}; >>> + >>> +/// Corresponds to the kernel's `struct net_device`. >>> +/// >>> +/// # Invariants >>> +/// >>> +/// The pointer is valid. >>> +pub struct Device(*mut bindings::net_device); >>> + >>> +impl Device { >>> + /// Creates a new [`Device`] instance. >>> + /// >>> + /// # Safety >>> + /// >>> + /// Callers must ensure that `ptr` must be valid. >>> + unsafe fn from_ptr(ptr: *mut bindings::net_device) -> Self { >>> + // INVARIANT: The safety requirements ensure the invariant. >>> + Self(ptr) >>> + } >>> + >>> + /// Gets a pointer to network device private data. >>> + fn priv_data_ptr(&self) -> *const c_void { >>> + // SAFETY: The type invariants guarantee that `self.0` is valid. >>> + // During the initialization of `Registration` instance, the kernel allocates >>> + // contiguous memory for `struct net_device` and a pointer to its private data. >>> + // So it's safe to read an address from the returned address from `netdev_priv()`. >>> + unsafe { core::ptr::read(bindings::netdev_priv(self.0) as *const *const c_void) } >> >> Why are at least `size_of::<*const c_void>` bytes allocated? Why is it a >> `*const c_void` pointer? This function does not give any guarantees about >> this pointer, is it valid? > > The reason is a device driver needs its data structure. It needs to > access to it via a pointer to bindings::net_device struct. The space > for the pointer is allocated during initialization of Registration and > it's valid until the Registration object is dropped. I think this should be encoded in the type invariants of `Device`. Because the safety comment needs some justification. > >> I know that you are allocating exactly this amount in `Registration`, but >> `Device` does not know about that. Should this be a type invariant? >> It might be a good idea to make `Driver` generic over `D`, the data that is >> stored behind this pointer. You could then return `D::Borrowed` instead. > > We could do: > > impl<D: DriverData> Device<D> { > ... > /// Gets the private data of a device driver. > pub fn drv_priv_data(&self) -> <D::Data as ForeignOwnable>::Borrowed<'_> { > unsafe { > D::Data::borrow(core::ptr::read( > bindings::netdev_priv(self.ptr) as *const *const c_void > )) > } > } > } LGTM (+ adding a Safety comment). > > >>> +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used >>> +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync` >>> +// so it's safe to sharing its pointer. >>> +unsafe impl Send for Device {} >>> +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used >>> +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync`, >>> +// can be used from any thread too. >>> +unsafe impl Sync for Device {} >>> + >>> +/// Trait for device driver specific information. >>> +/// >>> +/// This data structure is passed to a driver with the operations for `struct net_device` >>> +/// like `struct net_device_ops`, `struct ethtool_ops`, `struct rtnl_link_ops`, etc. >>> +pub trait DriverData { >>> + /// The object are stored in C object, `struct net_device`. >>> + type Data: ForeignOwnable + Send + Sync; >> >> Why is this an associated type? Could you not use >> `D: ForeignOwnable + Send + Sync` everywhere instead? >> I think this should be possible, since `DriverData` does not define >> anything else. > > With that approach, is it possible to allow a device driver to define > own data structure and functions taking the structure as aurgument > (like DevOps structutre in the 5th patch) > In the example both structs are empty so I am not really sure why it has to be two types. Can't we do this: ``` pub struct MyDriver { // Just some random fields... pub access_count: Cell<usize>, } impl DriverData for Box<MyDriver> {} // And then we could make `DeviceOperations: DriverData`. // Users would then do this: #[vtable] impl DeviceOperations for Box<MyDriver> { fn init(_dev: Device, data: &MyDriver) -> Result { data.access_count.set(0); Ok(()) } fn open(_dev: Device, data: &MyDriver) -> Result { data.access_count.set(data.access_count.get() + 1); Ok(()) } } ``` I think this would simplify things, because you do not have to carry the extra associated type around (and have to spell out `<D::Data as ForeignOwnable>` everywhere). > >>> +/// Registration structure for a network device driver. >>> +/// >>> +/// This allocates and owns a `struct net_device` object. >>> +/// Once the `net_device` object is registered via `register_netdev` function, >>> +/// the kernel calls various functions such as `struct net_device_ops` operations with >>> +/// the `net_device` object. >>> +/// >>> +/// A driver must implement `struct net_device_ops` so the trait for it is tied. >>> +/// Other operations like `struct ethtool_ops` are optional. >>> +pub struct Registration<T: DeviceOperations<D>, D: DriverData> { >>> + dev: Device, >>> + is_registered: bool, >>> + _p: PhantomData<(D, T)>, >>> +} >>> + >>> +impl<D: DriverData, T: DeviceOperations<D>> Drop for Registration<T, D> { >>> + fn drop(&mut self) { >>> + // SAFETY: The type invariants guarantee that `self.dev.0` is valid. >>> + unsafe { >>> + let _ = D::Data::from_foreign(self.dev.priv_data_ptr()); >> >> Why is `self.dev.priv_data_ptr()` a valid pointer? >> This `unsafe` block should be split to better explain the different safety >> requirements. > > Explained above. > >>> + if self.is_registered { >>> + bindings::unregister_netdev(self.dev.0); >>> + } >>> + bindings::free_netdev(self.dev.0); >>> + } >>> + } >>> +} >>> + >>> +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> { >>> + /// Creates a new [`Registration`] instance for ethernet device. >>> + /// >>> + /// A device driver can pass private data. >>> + pub fn try_new_ether(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> { >>> + // SAFETY: FFI call. >> >> If this FFI call has no safety requirements then say so. > > SAFETY: FFI call has no safety requirements. > > ? Yes you should just write that. > >>> + const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops { >>> + ndo_init: if <T>::HAS_INIT { >>> + Some(Self::init_callback) >>> + } else { >>> + None >>> + }, >>> + ndo_uninit: if <T>::HAS_UNINIT { >>> + Some(Self::uninit_callback) >>> + } else { >>> + None >>> + }, >>> + ndo_open: if <T>::HAS_OPEN { >>> + Some(Self::open_callback) >>> + } else { >>> + None >>> + }, >>> + ndo_stop: if <T>::HAS_STOP { >>> + Some(Self::stop_callback) >>> + } else { >>> + None >>> + }, >>> + ndo_start_xmit: if <T>::HAS_START_XMIT { >>> + Some(Self::start_xmit_callback) >>> + } else { >>> + None >>> + }, >>> + // SAFETY: The rest is zeroed out to initialize `struct net_device_ops`, >>> + // set `Option<&F>` to be `None`. >>> + ..unsafe { core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() } >>> + }; >>> + >>> + const fn build_device_ops() -> &'static bindings::net_device_ops { >>> + &Self::DEVICE_OPS >>> + } >> >> Why does this function exist? > > To get const struct net_device_ops *netdev_ops. Can't you just use `&Self::DEVICE_OPS`? > >>> + >>> + unsafe extern "C" fn init_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { >>> + from_result(|| { >> >> Since you are the first user of `from_result`, you can remove the >> `#[allow(dead_code)]` attribute. >> >> @Reviewers/Maintainers: Or would we prefer to make that change ourselves? > > Ah, either is fine by me. > >>> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >>> + let mut dev = unsafe { Device::from_ptr(netdev) }; >>> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >>> + // `Registration` object was created. >>> + // `D::Data::from_foreign` is only called by the object was released. >>> + // So we know `data` is valid while this function is running. >> >> This should be a type invariant of `Registration`. > > Understood. > >>> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >>> + T::init(&mut dev, data)?; >>> + Ok(0) >>> + }) >>> + } >>> + >>> + unsafe extern "C" fn uninit_callback(netdev: *mut bindings::net_device) { >>> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >>> + let mut dev = unsafe { Device::from_ptr(netdev) }; >>> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >>> + // `Registration` object was created. >>> + // `D::Data::from_foreign` is only called by the object was released. >>> + // So we know `data` is valid while this function is running. >>> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >>> + T::uninit(&mut dev, data); >>> + } >>> + >>> + unsafe extern "C" fn open_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { >>> + from_result(|| { >>> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >>> + let mut dev = unsafe { Device::from_ptr(netdev) }; >>> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >>> + // `Registration` object was created. >>> + // `D::Data::from_foreign` is only called by the object was released. >>> + // So we know `data` is valid while this function is running. >>> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >>> + T::open(&mut dev, data)?; >>> + Ok(0) >>> + }) >>> + } >>> + >>> + unsafe extern "C" fn stop_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { >>> + from_result(|| { >>> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >>> + let mut dev = unsafe { Device::from_ptr(netdev) }; >>> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >>> + // `Registration` object was created. >>> + // `D::Data::from_foreign` is only called by the object was released. >>> + // So we know `data` is valid while this function is running. >>> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >>> + T::stop(&mut dev, data)?; >>> + Ok(0) >>> + }) >>> + } >>> + >>> + unsafe extern "C" fn start_xmit_callback( >>> + skb: *mut bindings::sk_buff, >>> + netdev: *mut bindings::net_device, >>> + ) -> bindings::netdev_tx_t { >>> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >>> + let mut dev = unsafe { Device::from_ptr(netdev) }; >>> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >>> + // `Registration` object was created. >>> + // `D::Data::from_foreign` is only called by the object was released. >>> + // So we know `data` is valid while this function is running. >>> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >>> + // SAFETY: The C API guarantees that `skb` is valid while this function is running. >>> + let skb = unsafe { SkBuff::from_ptr(skb) }; >>> + T::start_xmit(&mut dev, data, skb) as bindings::netdev_tx_t >>> + } >>> +} >>> + >>> +// SAFETY: `Registration` exposes only `Device` object which can be used from >>> +// any thread. >>> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {} >>> +// SAFETY: `Registration` exposes only `Device` object which can be used from >>> +// any thread. >>> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Sync for Registration<T, D> {} >>> + >>> +/// Corresponds to the kernel's `enum netdev_tx`. >>> +#[repr(i32)] >>> +pub enum TxCode { >>> + /// Driver took care of packet. >>> + Ok = bindings::netdev_tx_NETDEV_TX_OK, >>> + /// Driver tx path was busy. >>> + Busy = bindings::netdev_tx_NETDEV_TX_BUSY, >>> +} >>> + >>> +/// Corresponds to the kernel's `struct net_device_ops`. >>> +/// >>> +/// A device driver must implement this. Only very basic operations are supported for now. >>> +#[vtable] >>> +pub trait DeviceOperations<D: DriverData> { >> >> Why is this trait generic over `D`? Why is this not `Self` or an associated >> type? > > DriverData also used in EtherOperationsAdapter (the second patch) and > there are other operations that uses DriverData (not in this patchset). Could you point me to those other things that use `DriverData`? > > >>> + /// Corresponds to `ndo_init` in `struct net_device_ops`. >>> + fn init(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result { >> >> Why do all of these functions take a `&mut Device`? `Device` already is a >> pointer, so why the double indirection? > > I guess that I follow the existing code like > > https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/amba.rs I do not really see a reason why it should be `&mut Device` over just `Device`.
Hi, On Sun, 25 Jun 2023 09:52:53 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 6/21/23 15:13, FUJITA Tomonori wrote: >> Hi, >> Thanks for reviewing. >> >> On Thu, 15 Jun 2023 13:01:50 +0000 >> Benno Lossin <benno.lossin@proton.me> wrote: >> >>> On 6/13/23 06:53, FUJITA Tomonori wrote: >>>> This patch adds very basic abstractions to implement network device >>>> drivers, corresponds to the kernel's net_device and net_device_ops >>>> structs with support for register_netdev/unregister_netdev functions. >>>> >>>> allows the const_maybe_uninit_zeroed feature for >>>> core::mem::MaybeUinit::<T>::zeroed() in const function. >>>> >>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >>>> --- >>>> rust/bindings/bindings_helper.h | 2 + >>>> rust/helpers.c | 16 ++ >>>> rust/kernel/lib.rs | 3 + >>>> rust/kernel/net.rs | 5 + >>>> rust/kernel/net/dev.rs | 344 ++++++++++++++++++++++++++++++++ >>>> 5 files changed, 370 insertions(+) >>>> create mode 100644 rust/kernel/net.rs >>>> create mode 100644 rust/kernel/net/dev.rs >>>> >>>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h >>>> index 3e601ce2548d..468bf606f174 100644 >>>> --- a/rust/bindings/bindings_helper.h >>>> +++ b/rust/bindings/bindings_helper.h >>>> @@ -7,6 +7,8 @@ >>>> */ >>>> >>>> #include <linux/errname.h> >>>> +#include <linux/etherdevice.h> >>>> +#include <linux/netdevice.h> >>>> #include <linux/slab.h> >>>> #include <linux/refcount.h> >>>> #include <linux/wait.h> >>>> diff --git a/rust/helpers.c b/rust/helpers.c >>>> index bb594da56137..70d50767ff4e 100644 >>>> --- a/rust/helpers.c >>>> +++ b/rust/helpers.c >>>> @@ -24,10 +24,26 @@ >>>> #include <linux/errname.h> >>>> #include <linux/refcount.h> >>>> #include <linux/mutex.h> >>>> +#include <linux/netdevice.h> >>>> +#include <linux/skbuff.h> >>>> #include <linux/spinlock.h> >>>> #include <linux/sched/signal.h> >>>> #include <linux/wait.h> >>>> >>>> +#ifdef CONFIG_NET >>>> +void *rust_helper_netdev_priv(const struct net_device *dev) >>>> +{ >>>> + return netdev_priv(dev); >>>> +} >>>> +EXPORT_SYMBOL_GPL(rust_helper_netdev_priv); >>>> + >>>> +void rust_helper_skb_tx_timestamp(struct sk_buff *skb) >>>> +{ >>>> + skb_tx_timestamp(skb); >>>> +} >>>> +EXPORT_SYMBOL_GPL(rust_helper_skb_tx_timestamp); >>>> +#endif >>>> + >>>> __noreturn void rust_helper_BUG(void) >>>> { >>>> BUG(); >>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs >>>> index 85b261209977..fc7d048d359d 100644 >>>> --- a/rust/kernel/lib.rs >>>> +++ b/rust/kernel/lib.rs >>>> @@ -13,6 +13,7 @@ >>>> >>>> #![no_std] >>>> #![feature(allocator_api)] >>>> +#![feature(const_maybe_uninit_zeroed)] >>>> #![feature(coerce_unsized)] >>>> #![feature(dispatch_from_dyn)] >>>> #![feature(new_uninit)] >>>> @@ -34,6 +35,8 @@ >>>> pub mod error; >>>> pub mod init; >>>> pub mod ioctl; >>>> +#[cfg(CONFIG_NET)] >>>> +pub mod net; >>>> pub mod prelude; >>>> pub mod print; >>>> mod static_assert; >>>> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs >>>> new file mode 100644 >>>> index 000000000000..28fe8f398463 >>>> --- /dev/null >>>> +++ b/rust/kernel/net.rs >>>> @@ -0,0 +1,5 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> + >>>> +//! Networking core. >>>> + >>>> +pub mod dev; >>>> diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs >>>> new file mode 100644 >>>> index 000000000000..d072c81f99ce >>>> --- /dev/null >>>> +++ b/rust/kernel/net/dev.rs >>>> @@ -0,0 +1,344 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> + >>>> +//! Network device. >>>> +//! >>>> +//! C headers: [`include/linux/etherdevice.h`](../../../../include/linux/etherdevice.h), >>>> +//! [`include/linux/ethtool.h`](../../../../include/linux/ethtool.h), >>>> +//! [`include/linux/netdevice.h`](../../../../include/linux/netdevice.h), >>>> +//! [`include/linux/skbuff.h`](../../../../include/linux/skbuff.h), >>>> +//! [`include/uapi/linux/if_link.h`](../../../../include/uapi/linux/if_link.h). >>>> + >>>> +use crate::{bindings, error::*, prelude::vtable, types::ForeignOwnable}; >>>> +use {core::ffi::c_void, core::marker::PhantomData}; >>>> + >>>> +/// Corresponds to the kernel's `struct net_device`. >>>> +/// >>>> +/// # Invariants >>>> +/// >>>> +/// The pointer is valid. >>>> +pub struct Device(*mut bindings::net_device); >>>> + >>>> +impl Device { >>>> + /// Creates a new [`Device`] instance. >>>> + /// >>>> + /// # Safety >>>> + /// >>>> + /// Callers must ensure that `ptr` must be valid. >>>> + unsafe fn from_ptr(ptr: *mut bindings::net_device) -> Self { >>>> + // INVARIANT: The safety requirements ensure the invariant. >>>> + Self(ptr) >>>> + } >>>> + >>>> + /// Gets a pointer to network device private data. >>>> + fn priv_data_ptr(&self) -> *const c_void { >>>> + // SAFETY: The type invariants guarantee that `self.0` is valid. >>>> + // During the initialization of `Registration` instance, the kernel allocates >>>> + // contiguous memory for `struct net_device` and a pointer to its private data. >>>> + // So it's safe to read an address from the returned address from `netdev_priv()`. >>>> + unsafe { core::ptr::read(bindings::netdev_priv(self.0) as *const *const c_void) } >>> >>> Why are at least `size_of::<*const c_void>` bytes allocated? Why is it a >>> `*const c_void` pointer? This function does not give any guarantees about >>> this pointer, is it valid? >> >> The reason is a device driver needs its data structure. It needs to >> access to it via a pointer to bindings::net_device struct. The space >> for the pointer is allocated during initialization of Registration and >> it's valid until the Registration object is dropped. > > I think this should be encoded in the type invariants of `Device`. > Because the safety comment needs some justification. > >> >>> I know that you are allocating exactly this amount in `Registration`, but >>> `Device` does not know about that. Should this be a type invariant? >>> It might be a good idea to make `Driver` generic over `D`, the data that is >>> stored behind this pointer. You could then return `D::Borrowed` instead. >> >> We could do: >> >> impl<D: DriverData> Device<D> { >> ... >> /// Gets the private data of a device driver. >> pub fn drv_priv_data(&self) -> <D::Data as ForeignOwnable>::Borrowed<'_> { >> unsafe { >> D::Data::borrow(core::ptr::read( >> bindings::netdev_priv(self.ptr) as *const *const c_void >> )) >> } >> } >> } > > LGTM (+ adding a Safety comment). > >> >> >>>> +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used >>>> +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync` >>>> +// so it's safe to sharing its pointer. >>>> +unsafe impl Send for Device {} >>>> +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used >>>> +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync`, >>>> +// can be used from any thread too. >>>> +unsafe impl Sync for Device {} >>>> + >>>> +/// Trait for device driver specific information. >>>> +/// >>>> +/// This data structure is passed to a driver with the operations for `struct net_device` >>>> +/// like `struct net_device_ops`, `struct ethtool_ops`, `struct rtnl_link_ops`, etc. >>>> +pub trait DriverData { >>>> + /// The object are stored in C object, `struct net_device`. >>>> + type Data: ForeignOwnable + Send + Sync; >>> >>> Why is this an associated type? Could you not use >>> `D: ForeignOwnable + Send + Sync` everywhere instead? >>> I think this should be possible, since `DriverData` does not define >>> anything else. >> >> With that approach, is it possible to allow a device driver to define >> own data structure and functions taking the structure as aurgument >> (like DevOps structutre in the 5th patch) >> > > In the example both structs are empty so I am not really sure why it has > to be two types. Can't we do this: > ``` > pub struct MyDriver { > // Just some random fields... > pub access_count: Cell<usize>, > } > > > impl DriverData for Box<MyDriver> {} > > // And then we could make `DeviceOperations: DriverData`. > // Users would then do this: > > #[vtable] > impl DeviceOperations for Box<MyDriver> { > fn init(_dev: Device, data: &MyDriver) -> Result { > data.access_count.set(0); > Ok(()) > } > > fn open(_dev: Device, data: &MyDriver) -> Result { > data.access_count.set(data.access_count.get() + 1); > Ok(()) > } > } > ``` > > I think this would simplify things, because you do not have to carry the > extra associated type around (and have to spell out > `<D::Data as ForeignOwnable>` everywhere). I'm still not sure if I correctly understand what you try to do. If I define DeviceOperations in dev.rs like the following: #[vtable] pub trait DeviceOperations<D: ForeignOwnable + Send + Sync> { /// Corresponds to `ndo_init` in `struct net_device_ops`. fn init(_dev: &mut Device, _data: D::Borrowed<'_>) -> Result { Ok(()) } } And the driver implmeents DeviceOperations like the folloing: #[vtable] impl<D: ForeignOwnable + Send + Sync> DeviceOperations<D> for Box<DriverData> { fn init(_dev: &mut Device, _data: &DriverData) -> Result { Ok(()) } } I got the following error: error[E0053]: method `init` has an incompatible type for trait --> samples/rust/rust_net_dummy.rs:24:39 | 24 | fn init(_dev: &mut Device, _data: &DriverData) -> Result { | ^^^^^^^^^^^ | | | expected associated type, found `&DriverData` | help: change the parameter type to match the trait: `<D as ForeignOwnable>::Borrowed<'_>` | = note: expected signature `fn(&mut Device, <D as ForeignOwnable>::Borrowed<'_>) -> core::result::Result<_, _>` found signature `fn(&mut Device, &DriverData) -> core::result::Result<_, _>` >>>> +/// Registration structure for a network device driver. >>>> +/// >>>> +/// This allocates and owns a `struct net_device` object. >>>> +/// Once the `net_device` object is registered via `register_netdev` function, >>>> +/// the kernel calls various functions such as `struct net_device_ops` operations with >>>> +/// the `net_device` object. >>>> +/// >>>> +/// A driver must implement `struct net_device_ops` so the trait for it is tied. >>>> +/// Other operations like `struct ethtool_ops` are optional. >>>> +pub struct Registration<T: DeviceOperations<D>, D: DriverData> { >>>> + dev: Device, >>>> + is_registered: bool, >>>> + _p: PhantomData<(D, T)>, >>>> +} >>>> + >>>> +impl<D: DriverData, T: DeviceOperations<D>> Drop for Registration<T, D> { >>>> + fn drop(&mut self) { >>>> + // SAFETY: The type invariants guarantee that `self.dev.0` is valid. >>>> + unsafe { >>>> + let _ = D::Data::from_foreign(self.dev.priv_data_ptr()); >>> >>> Why is `self.dev.priv_data_ptr()` a valid pointer? >>> This `unsafe` block should be split to better explain the different safety >>> requirements. >> >> Explained above. >> >>>> + if self.is_registered { >>>> + bindings::unregister_netdev(self.dev.0); >>>> + } >>>> + bindings::free_netdev(self.dev.0); >>>> + } >>>> + } >>>> +} >>>> + >>>> +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> { >>>> + /// Creates a new [`Registration`] instance for ethernet device. >>>> + /// >>>> + /// A device driver can pass private data. >>>> + pub fn try_new_ether(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> { >>>> + // SAFETY: FFI call. >>> >>> If this FFI call has no safety requirements then say so. >> >> SAFETY: FFI call has no safety requirements. >> >> ? > > Yes you should just write that. > >> >>>> + const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops { >>>> + ndo_init: if <T>::HAS_INIT { >>>> + Some(Self::init_callback) >>>> + } else { >>>> + None >>>> + }, >>>> + ndo_uninit: if <T>::HAS_UNINIT { >>>> + Some(Self::uninit_callback) >>>> + } else { >>>> + None >>>> + }, >>>> + ndo_open: if <T>::HAS_OPEN { >>>> + Some(Self::open_callback) >>>> + } else { >>>> + None >>>> + }, >>>> + ndo_stop: if <T>::HAS_STOP { >>>> + Some(Self::stop_callback) >>>> + } else { >>>> + None >>>> + }, >>>> + ndo_start_xmit: if <T>::HAS_START_XMIT { >>>> + Some(Self::start_xmit_callback) >>>> + } else { >>>> + None >>>> + }, >>>> + // SAFETY: The rest is zeroed out to initialize `struct net_device_ops`, >>>> + // set `Option<&F>` to be `None`. >>>> + ..unsafe { core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() } >>>> + }; >>>> + >>>> + const fn build_device_ops() -> &'static bindings::net_device_ops { >>>> + &Self::DEVICE_OPS >>>> + } >>> >>> Why does this function exist? >> >> To get const struct net_device_ops *netdev_ops. > > Can't you just use `&Self::DEVICE_OPS`? I think that it didn't work in the past but seems that it works now. I'll fix. >>>> + >>>> + unsafe extern "C" fn init_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { >>>> + from_result(|| { >>> >>> Since you are the first user of `from_result`, you can remove the >>> `#[allow(dead_code)]` attribute. >>> >>> @Reviewers/Maintainers: Or would we prefer to make that change ourselves? >> >> Ah, either is fine by me. >> >>>> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >>>> + let mut dev = unsafe { Device::from_ptr(netdev) }; >>>> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >>>> + // `Registration` object was created. >>>> + // `D::Data::from_foreign` is only called by the object was released. >>>> + // So we know `data` is valid while this function is running. >>> >>> This should be a type invariant of `Registration`. >> >> Understood. >> >>>> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >>>> + T::init(&mut dev, data)?; >>>> + Ok(0) >>>> + }) >>>> + } >>>> + >>>> + unsafe extern "C" fn uninit_callback(netdev: *mut bindings::net_device) { >>>> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >>>> + let mut dev = unsafe { Device::from_ptr(netdev) }; >>>> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >>>> + // `Registration` object was created. >>>> + // `D::Data::from_foreign` is only called by the object was released. >>>> + // So we know `data` is valid while this function is running. >>>> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >>>> + T::uninit(&mut dev, data); >>>> + } >>>> + >>>> + unsafe extern "C" fn open_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { >>>> + from_result(|| { >>>> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >>>> + let mut dev = unsafe { Device::from_ptr(netdev) }; >>>> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >>>> + // `Registration` object was created. >>>> + // `D::Data::from_foreign` is only called by the object was released. >>>> + // So we know `data` is valid while this function is running. >>>> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >>>> + T::open(&mut dev, data)?; >>>> + Ok(0) >>>> + }) >>>> + } >>>> + >>>> + unsafe extern "C" fn stop_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { >>>> + from_result(|| { >>>> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >>>> + let mut dev = unsafe { Device::from_ptr(netdev) }; >>>> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >>>> + // `Registration` object was created. >>>> + // `D::Data::from_foreign` is only called by the object was released. >>>> + // So we know `data` is valid while this function is running. >>>> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >>>> + T::stop(&mut dev, data)?; >>>> + Ok(0) >>>> + }) >>>> + } >>>> + >>>> + unsafe extern "C" fn start_xmit_callback( >>>> + skb: *mut bindings::sk_buff, >>>> + netdev: *mut bindings::net_device, >>>> + ) -> bindings::netdev_tx_t { >>>> + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. >>>> + let mut dev = unsafe { Device::from_ptr(netdev) }; >>>> + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when >>>> + // `Registration` object was created. >>>> + // `D::Data::from_foreign` is only called by the object was released. >>>> + // So we know `data` is valid while this function is running. >>>> + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; >>>> + // SAFETY: The C API guarantees that `skb` is valid while this function is running. >>>> + let skb = unsafe { SkBuff::from_ptr(skb) }; >>>> + T::start_xmit(&mut dev, data, skb) as bindings::netdev_tx_t >>>> + } >>>> +} >>>> + >>>> +// SAFETY: `Registration` exposes only `Device` object which can be used from >>>> +// any thread. >>>> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {} >>>> +// SAFETY: `Registration` exposes only `Device` object which can be used from >>>> +// any thread. >>>> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Sync for Registration<T, D> {} >>>> + >>>> +/// Corresponds to the kernel's `enum netdev_tx`. >>>> +#[repr(i32)] >>>> +pub enum TxCode { >>>> + /// Driver took care of packet. >>>> + Ok = bindings::netdev_tx_NETDEV_TX_OK, >>>> + /// Driver tx path was busy. >>>> + Busy = bindings::netdev_tx_NETDEV_TX_BUSY, >>>> +} >>>> + >>>> +/// Corresponds to the kernel's `struct net_device_ops`. >>>> +/// >>>> +/// A device driver must implement this. Only very basic operations are supported for now. >>>> +#[vtable] >>>> +pub trait DeviceOperations<D: DriverData> { >>> >>> Why is this trait generic over `D`? Why is this not `Self` or an associated >>> type? >> >> DriverData also used in EtherOperationsAdapter (the second patch) and >> there are other operations that uses DriverData (not in this patchset). > > Could you point me to those other things that use `DriverData`? net_device struct has some like tlsdev_ops, rtnl_link_ops.. A device driver might need to access to the private data via net_device in these operations. thanks,
On 25.06.23 16:27, FUJITA Tomonori wrote: > Hi, > > On Sun, 25 Jun 2023 09:52:53 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > >>>>> +/// Trait for device driver specific information. >>>>> +/// >>>>> +/// This data structure is passed to a driver with the operations for `struct net_device` >>>>> +/// like `struct net_device_ops`, `struct ethtool_ops`, `struct rtnl_link_ops`, etc. >>>>> +pub trait DriverData { >>>>> + /// The object are stored in C object, `struct net_device`. >>>>> + type Data: ForeignOwnable + Send + Sync; >>>> >>>> Why is this an associated type? Could you not use >>>> `D: ForeignOwnable + Send + Sync` everywhere instead? >>>> I think this should be possible, since `DriverData` does not define >>>> anything else. >>> >>> With that approach, is it possible to allow a device driver to define >>> own data structure and functions taking the structure as aurgument >>> (like DevOps structutre in the 5th patch) >>> >> >> In the example both structs are empty so I am not really sure why it has >> to be two types. Can't we do this: >> ``` >> pub struct MyDriver { >> // Just some random fields... >> pub access_count: Cell<usize>, >> } >> >> >> impl DriverData for Box<MyDriver> {} >> >> // And then we could make `DeviceOperations: DriverData`. >> // Users would then do this: >> >> #[vtable] >> impl DeviceOperations for Box<MyDriver> { >> fn init(_dev: Device, data: &MyDriver) -> Result { >> data.access_count.set(0); >> Ok(()) >> } >> >> fn open(_dev: Device, data: &MyDriver) -> Result { >> data.access_count.set(data.access_count.get() + 1); >> Ok(()) >> } >> } >> ``` >> >> I think this would simplify things, because you do not have to carry the >> extra associated type around (and have to spell out >> `<D::Data as ForeignOwnable>` everywhere). > > I'm still not sure if I correctly understand what you try to do. > > If I define DeviceOperations in dev.rs like the following: > > #[vtable] > pub trait DeviceOperations<D: ForeignOwnable + Send + Sync> { > /// Corresponds to `ndo_init` in `struct net_device_ops`. > fn init(_dev: &mut Device, _data: D::Borrowed<'_>) -> Result { > Ok(()) > } > } > > And the driver implmeents DeviceOperations like the folloing: > > #[vtable] > impl<D: ForeignOwnable + Send + Sync> DeviceOperations<D> for Box<DriverData> { > fn init(_dev: &mut Device, _data: &DriverData) -> Result { > Ok(()) > } > } > > I got the following error: > > error[E0053]: method `init` has an incompatible type for trait > --> samples/rust/rust_net_dummy.rs:24:39 > | > 24 | fn init(_dev: &mut Device, _data: &DriverData) -> Result { > | ^^^^^^^^^^^ > | | > | expected associated type, found `&DriverData` > | help: change the parameter type to match the trait: `<D as ForeignOwnable>::Borrowed<'_>` > | > = note: expected signature `fn(&mut Device, <D as ForeignOwnable>::Borrowed<'_>) -> core::result::Result<_, _>` > found signature `fn(&mut Device, &DriverData) -> core::result::Result<_, _>` > I thought you could do this: ``` #[vtable] pub trait DeviceOperations: ForeignOwnable + Send + Sync { /// Corresponds to `ndo_init` in `struct net_device_ops`. fn init(_dev: &mut Device, _data: Self::Borrowed<'_>) -> Result { Ok(()) } } #[vtable] impl DeviceOperations<D> for Box<DriverData> { fn init(_dev: &mut Device, _data: &DriverData) -> Result { Ok(()) } } ``` >>>>> + const fn build_device_ops() -> &'static bindings::net_device_ops { >>>>> + &Self::DEVICE_OPS >>>>> + } >>>> >>>> Why does this function exist? >>> >>> To get const struct net_device_ops *netdev_ops. >> >> Can't you just use `&Self::DEVICE_OPS`? > > I think that it didn't work in the past but seems that it works > now. I'll fix. > > >>>>> +/// Corresponds to the kernel's `struct net_device_ops`. >>>>> +/// >>>>> +/// A device driver must implement this. Only very basic operations are supported for now. >>>>> +#[vtable] >>>>> +pub trait DeviceOperations<D: DriverData> { >>>> >>>> Why is this trait generic over `D`? Why is this not `Self` or an associated >>>> type? >>> >>> DriverData also used in EtherOperationsAdapter (the second patch) and >>> there are other operations that uses DriverData (not in this patchset). >> >> Could you point me to those other things that use `DriverData`? > > net_device struct has some like tlsdev_ops, rtnl_link_ops.. A device > driver might need to access to the private data via net_device in > these operations. In my mental model you can just implement the `TLSOperations` trait alongside the `DeviceOperations` trait. But I would have to see the API that will be used for that to be sure. I do not think that you need to have different private data for each of those operations traits. -- Cheers, Benno > > > thanks,
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 3e601ce2548d..468bf606f174 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -7,6 +7,8 @@ */ #include <linux/errname.h> +#include <linux/etherdevice.h> +#include <linux/netdevice.h> #include <linux/slab.h> #include <linux/refcount.h> #include <linux/wait.h> diff --git a/rust/helpers.c b/rust/helpers.c index bb594da56137..70d50767ff4e 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -24,10 +24,26 @@ #include <linux/errname.h> #include <linux/refcount.h> #include <linux/mutex.h> +#include <linux/netdevice.h> +#include <linux/skbuff.h> #include <linux/spinlock.h> #include <linux/sched/signal.h> #include <linux/wait.h> +#ifdef CONFIG_NET +void *rust_helper_netdev_priv(const struct net_device *dev) +{ + return netdev_priv(dev); +} +EXPORT_SYMBOL_GPL(rust_helper_netdev_priv); + +void rust_helper_skb_tx_timestamp(struct sk_buff *skb) +{ + skb_tx_timestamp(skb); +} +EXPORT_SYMBOL_GPL(rust_helper_skb_tx_timestamp); +#endif + __noreturn void rust_helper_BUG(void) { BUG(); diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 85b261209977..fc7d048d359d 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -13,6 +13,7 @@ #![no_std] #![feature(allocator_api)] +#![feature(const_maybe_uninit_zeroed)] #![feature(coerce_unsized)] #![feature(dispatch_from_dyn)] #![feature(new_uninit)] @@ -34,6 +35,8 @@ pub mod error; pub mod init; pub mod ioctl; +#[cfg(CONFIG_NET)] +pub mod net; pub mod prelude; pub mod print; mod static_assert; diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs new file mode 100644 index 000000000000..28fe8f398463 --- /dev/null +++ b/rust/kernel/net.rs @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Networking core. + +pub mod dev; diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs new file mode 100644 index 000000000000..d072c81f99ce --- /dev/null +++ b/rust/kernel/net/dev.rs @@ -0,0 +1,344 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Network device. +//! +//! C headers: [`include/linux/etherdevice.h`](../../../../include/linux/etherdevice.h), +//! [`include/linux/ethtool.h`](../../../../include/linux/ethtool.h), +//! [`include/linux/netdevice.h`](../../../../include/linux/netdevice.h), +//! [`include/linux/skbuff.h`](../../../../include/linux/skbuff.h), +//! [`include/uapi/linux/if_link.h`](../../../../include/uapi/linux/if_link.h). + +use crate::{bindings, error::*, prelude::vtable, types::ForeignOwnable}; +use {core::ffi::c_void, core::marker::PhantomData}; + +/// Corresponds to the kernel's `struct net_device`. +/// +/// # Invariants +/// +/// The pointer is valid. +pub struct Device(*mut bindings::net_device); + +impl Device { + /// Creates a new [`Device`] instance. + /// + /// # Safety + /// + /// Callers must ensure that `ptr` must be valid. + unsafe fn from_ptr(ptr: *mut bindings::net_device) -> Self { + // INVARIANT: The safety requirements ensure the invariant. + Self(ptr) + } + + /// Gets a pointer to network device private data. + fn priv_data_ptr(&self) -> *const c_void { + // SAFETY: The type invariants guarantee that `self.0` is valid. + // During the initialization of `Registration` instance, the kernel allocates + // contiguous memory for `struct net_device` and a pointer to its private data. + // So it's safe to read an address from the returned address from `netdev_priv()`. + unsafe { core::ptr::read(bindings::netdev_priv(self.0) as *const *const c_void) } + } +} + +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync` +// so it's safe to sharing its pointer. +unsafe impl Send for Device {} +// SAFETY: `Device` is just a wrapper for the kernel`s `struct net_device`, which can be used +// from any thread. `struct net_device` stores a pointer to `DriverData::Data`, which is `Sync`, +// can be used from any thread too. +unsafe impl Sync for Device {} + +/// Trait for device driver specific information. +/// +/// This data structure is passed to a driver with the operations for `struct net_device` +/// like `struct net_device_ops`, `struct ethtool_ops`, `struct rtnl_link_ops`, etc. +pub trait DriverData { + /// The object are stored in C object, `struct net_device`. + type Data: ForeignOwnable + Send + Sync; +} + +/// Registration structure for a network device driver. +/// +/// This allocates and owns a `struct net_device` object. +/// Once the `net_device` object is registered via `register_netdev` function, +/// the kernel calls various functions such as `struct net_device_ops` operations with +/// the `net_device` object. +/// +/// A driver must implement `struct net_device_ops` so the trait for it is tied. +/// Other operations like `struct ethtool_ops` are optional. +pub struct Registration<T: DeviceOperations<D>, D: DriverData> { + dev: Device, + is_registered: bool, + _p: PhantomData<(D, T)>, +} + +impl<D: DriverData, T: DeviceOperations<D>> Drop for Registration<T, D> { + fn drop(&mut self) { + // SAFETY: The type invariants guarantee that `self.dev.0` is valid. + unsafe { + let _ = D::Data::from_foreign(self.dev.priv_data_ptr()); + if self.is_registered { + bindings::unregister_netdev(self.dev.0); + } + bindings::free_netdev(self.dev.0); + } + } +} + +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> { + /// Creates a new [`Registration`] instance for ethernet device. + /// + /// A device driver can pass private data. + pub fn try_new_ether(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> { + // SAFETY: FFI call. + let ptr = from_err_ptr(unsafe { + bindings::alloc_etherdev_mqs( + core::mem::size_of::<*const c_void>() as i32, + tx_queue_size, + rx_queue_size, + ) + })?; + + // SAFETY: `ptr` is valid and non-null since `alloc_etherdev_mqs()` + // returned a valid pointer which was null-checked. + let dev = unsafe { Device::from_ptr(ptr) }; + // SAFETY: It's safe to write an address to the returned pointer + // from `netdev_priv()` because `alloc_etherdev_mqs()` allocates + // contiguous memory for `struct net_device` and a pointer. + unsafe { + let priv_ptr = bindings::netdev_priv(ptr) as *mut *const c_void; + core::ptr::write(priv_ptr, data.into_foreign()); + } + Ok(Registration { + dev, + is_registered: false, + _p: PhantomData, + }) + } + + /// Returns a network device. + /// + /// A device driver normally configures the device before registration. + pub fn dev_get(&mut self) -> &mut Device { + &mut self.dev + } + + /// Registers a network device. + pub fn register(&mut self) -> Result { + if self.is_registered { + return Err(code::EINVAL); + } + // SAFETY: The type invariants guarantee that `self.dev.0` is valid. + let ret = unsafe { + (*self.dev.0).netdev_ops = Self::build_device_ops(); + bindings::register_netdev(self.dev.0) + }; + if ret != 0 { + Err(Error::from_errno(ret)) + } else { + self.is_registered = true; + Ok(()) + } + } + + const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops { + ndo_init: if <T>::HAS_INIT { + Some(Self::init_callback) + } else { + None + }, + ndo_uninit: if <T>::HAS_UNINIT { + Some(Self::uninit_callback) + } else { + None + }, + ndo_open: if <T>::HAS_OPEN { + Some(Self::open_callback) + } else { + None + }, + ndo_stop: if <T>::HAS_STOP { + Some(Self::stop_callback) + } else { + None + }, + ndo_start_xmit: if <T>::HAS_START_XMIT { + Some(Self::start_xmit_callback) + } else { + None + }, + // SAFETY: The rest is zeroed out to initialize `struct net_device_ops`, + // set `Option<&F>` to be `None`. + ..unsafe { core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() } + }; + + const fn build_device_ops() -> &'static bindings::net_device_ops { + &Self::DEVICE_OPS + } + + unsafe extern "C" fn init_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { + from_result(|| { + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. + let mut dev = unsafe { Device::from_ptr(netdev) }; + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when + // `Registration` object was created. + // `D::Data::from_foreign` is only called by the object was released. + // So we know `data` is valid while this function is running. + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; + T::init(&mut dev, data)?; + Ok(0) + }) + } + + unsafe extern "C" fn uninit_callback(netdev: *mut bindings::net_device) { + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. + let mut dev = unsafe { Device::from_ptr(netdev) }; + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when + // `Registration` object was created. + // `D::Data::from_foreign` is only called by the object was released. + // So we know `data` is valid while this function is running. + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; + T::uninit(&mut dev, data); + } + + unsafe extern "C" fn open_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { + from_result(|| { + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. + let mut dev = unsafe { Device::from_ptr(netdev) }; + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when + // `Registration` object was created. + // `D::Data::from_foreign` is only called by the object was released. + // So we know `data` is valid while this function is running. + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; + T::open(&mut dev, data)?; + Ok(0) + }) + } + + unsafe extern "C" fn stop_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int { + from_result(|| { + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. + let mut dev = unsafe { Device::from_ptr(netdev) }; + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when + // `Registration` object was created. + // `D::Data::from_foreign` is only called by the object was released. + // So we know `data` is valid while this function is running. + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; + T::stop(&mut dev, data)?; + Ok(0) + }) + } + + unsafe extern "C" fn start_xmit_callback( + skb: *mut bindings::sk_buff, + netdev: *mut bindings::net_device, + ) -> bindings::netdev_tx_t { + // SAFETY: The C API guarantees that `netdev` is valid while this function is running. + let mut dev = unsafe { Device::from_ptr(netdev) }; + // SAFETY: The returned pointer was initialized by `D::Data::into_foreign` when + // `Registration` object was created. + // `D::Data::from_foreign` is only called by the object was released. + // So we know `data` is valid while this function is running. + let data = unsafe { D::Data::borrow(dev.priv_data_ptr()) }; + // SAFETY: The C API guarantees that `skb` is valid while this function is running. + let skb = unsafe { SkBuff::from_ptr(skb) }; + T::start_xmit(&mut dev, data, skb) as bindings::netdev_tx_t + } +} + +// SAFETY: `Registration` exposes only `Device` object which can be used from +// any thread. +unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {} +// SAFETY: `Registration` exposes only `Device` object which can be used from +// any thread. +unsafe impl<D: DriverData, T: DeviceOperations<D>> Sync for Registration<T, D> {} + +/// Corresponds to the kernel's `enum netdev_tx`. +#[repr(i32)] +pub enum TxCode { + /// Driver took care of packet. + Ok = bindings::netdev_tx_NETDEV_TX_OK, + /// Driver tx path was busy. + Busy = bindings::netdev_tx_NETDEV_TX_BUSY, +} + +/// Corresponds to the kernel's `struct net_device_ops`. +/// +/// A device driver must implement this. Only very basic operations are supported for now. +#[vtable] +pub trait DeviceOperations<D: DriverData> { + /// Corresponds to `ndo_init` in `struct net_device_ops`. + fn init(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result { + Ok(()) + } + + /// Corresponds to `ndo_uninit` in `struct net_device_ops`. + fn uninit(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) {} + + /// Corresponds to `ndo_open` in `struct net_device_ops`. + fn open(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result { + Ok(()) + } + + /// Corresponds to `ndo_stop` in `struct net_device_ops`. + fn stop(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result { + Ok(()) + } + + /// Corresponds to `ndo_start_xmit` in `struct net_device_ops`. + fn start_xmit( + _dev: &mut Device, + _data: <D::Data as ForeignOwnable>::Borrowed<'_>, + _skb: SkBuff, + ) -> TxCode { + TxCode::Busy + } +} + +/// Corresponds to the kernel's `struct sk_buff`. +/// +/// A driver manages `struct sk_buff` in two ways. In both ways, the ownership is transferred +/// between C and Rust. The allocation and release are done asymmetrically. +/// +/// On the tx side (`ndo_start_xmit` operation in `struct net_device_ops`), the kernel allocates +/// a `sk_buff' object and passes it to the driver. The driver is responsible for the release +/// after transmission. +/// On the rx side, the driver allocates a `sk_buff` object then passes it to the kernel +/// after receiving data. +/// +/// # Invariants +/// +/// The pointer is valid. +pub struct SkBuff(*mut bindings::sk_buff); + +impl SkBuff { + /// Creates a new [`SkBuff`] instance. + /// + /// # Safety + /// + /// Callers must ensure that `ptr` must be valid. + unsafe fn from_ptr(ptr: *mut bindings::sk_buff) -> Self { + // INVARIANT: The safety requirements ensure the invariant. + Self(ptr) + } + + /// Provides a time stamp. + pub fn tx_timestamp(&mut self) { + // SAFETY: The type invariants guarantee that `self.0` is valid. + unsafe { + bindings::skb_tx_timestamp(self.0); + } + } +} + +impl Drop for SkBuff { + fn drop(&mut self) { + // SAFETY: The type invariants guarantee that `self.0` is valid. + unsafe { + bindings::kfree_skb_reason( + self.0, + bindings::skb_drop_reason_SKB_DROP_REASON_NOT_SPECIFIED, + ) + } + } +}
This patch adds very basic abstractions to implement network device drivers, corresponds to the kernel's net_device and net_device_ops structs with support for register_netdev/unregister_netdev functions. allows the const_maybe_uninit_zeroed feature for core::mem::MaybeUinit::<T>::zeroed() in const function. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/bindings/bindings_helper.h | 2 + rust/helpers.c | 16 ++ rust/kernel/lib.rs | 3 + rust/kernel/net.rs | 5 + rust/kernel/net/dev.rs | 344 ++++++++++++++++++++++++++++++++ 5 files changed, 370 insertions(+) create mode 100644 rust/kernel/net.rs create mode 100644 rust/kernel/net/dev.rs