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

Hi,

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

> On Thu, 15 Jun 2023 14:51:10 +0200 Andrew Lunn wrote:
>> On Wed, Jun 14, 2023 at 11:01:28PM -0700, Jakub Kicinski wrote:
>> > On Tue, 13 Jun 2023 13:53:21 +0900 FUJITA Tomonori wrote:  
>> > > This patchset adds minimum Rust abstractions for network device
>> > > drivers and an example of a Rust network device driver, a simpler
>> > > version of drivers/net/dummy.c.
>> > > 
>> > > The dummy network device driver doesn't attach any bus such as PCI so
>> > > the dependency is minimum. Hopefully, it would make reviewing easier.
>> > > 
>> > > Thanks a lot for reviewing on RFC patchset at rust-for-linux ml.
>> > > Hopefully, I've addressed all the issues.  
>> > 
>> > First things first, what are the current expectations for subsystems
>> > accepting rust code?
>> > 
>> > I was hoping someone from the Rust side is going to review this.
>> > We try to review stuff within 48h at netdev, and there's no review :S  
>> 
>> As pointed out elsewhere, i've looked the code over. I cannot say
>> anything about the rust, but i don't see anything too obviously wrong
>> with the way it use a few netdev API calls.
> 
> The skb freeing looks shady from functional perspective.
> I'm guessing some form of destructor frees the skb automatically
> in xmit handler(?), but (a) no reason support, (b) kfree_skb_reason()
> is most certainly not safe to call on all xmit paths...

Yeah, I assume that a driver keeps a skb in private data structure
(such as tx ring) then removes the skb from it after the completion of
tx; automatically the drop() method runs (where we need to free the
skb).

I thought that calling dev_kfree_skb() is fine but no? We also need
something different for drivers that use other ways to free the skb
though.

I use kfree_skb_reason() because dev_kfree_skb() is a macro so it
can't be called directly from Rust. But I should have used
dev_kfree_skb() with a helper function.

thanks,