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

[Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>]

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

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

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

Similarly for `SkBuff` below.

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

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

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

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

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

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

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

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

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

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

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

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

Do you mean `dev.0`?

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

Please capitalize to be consistent.

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

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

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

Thanks!

Cheers,
Miguel