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

Nflog support MVP #48

Merged
merged 1 commit into from
Oct 30, 2019
Merged

Nflog support MVP #48

merged 1 commit into from
Oct 30, 2019

Conversation

vorner
Copy link
Contributor

@vorner vorner commented Oct 12, 2019

Hello

This adds the support for basic use of the nflog protocol. There are still some parts missing, but I'd prefer to have a feedback about it sooner than later and I tried to make sure all the missing pieces can be added without breaking API.

I've also tried to structure it in a way that makes it possible/natural to add all the other netfilter protocols over time (NFQUEUE, CONNTRACK).

There are, however, few things I'm not entirely happy about or don't know how to handle properly. I'll probably open separate issues to discuss them one by one, if there's some nice solution for them. But I'll still list my worries here first.

  • When receiving messages over the socket, I have to specify into what type I want them to deserialize. But this might depend on the type. This can be seen on the added example, sometimes I get a message with type 3 ‒ which I think corresponds to NLMSG_DONE. I'm not sure if I should make it part of the new NetfilterMsg enum, but that would still not help much, because it would still try to deserialize the rest as LogPacket. This doesn't currently fail, but is kind of wrong. It would be great if I could somehow specify a nl-type => parse type relation. Unlike NlAttr<_, Vec<u8>>, the Nlmsghdr<_, Vec>` doesn't have methods to parse as one or another type.
  • Some of the structs/messages are ever sent in only one direction. So it makes no sense to provide eg. serialize for the LogPacket. For now, I've put unimplemented in them, but it kind of doesn't feel right. Would it make sense to split the Nl trait into reading and writing halves?
  • The crate uses the Vec type quite generously. For example, when parsing a message, it allocates a Vec as the receive buffer, then it allocates a Vec for all the attributes and, as they have diffferent types, each one is a smaller Vec... As the Vec contains a heap allocation, it might turn somewhat expensive. This probably doesn't matter for things like manipulating the routing table, but with the NFLOG, the number of packets received per second can be quite huge. It would be great if there was a way to use the library with smaller number of allocations (some of my ideas could get down to 0 per packet).
  • The impl_* macros don't want to let me put documentation onto the individual variants. I've tried to poke at them a little bit, but I've managed to only break existing code, so I've given up for now.

Anyway, if I'm missing anything important here, or if there's any way to improve the code or interface, I'm happy to hear it. I'd then add some fixup commits and rebuild the branch before merging.

@vorner
Copy link
Contributor Author

vorner commented Oct 12, 2019

Linking here #37, the macros...

@vorner
Copy link
Contributor Author

vorner commented Oct 12, 2019

CI can't compile libc :-(.

@vorner
Copy link
Contributor Author

vorner commented Oct 13, 2019

Note to self. I've just discovered methods like u32::to_be. This means all the htons are not necessary and I'll get rid of them in a fixup some time soonish.

@jbaublitz
Copy link
Owner

jbaublitz commented Oct 18, 2019

Restarting CI here too.

@jbaublitz
Copy link
Owner

CI is fixed.

src/consts/netfilter.rs Show resolved Hide resolved
src/netfilter.rs Show resolved Hide resolved
src/socket.rs Outdated
@@ -336,9 +336,9 @@ impl NlSocket {
Some(ref mut b) => Nlmsghdr::deserialize(b)?,
None => unreachable!(),
};
if self.pid.is_none() {
if self.pid.is_none() && msg.nl_pid != 0 {
Copy link
Owner

Choose a reason for hiding this comment

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

Please move the socket changes into a separate PR. I'm happy to merge those before this one. I agree that PID tracking is very broken and I'm looking to fix that in v0.5.0 but am happy to turn this off for now in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've submitted the commit as #57. It's the same one, so it should disappear from the diff here once that one is merged into master.

Copy link
Owner

Choose a reason for hiding this comment

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

Excellent thank you! I want to think through that one a little more before merging, hence the separate PR. Once we figure that out, I'll ask you to rebase onto master so that you pull in whatever we decide on and then I'll merge this one.

@jbaublitz
Copy link
Owner

Hi @vorner, thanks for your other contribution. I've merged that PR so can you rebase onto master and force push here? I'll do one final review before merging.

Support receiving packets from the kernel over nflog. Some parts are not
yet implemented ‒ conntrack integration, hardware headers...

Closes jbaublitz#38.
@jbaublitz jbaublitz merged commit 0abd331 into jbaublitz:master Oct 30, 2019
@vorner vorner deleted the nflog branch November 17, 2019 19:58
bors added a commit to rust-lang/libc that referenced this pull request Nov 19, 2019
Nfnetfilter log

This adds the constants from linux/netfilter/nfnetlink.h and nfnetlink_log.h. These are the files I need for jbaublitz/neli#48. After this gets in, I'd like to follow-up with the other nfnetlink_*.h files too, as I'd like to extend neli with further protocols in the future, but I want to do a smaller PR first to see if there are some things to tweak.

I've noticed similar netfilter constants are also in the android subfolder, therefore I'm adding them there too (I don't like the copy-pasting, but it seems the other ones are already copy-pasted). I assume the test will catch it if anything is different on that platform.
bors added a commit to rust-lang/libc that referenced this pull request Nov 20, 2019
Nfnetfilter log

This adds the constants from linux/netfilter/nfnetlink.h and nfnetlink_log.h. These are the files I need for jbaublitz/neli#48. After this gets in, I'd like to follow-up with the other nfnetlink_*.h files too, as I'd like to extend neli with further protocols in the future, but I want to do a smaller PR first to see if there are some things to tweak.

I've noticed similar netfilter constants are also in the android subfolder, therefore I'm adding them there too (I don't like the copy-pasting, but it seems the other ones are already copy-pasted). I assume the test will catch it if anything is different on that platform.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants