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

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

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

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

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

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

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

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

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

Cheers,
Miguel