Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Example for custom generic netlink family with custom attributes #103

Merged
merged 4 commits into from
Mar 13, 2021
Merged

Example for custom generic netlink family with custom attributes #103

merged 4 commits into from
Mar 13, 2021

Conversation

phip1611
Copy link
Contributor

@phip1611 phip1611 commented Dec 5, 2020

I'd like to add an example on how to use neli with custom properties for custom generic netlink families. I use this at an uni project where I develop a kernel module (which registers a custom generic netlink family), a userland component in Rust (using neli), and a other userland component using libnl. Therefore I can ensure that my approach works with the kernel / other netlink implementations.

Let's assume we have a custom kernel driver that registers a "your_family_name_here" family. The subject of our problem knows the operations/commands echo, create device, and delete device. It also knows the attributes msg (string) and device name (string).

In this example (see code) an echo message is send to the kernel and the reply received:

PS: Example kernel module that fulfills this contract: https://github.com/phip1611/generic-netlink-user-kernel-rust

PPS: When I started doing this project (with neli 0.4.3) I spend so much time on this. Therefore I'd love to have an easy example in this great lib so that others can profit.

Let me know what you think :)

@phip1611
Copy link
Contributor Author

phip1611 commented Dec 5, 2020

Of course this will not build/terminate in any CI system because the kernel module doesn't exist there. But it will fail, not stuck in a loop or so.

sock.recv().expect("Should receive a message").unwrap();

// According to Netlink Linux kernel code nl_type is either 0x2 (error) or our family id (ok)
if res.nl_type == 0x2 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I have to refactor this. Looks like this check is already covered by sock.recv(). It returns Err if nl_type is 0x2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, neli tells The buffer was not large enough to complete the deserialize operation. Doesn't look what I would neli expect to do. Technically an "well formed error message" is okay but not error

@phip1611
Copy link
Contributor Author

phip1611 commented Dec 5, 2020

Probably found a general neli problem that is not related to this specific PR. I just want to note it here:
If I reply from kernel with a netlink error message (nl_type in netlink-header is NLMSG_ERR (0x2)) then ´socket.recv()´ returns ERR because of The buffer was not large enough to complete the deserialize operation. My kernel module added no payload to the response, so no struct nlmsgerr. I don't know if this is right.. this is valid (generic) netlink behaviour. A well formed error message from the kernel should be okay, even if it doesn't contain any payload.. or am I wrong? Especially because struct nlmsgerr comes from "the old netlink times" without generic netlink. I think signaling an error with custom attributes is much better. The lib shoud support that.. but this is subject to another issue/PR.

Netlinks man page also encourages developers to use NLMSG_ERR to signalize error.
.recv() should only return Err if it couldn't parse the buffer. (which is in this case kinda true because neli is probably having a problem that the massage having no payload, but it should be parsed as Ok)

@jbaublitz
Copy link
Owner

@phip1611 Thanks for the report! So just to confirm, you returned an error packet with no nlmsgerr payload? According to the netlink manpage, this does not appear to be valid behavior at this point in time.

NLMSG_ERROR message signals an error and the payload contains an nlmsgerr structure

from here.

Unless you can give me a specific link to documentation that specifies that this payload is optional, I think this is a problem in your implementation, not in mine.

@jbaublitz
Copy link
Owner

I am happy to create an issue to improve error messaging if that would be helpful for you. I've also bumped into some situations where the error messaging is not adequate and I'd like to improve the error provided to the user in certain cases.

@phip1611
Copy link
Contributor Author

phip1611 commented Dec 6, 2020

@jbaublitz You are right. I'm not a netlink expert and I just tried to use it in a way that it works for me. My Kernel module didn't include struct nlmsgerr in the payload. I was not following the standard.

But can you imagine a more convenient way to recognize the absence of this struct, when nl_type is NLMSG_ERR? Because the current message "The buffer was not large enough to complete the deserialize operation" is not effective.

PS: I update my kernel module to include the struct in the payload in error case. I edit this comment when I have an update on this.

@jbaublitz
Copy link
Owner

Perfect! I'll open an issue and tag you in it so we can discuss details.

@jbaublitz jbaublitz mentioned this pull request Dec 7, 2020
@jbaublitz
Copy link
Owner

I have a few comments on the code so far. I'll review this soon and request some changes just so that people looking at the code are guided towards neli best practices when they look at the example.

@jbaublitz jbaublitz assigned jbaublitz and phip1611 and unassigned jbaublitz Dec 7, 2020
@jbaublitz jbaublitz self-requested a review December 7, 2020 02:16
Copy link
Owner

@jbaublitz jbaublitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some initial notes.

Comment on lines 12 to 26
use neli::consts::nl::Nlmsg;
use neli::consts::nl::{NlmF, NlmFFlags};
use neli::consts::socket::NlFamily;
use neli::genl::{Genlmsghdr, Nlattr};
use neli::nl::{NlPayload, Nlmsghdr};
use neli::socket::NlSocketHandle;
use neli::types::{Buffer, GenlBuffer};
use neli::utils::U32Bitmask;
use neli::Nl;
use std::process;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you switch this to nested imports? I've been meaning to put out some formal coding style guidelines but haven't gotten around to that quite yet.

sock.recv().expect("Should receive a message").unwrap();

// According to Netlink Linux kernel code nl_type is either 0x2 (error) or our family id (ok)
if res.nl_type == u16::from(Nlmsg::Error)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neli has special handling for the Nlmsg::Error value as of 0.5.0. This value indicates either an actual error if the error code is non-zero or an ACK if the error code is 0. neli will return these as values of either NlPayload::Ack or NlPayload::Err in the resulting Nlmsghdr. In the error case, recv() will detect this as an error and return it as an Err(Nlmsgerr) value. Can you explain a little bit more why you chose to do the checking manually here? Does this use case not work for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, apparently I was using the neli lib wrong in my uni project and therefore also in this demo.
I didn't had a proper example at the first place that fit my use case, therefore I just tried to figure everything out by myself. I'll improve the code and let you know if it works better. @jbaublitz

Comment on lines 135 to 143
let received = attr_handle.get_attribute(FoobarAttribute::Msg).unwrap();
let received = String::deserialize(received.nla_payload.as_ref()).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be more concisely represented by attr_handle.get_attr_payload_as::<String>(FoobarAttribute::Msg).

@phip1611
Copy link
Contributor Author

@jbaublitz FYI I'll fix this in the next weeks, I didn't forget it. I'm having a deadline for an uni project this sunday (where I use neli :))

@phip1611
Copy link
Contributor Author

phip1611 commented Feb 10, 2021

@jbaublitz Sorry it took a while, I updated the PR. Any more comments on the code? Of course I tested it and it's working.

PS: I don't get why this fails: https://github.com/jbaublitz/neli/pull/103/checks?check_run_id=1871771582

@jbaublitz
Copy link
Owner

I just reran the failed test and realized that this is a new clippy error in my code. I'll fix that.

@jbaublitz jbaublitz mentioned this pull request Feb 12, 2021
@jbaublitz
Copy link
Owner

@phip1611 Clippy checks are now optional in CI. If you rebase onto master, you should be good to go.

@phip1611
Copy link
Contributor Author

@jbaublitz done; pushed the rebased branch

Copy link
Owner

@jbaublitz jbaublitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this last change is made, I'll merge this. Thanks for putting so much effort into the documentation!

examples/custom_generic_nl_family_custom_types.rs Outdated Show resolved Hide resolved
@phip1611
Copy link
Contributor Author

@jbaublitz I fixed it.

@phip1611 phip1611 requested a review from jbaublitz March 4, 2021 11:51
@jbaublitz jbaublitz merged commit ac03202 into jbaublitz:master Mar 13, 2021
@phip1611 phip1611 deleted the example-custom-types branch March 15, 2021 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants