Rust NetDevice and NetDeviceOperationsVtable struct #208

kloenk · 2021-04-19T11:03:15Z

add function for net_device functions

Contains #226 and #227

kloenk · 2021-04-20T08:45:45Z

I'm a bit struggling with the rtnl_link_ops struct. For the other *_ops table I use a const as the FileOperationsVtable. But rtnl_link_ops has to be writable, even if that is not often (__read_mostly). Any ideas on this?

Also I'm unsure on how to populate the kind field of that struct

adamrk · 2021-04-21T08:40:15Z

Maybe I'm misunderstanding your issue, but if you're thinking of doing something similar to the existing FileOperationsVtable then I'd think it's ok. The VTABLE there needs to be const so that it can be constructed at compile time - when the device is registered, a reference to the file_operations is passed to the C side and could be mutated.

For the kind field, I'd think you'd want to have the user implement a trait with const KIND: &str and convert that to the kind entry in rtnl_link_ops.

kloenk · 2021-04-21T20:51:49Z

@adamrk I think I understand how the FileOperationsVtable works. My problem with rtnl_link_ops is, that this has to be writable, as (as far as the folks from the kernel told me) there is a linked list inside which is used internally by the kernel. It's only written very rare, but it's written to (that's why it's in .data.read_mosty). My Idea is to try it with a macro which creates a static mut variable, which than can be assigned to that link. Did not get around yet to implement that, maybe only after the 4th of may as I have to write my finals :-)

kloenk · 2021-04-22T20:45:15Z

I created a proc macro, which creates a wrapped rtnl_link_ops (wrapped as we need Sync). This seems to work so far, I'm still trying to reimplement the drivers/net/dummy driver

kloenk · 2021-04-25T11:41:23Z

Rebased to have rust-analyzer, so now contains #226 and #227.

kloenk · 2021-04-29T17:14:22Z

I got my first 10 pings via the rust dummy_rs module implemented here. ip li add name test type dummy_rs still creates an oops though

kloenk · 2021-04-29T18:53:22Z

and ip li add name test type dummy_rs is working now 🎉🎉

Now I have to document everything. help is appreciated :-)

ojeda · 2021-05-08T04:13:43Z

drivers/android/rust_binder.rs

    description: b"Android Binder",
    license: b"GPL v2",
-    params: {},
 }


This should likely go into a different commit -- or is GitHub confused due to having several commits in the PR? Please rebase to have 1 commit anyway.

drivers/net/Kconfig

ojeda · 2021-05-08T04:14:45Z

drivers/net/dummy_rs.rs

+//! This is a demonstration of what a small driver looks like in Rust, based on drivers/net/dummy.c.
+//! This code is provided as a demonstration only, not as a proposal to mass-rewrite existing drivers in Rust


See my comment on #254 (comment).

I guess we can talk about this tomorrow in the call :)

I think that was the wording of @joshtriplett, while talking about this at our last call.

I'm not fixated on the exact wording, I just want to make sure that people don't see a Rust version of an existing C driver and assume we're trying to rewrite the world in Rust. In this context, we're providing samples of what drivers would look like; we're not writing replacements.

In the future, some driver maintainers may choose to use Rust in existing drivers, but that's up to those driver maintainers, not us.

drivers/net/dummy_rs.rs

ojeda · 2021-05-08T04:22:05Z

rust/helpers.c

 // See https://github.com/rust-lang/rust-bindgen/issues/1671
+#if !defined(CONFIG_ARM)


? Perhaps GitHub is confused -- please rebase.

Tried that yesterday, but not even the rust branch builds on my system currently. Something with exports.o and symbols

after a git clean -xfd (as make mrproper does not work #255 ) it builds the rebased version.
I pushed the rebased version, but I'm sot sure if/why it still makes this as a change.

Ah, now it is clear -- somehow you ended up with the comment moved outside of the #ifdef.

ojeda · 2021-05-08T04:22:38Z

rust/kernel/error.rs

+/// Used by the rtnl_link_ops macro to interface with C
+pub fn c_from_kernel_result<T>(r: KernelResult<T>) -> T


If it is not intended to be used by users, hide the docs.

ojeda · 2021-05-08T04:24:13Z

rust/kernel/net/device.rs

+        }
+    }
+
+    /// register_locked - register a network device if the RtnlLock is already hold


I guess this is copied from C docs, but do not put the name of the function etc.

ojeda · 2021-05-08T04:24:24Z

rust/kernel/net/device.rs

+    ///
+    /// # Safety
+    ///
+    /// caller must hold the RtnlLock and semaphore


Suggested change

/// caller must hold the RtnlLock and semaphore

/// Caller must hold the [`RtnlLock`] and semaphore.

And similar everywhere else.

ojeda · 2021-05-08T04:25:38Z

rust/kernel/net/netlink.rs

+    /*pub unsafe fn from_ptr(ptr: *const bindings::nlattr) -> &'static mut Self {
+        (ptr as *mut NlAttr).as_mut().unwrap()
+    }*/


I see a bunch of commented out code etc.

ojeda · 2021-05-08T04:28:22Z

Now I have to document everything. help is appreciated :-)

From the quick review I did above, I assume you are still working on this (i.e. there is a bunch of commented out code, SAFETY comments needed, etc.).

kloenk · 2021-05-08T10:01:31Z

rust/kernel/net/device.rs

+    }
+}
+
+impl<I: NetDeviceAdapter> Deref for NetDevice<I> {


Should we implement this Deref and DerefMut or should we consider this unsafe, as this exposes the direct bindings?

kloenk · 2021-05-08T19:55:39Z

To summarise what I understood in the meeting. I will remove the line saying it is just a test driver. I finish up the abstraction in the rust/ folder, than we will submit the rust folder, and if it is merged into upstream linux, I will submit the drivers/net/dummy_rs.rs file myself.
Or would you recommend another way?

ojeda · 2021-05-08T20:15:03Z

If you prefer, you can put it here if you want, too. Like with Binder, for the moment we have it in this tree to have everything in one place. While they will not be merged with the Rust support, I can submit it as an extra patch (with you as the author etc.) like I did with Binder from Wedson in the RFC.

@wedsonaf @TheSven73 since they have the other two potential drivers.

kloenk · 2021-05-08T20:25:29Z

If you prefer, you can put it here if you want, too. Like with Binder, for the moment we have it in this tree to have everything in one place. While they will not be merged with the Rust support, I can submit it as an extra patch (with you as the author etc.) like I did with Binder from Wedson in the RFC.

Sounds great. I will create a second commit like Wedson explained, so one with abstractions and one with the driver.
Author is not important for me, if it is easier with something else, that's also fine for me. I just looked at the C dummy driver and what is there as author.

ojeda · 2021-05-08T21:06:19Z

If you prefer, different PRs is also fine (and allows us to merge things bit by bit).

kloenk · 2021-05-08T21:25:00Z

I would prefer 2 commits. One pr is easier I guess, but two is also fine. Whatever you prefer.

alex · 2021-05-12T01:01:10Z

FYI: there's a merge conflict here.

soruh

There are quite a few places where I'd like more safety comments and the address functions in net/mod.rs could probably be safer (see my comment there).
(I didn't look at the changes to module.rs)

soruh · 2021-05-20T00:00:11Z

rust/kernel/net/ethtool.rs

+    pub get_ts_info: bool,
+}
+
+/// This trait does not include any functions.


These comments should probably be updated to reflect that this struct describes which methods are implemented by an implementation of EthToolOps not which ones are defined by the trait itself.

soruh · 2021-05-20T00:00:24Z

rust/kernel/net/ethtool.rs

+/// Operations table needed for ethtool.
+/// [`Self::TO_USE`] defines which functions are implemented by this type.
+pub trait EthToolOps<T: NetDeviceAdapter>: Send + Sync + Sized {
+    /// Struct [`EthToolToUse`] which signals which function are ipmlemeted by this type.


s/ipmlemeted/implemeted/

soruh · 2021-05-20T00:22:31Z

rust/kernel/net/rtnl.rs

+pub const RTNL_LINK_OPS_EMPTY: bindings::rtnl_link_ops = bindings::rtnl_link_ops {
+    list: bindings::list_head {
+        next: ptr::null::<bindings::list_head>() as *mut bindings::list_head,
+        prev: ptr::null::<bindings::list_head>() as *mut bindings::list_head,


This could use ptr::null_mut

soruh · 2021-05-20T00:24:07Z

rust/kernel/net/rtnl.rs

+
+    /// Get a pointer to this struct.
+    pub unsafe fn get_pointer(&self) -> *const bindings::rtnl_link_ops {
+        &self.0 as *const _ as *const bindings::rtnl_link_ops


the as *const _ seems redundant

Yes, but I think rust wants them, as this is not directly the right type for the view of rust

I think in this case you don't need it is since you're turning a &rtnl_link_ops into a *const rtnl_link_ops. I agree that the other ones are necessary.

(I could of course be wrong about that and it's ultimately not that important)

Could be that I missed .0 befor. And then you need this. Is it is valid, but rust doesn't know that.

That was probably the case, you can only do &T as *const T but not &U as *const T, even if U only contains a T.

Yes, as U has repr Transparent. It would also be valid, but rust isn't able to check that. That's the reason to go over *const _

But yeah, will change it.

soruh · 2021-05-20T00:25:40Z

rust/kernel/net/rtnl.rs

+
+    /// Deregister the op table from the kernel.
+    pub fn unregister(&self) {
+        let ptr = self as *const _ as *mut bindings::rtnl_link_ops;


this could be self.get_pointer() as *mut bindings::rtnl_link_ops

soruh · 2021-05-20T00:34:28Z

rust/kernel/net/skbuff.rs

+    /// Specifically, one should make absolutely sure that this function is
+    /// called before TX completion of this packet can trigger.  Otherwise
+    /// the packet could potentially already be freed.
+    pub fn tx_timestamp(&mut self) {


Otherwise the packet could potentially already be freed.

This sounds like this function should potentially be unsafe.

I think It couldn't, as then we don't have self anymore? But not completely sure right now

I don't follow completely, are you saying that this is indeed safe/you're not sure about it's safety or that this could't be an unsafe method?

If we're not certain about about the safety of a method, the default should really be making it unsafe.

soruh · 2021-05-20T00:36:53Z

rust/kernel/net/skbuff.rs

+    #[cfg(not(target_pointer_width = "64"))]
+    fn end_pointer(&self) -> *mut u8 {
+        let sk_reff = self.get_internal();
+        (sk_reff.end) as *mut u8


unnecessary parenthesis

soruh · 2021-05-20T00:41:31Z

drivers/net/dummy_rs.rs

+    }
+
+    fn start_xmit(skb: SkBuff, dev: &mut NetDevice<Self>) -> kernel::net::device::NetdevTX {
+        let mut skb = skb;


use fn start_xmit(mut skb: SkBuff, ... and remove the let mut skb = skb;.

soruh · 2021-05-20T00:42:34Z

drivers/net/dummy_rs.rs

+        dev.carrier_set(new_carrier);
+
+        Ok(())
+    }


Why does this return a Result if it can't fail?

soruh · 2021-05-20T00:43:37Z

drivers/net/dummy_rs.rs

+        unsafe {
+            kernel::bindings::strlcpy(
+                &(info.driver) as *const _ as *mut i8,
+                b"dummy_rs\0" as *const _ as *mut i8,


I think you could use the cstr! macro here

bjorn3 · 2021-05-20T14:38:53Z

rust/kernel/net/mod.rs

+/// Please note: addr must be aligned to u16.
+#[cfg(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)]
+pub unsafe fn is_zero_ether_addr(addr: *const u8) -> bool {
+    *(addr as *const u32) | (*((addr as usize + 4) as *const u16) as u32) == 0


I am pretty sure this will need to use ptr::read_unaligned. Even if the underlying cpu architecture allows efficient unaligned accesses using normal load instructions, LLVM can optimize based on the assumption that addr as usize % 4 == 0. For example it could vectorize in ways that would cause overlap if the address is not aligned to 4 bytes. Or it could optimize away manual alignment checks. By the way you can use addr.offset(4) instead of addr as usize + 4.

bjorn3 · 2021-05-20T14:46:47Z

rust/kernel/net/mod.rs

+    *(addr as *const u16)
+        | *((addr as usize + 2) as *const u16)
+        | *((addr as usize + 4) as *const u16)
+        == 0


I was thinking that

Suggested change

*(addr as *const u16)

| *((addr as usize + 2) as *const u16)

| *((addr as usize + 4) as *const u16)

== 0

*(addr as *const [u16; 3]) == [0; 3]

would be nicer to read. When I tested if LLVM would be able to optimize it I noticed the following: On x86_64 and aarch64 the current version uses 3 loads while the suggested version does 2 loads just like the CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS version. On armv7 this results in a branch + load from a constant pool for the suggested version while version uses 3 loads and no constant pool.

Because of this I think this suggestion should be used for CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to avoid UB at no perf cost, but not(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) should remain the same.

Would it maybe make sense for all of these methods to take a &[u8; 6] and be completely safe to call or is there an inherent reason we need to treat these values as u16s?

For CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS there would be no change. for not(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) that would require 6 loads (one for each u8) instead of 3 (one for each u16).

nbdd0121 · 2021-05-21T00:15:58Z

rust/kernel/net/device.rs

+
+unsafe impl<T: NetDeviceAdapter> Sync for NetDevice<T> {}
+
+impl<T: NetDeviceAdapter> NetDevice<T> {


Note that Rust's polymorphize support is not yet mature, so one copy of each function might be monomorphized for each T. Not a big concern here though.

nbdd0121 · 2021-05-21T00:25:29Z

rust/kernel/net/device.rs

+
+/// Iff flags
+#[repr(u32)]
+#[allow(non_camel_case_types)]


Any particular reason to use SCREAMING_SNAKE_CASE here?

nbdd0121 · 2021-05-21T00:26:30Z

rust/kernel/net/device.rs

+    use core::convert::{From, Into};
+    use core::ops::{Add, AddAssign, Deref, DerefMut, Sub, SubAssign};
+
+    /// Holds multiple flags to give to an interface via [`NetDevice::set_flag`].


This is likely to be very common in bindings. We may want a mini version of bitflags!.

Excluding tests and docs the bitflags crate is a little less than 600 lines big. bitflags is a slow moving crate and doesn't implement a huge set of features. I don't know what the policy on vendoring extern crates is, but I don't think there is much benefit of writing a mini version over just vendoring bitflags. At most it would save codegen of a couple of methods, though most methods are marked as #[inline] and thus only codegened when used.

nbdd0121 · 2021-05-21T00:29:16Z

rust/kernel/net/ethtool.rs

+}
+
+/// Represents which fields of [`struct ethtool_ops`] should pe populated with pointers for the trait [`EthToolOps`]
+pub struct EthToolToUse {


This also seems to be pretty common for bindings. Maybe we need to have a attribute macro. I'll think about a sensible design.

nbdd0121 · 2021-05-21T00:32:32Z

rust/kernel/net/device.rs

+        })
+    }
+
+    /// Return a refference to the private data of the [`NetDevice`].


s/refference/reference/g

bjorn3 · 2021-05-21T08:23:03Z

rust/kernel/net/ethtool.rs

+    }
+}
+
+/// Represents which fields of [`struct ethtool_ops`] should pe populated with pointers for the trait [`EthToolOps`]


Suggested change

/// Represents which fields of [`struct ethtool_ops`] should pe populated with pointers for the trait [`EthToolOps`]

/// Represents which fields of [`struct ethtool_ops`] should be populated with pointers for the trait [`EthToolOps`]

nit

ci: retry add-apt-repository to deal with network issues

ksquirrel · 2021-06-06T11:37:40Z

Review of 818532604b6d:

⚠️ This PR adds/deletes more than 500 lines.
❌ Commit 8185326: Private GitHub email address (@users.noreply.github.com) used for commit author.
⚠️ Commit 8185326: The commit committer does not match the commit author.
❌ Commit 8185326: The last line of the commit message must be a Signed-off-by and must match the submitter of the PR.
❌ Commit 8185326: Unknown tag. Known ones are: Acked-by, Cc, Co-developed-by, Fixes, Link, Reported-by, Reviewed-by, Signed-off-by, Suggested-by, Tested-by.

kloenk · 2021-07-10T09:52:24Z

Closing this, as it is quite outdated, and the commit history Is completely broken.

kloenk force-pushed the rust-netdevice branch from 28d4703 to b6e0242 Compare April 25, 2021 11:40

alex marked this pull request as ready for review April 25, 2021 15:03

alex marked this pull request as draft April 25, 2021 15:03

kloenk force-pushed the rust-netdevice branch from 2c7e3be to e6b2cb7 Compare April 25, 2021 15:25

kloenk mentioned this pull request Apr 28, 2021

Initialize field owner in file_operations (issue #214) #228

Closed

kloenk force-pushed the rust-netdevice branch 4 times, most recently from 14ea91c to 6e3bed3 Compare April 29, 2021 15:58

kloenk force-pushed the rust-netdevice branch from de0992b to deda3f0 Compare April 29, 2021 20:24

kloenk changed the title ~~WIP: Rust NetDevice and NetDeviceOperationsVtable struct~~ Rust NetDevice and NetDeviceOperationsVtable struct Apr 29, 2021

kloenk force-pushed the rust-netdevice branch from deda3f0 to 4163634 Compare April 30, 2021 08:54

kloenk marked this pull request as ready for review April 30, 2021 10:55

kloenk force-pushed the rust-netdevice branch from 4163634 to 5d43f1f Compare April 30, 2021 11:30

ojeda reviewed May 8, 2021

View reviewed changes

kloenk force-pushed the rust-netdevice branch from 5d43f1f to 07fc327 Compare May 8, 2021 09:43

kloenk commented May 8, 2021

View reviewed changes

kloenk force-pushed the rust-netdevice branch 2 times, most recently from f3ddd55 to 018db0a Compare May 8, 2021 11:05

kloenk force-pushed the rust-netdevice branch 2 times, most recently from 6ca2cb4 to f973478 Compare May 12, 2021 14:05

This comment has been minimized.

Sign in to view

soruh reviewed May 20, 2021

View reviewed changes

bjorn3 reviewed May 20, 2021

View reviewed changes

nbdd0121 reviewed May 21, 2021

View reviewed changes

bjorn3 reviewed May 21, 2021

View reviewed changes

This was referenced May 27, 2021

Depend on syn #302

Closed

Implement a #[vtable] macro #322

Merged

Merge pull request Rust-for-Linux#360 from Rust-for-Linux/alex-patch-1

8185326

ci: retry add-apt-repository to deal with network issues

kloenk force-pushed the rust-netdevice branch from f973478 to 8185326 Compare June 6, 2021 11:37

kloenk closed this Jul 10, 2021

kloenk mentioned this pull request Jul 10, 2021

DRAFT: [RFC]: Add rust net_device wrappers #439

Draft

		//! This is a demonstration of what a small driver looks like in Rust, based on drivers/net/dummy.c.
		//! This code is provided as a demonstration only, not as a proposal to mass-rewrite existing drivers in Rust

		// See https://github.com/rust-lang/rust-bindgen/issues/1671
		#if !defined(CONFIG_ARM)

		/// Used by the rtnl_link_ops macro to interface with C
		pub fn c_from_kernel_result<T>(r: KernelResult<T>) -> T

	/// caller must hold the RtnlLock and semaphore
	/// Caller must hold the [`RtnlLock`] and semaphore.


		unsafe impl<T: NetDeviceAdapter> Sync for NetDevice<T> {}

		impl<T: NetDeviceAdapter> NetDevice<T> {

	/// Represents which fields of [`struct ethtool_ops`] should pe populated with pointers for the trait [`EthToolOps`]
	/// Represents which fields of [`struct ethtool_ops`] should be populated with pointers for the trait [`EthToolOps`]

Rust NetDevice and NetDeviceOperationsVtable struct #208

Rust NetDevice and NetDeviceOperationsVtable struct #208

Conversation

kloenk commented Apr 19, 2021 • edited Loading

kloenk commented Apr 20, 2021

adamrk commented Apr 21, 2021

kloenk commented Apr 21, 2021

kloenk commented Apr 22, 2021

kloenk commented Apr 25, 2021 • edited Loading

kloenk commented Apr 29, 2021

kloenk commented Apr 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda commented May 8, 2021

Choose a reason for hiding this comment

kloenk commented May 8, 2021

ojeda commented May 8, 2021 • edited Loading

kloenk commented May 8, 2021

ojeda commented May 8, 2021

kloenk commented May 8, 2021

alex commented May 12, 2021

This comment has been minimized.

soruh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kloenk May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksquirrel commented Jun 6, 2021

kloenk commented Jul 10, 2021

kloenk commented Apr 19, 2021 •

edited

Loading

kloenk commented Apr 25, 2021 •

edited

Loading

ojeda commented May 8, 2021 •

edited

Loading

kloenk May 20, 2021 •

edited

Loading

bjorn3 May 20, 2021 •

edited

Loading