linux-kernel - Re: [PATCH 1/2] rust: add Soft-RoCE driver basic structure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CANiq72n+xr+3TqQMaoP3xOzM2ZqUyzKvGm2wDMd8SbsqBK7nvg@mail.gmail.com>
Date:   Sat, 30 Sep 2023 20:48:59 +0200
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     AllenX <xubo3006@....com>
Cc:     rust-for-linux@...r.kernel.org, ojeda@...nel.org,
        alex.gaynor@...il.com, wedsonaf@...il.com, boqun.feng@...il.com,
        gary@...yguo.net, bjorn3_gh@...tonmail.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] rust: add Soft-RoCE driver basic structure

Hi Allen,

A quick few notes I noticed that you will probably want to address
before sending this to the Infiniband/rdma maintainers. Please note
that some of these apply in more than one instance, and that these are
just surface-level -- I have not actually looked at what the code is
doing or whether it is sound.

On Sat, Sep 30, 2023 at 6:16 PM AllenX <xubo3006@....com> wrote:
>
> +//! Infiniband soft-Roce devices.

Please follow the formatting/coding style of the rest of the code
already upstream. For instance, a newline should be placed here.

> +/// Soft-Roce transport registration.
> +///

Docs should not end in an empty line.

> +impl<T: RxeOperation> Registration<T> {
> +    /// Creates a new [`Registration`] but does not register it yet.
> +    ///
> +    /// It is allowed to move.
> +    pub fn new(name: &'static CStr) -> Self {
> +        // INVARIANT: `registered` is `false`

There is no `# Invariants` section in the type. Please check other
types we have to see how it is usually done.

> +    /// Registers a infiniband soft-Roce device

Docs should only have a single first line/sentence and end in a period.

> +        // SAFETY: The adapter is compatible with the rdma_link_register

Please use Markdown in code comments, just like in the documentation,
and end sentences and comments in a period for consistency with the
rest of the code; e.g.

    with the `rdma_link_register`.

> +        pr_info!("loaded");

Debugging code?

> +/// soft-Roce register net sockets

Please be consistent in the documentation, e.g. you used "Soft-Roce" above.

> +    /// Init ipv4 socket
> +    fn ipv4_init(&mut self) -> Result<()> {
> +        let mut udp_cfg = bindings::udp_port_cfg::default();
> +        let mut tnl_cfg = bindings::udp_tunnel_sock_cfg::default();
> +        let mut sock: *mut bindings::socket = ptr::null_mut();
> +
> +        udp_cfg.family = bindings::AF_INET as u8;
> +        udp_cfg.local_udp_port = 46866;
> +        // SAFETY: [`bindings::init_net`] and [`udp_cfg`] can be safely passed to [`bindings::udp_sock_create4`]
> +        // [`sock`] will be pass to [`self.sk4`] later, it will live at least as long as the module, which is an implicit requirement
> +        let err =
> +            unsafe { bindings::udp_sock_create4(&mut bindings::init_net, &mut udp_cfg, &mut sock) };
> +
> +        if err < 0 {
> +            pr_err!("Failed to create IPv4 UDP tunnel\n");
> +            return Err(Error::from_kernel_errno(err));
> +        }
> +
> +        tnl_cfg.encap_type = 1;
> +        tnl_cfg.encap_rcv = RxeUdpEncapRecvFuncTable::<T>::build_func();
> +
> +        // SAFETY: [`bindings::init_net`] and [`tnl_cfg`] can be safely passed to [`bindings::setup_udp_tunnel_sock`]
> +        // [`sock`] will be pass to [`self.sk4`] later, it will live at least as long as the module, which is an implicit requirement
> +        unsafe { bindings::setup_udp_tunnel_sock(&mut bindings::init_net, sock, &mut tnl_cfg) }
> +        self.sk4 = Some(sock);
> +        Ok(())
> +    }

Some networking abstractions will be needed here, instead of calling
the C APIs directly. There are some networking discussions going on in
the list and in our Zulip -- please take a look!

> +            if err < 0 {
> +                // EAFNOSUPPORT
> +                if err == -97 {

The error code should be added to the list that we import from the C
headers, instead of hardcoding the value.

> +                    pr_err!("IPv6 is not supported, can not create a UDPv6 socket\n");
> +                    return Ok(());

Should this really return `Ok`?

Does the C side print errors too?

> +            if self.rxe_net_notifier.is_some() {
> +                // SAFETY: [`self.rxe_net_notifier`] is Some, it was previously created by
> +                // RxeNotifyFuncTable::<T>::build().
> +                unsafe {
> +                    bindings::unregister_netdevice_notifier(
> +                        &mut self.rxe_net_notifier.take().unwrap(),
> +                    )
> +                };
> +            }
> +            return Err(Error::from_kernel_errno(err));

This looks like it is undoing an operation before returning in an
error path -- there are usually better patterns for this, like RAII or
the `ScopeGuard` type (already in-tree).

> +config SAMPLE_RUST_RXE
> +       tristate "Soft-Roce"
> +       help
> +         This option builds the self test cases for Rust.

Nowadays we have KUnit support for running Rust code documentation
tests -- you may be interested in those.

Also, more documentation, including `# Examples` sections in the
abstractions would be very helpful, I would imagine.

Cheers,
Miguel