Skip to content

Support Go netlink sockets#3441

Merged
stevenengler merged 1 commit intoshadow:mainfrom
haxxpop:go-netlink
Dec 21, 2024
Merged

Support Go netlink sockets#3441
stevenengler merged 1 commit intoshadow:mainfrom
haxxpop:go-netlink

Conversation

@haxxpop
Copy link
Copy Markdown
Contributor

@haxxpop haxxpop commented Nov 18, 2024

When you get the interfaces in Go using net.Interfaces(), it uses netlink sockets to do so.

However, the messages sent by Go sometimes are too short and just ignore some fields.

For example, the RTM_GETLINK message sent by Go looks like this: [17, 0, 0, 0, 18, 0, 1, 3, 1, 0, 0, 0, 0, 0, 0, 0, 0]

You can see that nlmsghdr took 16 bytes already and it left one byte at the end as a payload.

In fact, the length should be at least 32 not 17 (16 bytes for nlmsghdr and 16 bytes for ifinfomsg).

This PR pads the message with zeroes until it reaches the minimum length.

I'm not sure how Linux handles it, but I think it's not worth spending time to investigate more.

Before submitting your pull request, please see our documentation about contributing to Shadow:

https://shadow.github.io/docs/guide/contributing.html

@github-actions github-actions bot added Component: Libraries Support functions like LD_PRELOAD and logging Component: Testing Unit and integration tests and frameworks Component: Main Composing the core Shadow executable Component: Build Build/install tools and dependencies labels Nov 18, 2024
@stevenengler
Copy link
Copy Markdown
Contributor

stevenengler commented Nov 18, 2024

One of the issues with using third-party libraries for handling kernel data structures that we've continually run into in Shadow is that they're always designed for sending data to the kernel, whereas in Shadow we need to receive data from applications. Most rust libraries are not designed to handle edge cases like this. This is common in the networking syscalls (especially socket addresses). I think there are a few other syscalls too where we need to be able to handle fewer bytes than the size of the struct.

In general, I think we try not to add "fake zero bytes" since they may have some unintended meaning. Instead it's usually better to only use the fields if they're given (if they're within the bounds given by len) rather than pretending that they're zero. For example in the rseq syscall handler, the size of the rseq struct can change between kernel versions so we use the field_project macro for this:

let Some((cpu_id, cpu_id_start)) = field_project!(rseq_bytes, rseq, (cpu_id, cpu_id_start))
else {
return Err(Errno::EINVAL);
};

(We have a test using this with nlmsghdr)

#[test]
fn field_project_1() {
let mut foo: libc::nlmsghdr = shadow_pod::zeroed();
let foo_bytes = unsafe { shadow_pod::as_u8_slice_mut(&mut foo) };
let foo_nlmsg_type = field_project!(foo_bytes, libc::nlmsghdr, nlmsg_type).unwrap();
foo_nlmsg_type.write(10);
assert_eq!(foo.nlmsg_type, 10);
}

But I don't think neli gives us this option. Enough bytes need to be given to fill the entire Ifaddrmsg struct. So I think zero-filling the struct is the only option here, and it makes sense to go with that approach.

@stevenengler
Copy link
Copy Markdown
Contributor

@sporksmith Do you agree with my comments here? We've discussed this topic (fewer bytes than the size of the struct) before I think, but I don't remember what your thoughts were.

@sporksmith
Copy link
Copy Markdown
Contributor

@sporksmith Do you agree with my comments here? We've discussed this topic (fewer bytes than the size of the struct) before I think, but I don't remember what your thoughts were.

Yup, what you wrote makes sense to me

Copy link
Copy Markdown
Contributor

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the tests! I have one last change request in the comments, then I think it's ready. Then you can rebase on main, squash, and I'll merge it.

@haxxpop haxxpop requested a review from stevenengler December 20, 2024 15:11
@haxxpop
Copy link
Copy Markdown
Contributor Author

haxxpop commented Dec 20, 2024

@stevenengler Can you confirm that it's looking good? and then I will rebase and squash it.

@stevenengler
Copy link
Copy Markdown
Contributor

@stevenengler Can you confirm that it's looking good? and then I will rebase and squash it.

Yep looks good! Thanks for the changes. It's ready for rebase and squash.

When you get the interfaces in Go using `net.Interfaces()`, it uses
netlink sockets to do so.

However, the messages sent by Go sometimes are too short and just ignore
some fields.

For example, the RTM_GETLINK message sent by Go looks like this:
[17, 0, 0, 0, 18, 0, 1, 3, 1, 0, 0, 0, 0, 0, 0, 0, 0]

You can see that `nlmsghdr` took 16 bytes already and it left one byte
at the end as a payload.

In fact, the length should be at least 32 not 17 (16 bytes for
`nlmsghdr` and 16 bytes for `ifinfomsg`).

This PR pads the message with zeroes until it reaches the minimum
length.

I'm not sure how Linux handles it, but I think it's not worth spending
time to investigate more.
@stevenengler stevenengler merged commit 3dfee2f into shadow:main Dec 21, 2024
@haxxpop haxxpop deleted the go-netlink branch December 22, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Build Build/install tools and dependencies Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants