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

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20230616.211821.1815408081024606989.ubuntu@gmail.com>
Date: Fri, 16 Jun 2023 21:18:21 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: kuba@...nel.org
Cc: miguel.ojeda.sandonis@...il.com, fujita.tomonori@...il.com,
 netdev@...r.kernel.org, rust-for-linux@...r.kernel.org,
 aliceryhl@...gle.com, andrew@...n.ch
Subject: Re: [PATCH 0/5] Rust abstractions for network device drivers

Hi,

On Thu, 15 Jun 2023 19:19:31 -0700
Jakub Kicinski <kuba@...nel.org> wrote:

> TBH I was hoping that the code will be more like reading "modern C++"
> for a C developer. I can't understand much of what's going on.
> 
> Taking an example of what I have randomly on the screen as I'm writing
> this email:
> 
> +    /// Updates TX stats.
> +    pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) {
> +        // SAFETY: We have exclusive access to the `rtnl_link_stats64`, so writing to it is okay.
> +        unsafe {
> +            let inner = Opaque::get(&self.0);
> +            (*inner).tx_packets = packets;
> +            (*inner).tx_bytes = bytes;
> +            (*inner).tx_errors = errors;
> +            (*inner).tx_dropped = dropped;
> +        }
> +    }
> 
> What is this supposed to be doing? Who needs to _set_ unrelated
> statistics from a function call? Yet no reviewer is complaining
> which either means I don't understand, or people aren't really 
> paying attention :(

Sorry, this function was used in the dummy driver to implement
net_device_ops->ndo_get_stats64:

https://lore.kernel.org/rust-for-linux/01010188a025632b-16a4fb69-5601-4f46-a170-52b5f2921ed2-000000@us-west-2.amazonses.com/T/#m518550baea9c76224524e44ab3ee5a0ecd01b1b9

The old version uses atomic types in xmit path to save tx stats for
ndo_get_stats64. But Andrew said that using atomic types in xmit path
isn't a good idea in even sample driver so I removed that code.


>> But, indeed, it is best if a `F:` entry is added wherever you think it
>> is best. Some subsystems may just add it to their entry (e.g. KUnit
>> wants to do that). Others may decide to split the Rust part into
>> another entry, so that maintainers may be a subset (or a different set
>> -- sometimes this could be done, for instance, if a new maintainer
>> shows up that wants to take care of the Rust abstractions).
> 
> I think F: would work for us.
> 
> Are there success stories in any subsystem for getting a driver for
> real HW supported? I think the best way to focus the effort would be 
> to set a target on a relatively simple device.

As far as I know, no subsystem has accepted Rust bindings yet.

Replacing the existing C driver for real HW with Rust new one doesn't
make sense, right? So a necessary condition of getting Rust bindings
for a subsystem accepted is that a HW verndor implements both a driver
and bindings for their new HW?

thanks,