-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add UDP probes in anticipation of DNS monitoring #206
Conversation
We could probably do the set_state/get_state trick that we have in Helpers.h which maintains state between entry and ret so that we get access to the input argument pointers in the handler for ret.
I believe this is where TC runs on the RX path: Another thing is at this early L2 stage we don't even know if the packet will actually reach the application, there is quite a lot of filtering and drop scenarios in between. So I think I'm leaning towards the hooks as they are done in this PR if we can easily get packet data there and parse it. I assume we want to filter for port 53 in the kernel and avoid creating events for other UDP packets otherwise there could be a lot. |
@stanek-michal so, I did some tinkering over the three-day weekend and came to a lot of the conclusions you did, namely that we could use a state getter/setter for kretprobes, and also the problem with finding the userspace pid with a TC/classifier probe. I'm leaning towards the getter/setter with kretprobe idea for this, since it's a little simpler (we don't need some other mechanism for tracking what userland process owns the pid). |
Taking this out of draft since I think this is a fairly decent framework going forward if/when we want to parse DNS packets. |
This won't work for unconnected sockets, as the socket doesn't have an address, if you run the following program you will see the packets don't show up. Worth noting that unconnected udp sockets are likely more common than connected. You can use the following program to verify:
As I mentioned to you yesterday, I think this is still not a good place to tap, I'll do a proper review later tonight and have more comments, but if you use the program above with events trace you can spot the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just a bit of clean up left I think. Oh, and the tests are not passing.
This reverts commit fbf1608.
Alright, so, reverted the commit that swapped over to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs some work, but it's a step in the right direction imho.
It would be better to clean up the unused bits before asking for a review.
As we discussed yesterday, it's likely you want to use the varlen thingy to express the packet instead of hard coding it.
Let me know if anything is not clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing an error case in sendto, and just make the test more idiomatic.
I wonder, why not use the udpsend I wrote in the PR? that way you can test both connected and unconnected, not that it makes a difference now.
At any rate I'm fine with this given this changes.
@haesbaert totally forgot about the earlier udp send example you wrote, so I switched to that. |
I haven't tested the last iteration but looks good to me, good work! |
see: https://datatracker.ietf.org/doc/html/rfc1035#autoid-40
This PR adds basic support for DNS monitoring, using the
ip_send_skb
andskb_consume_udp
functions in the kernel networking stack. The logic here is very basic, and includes no parsing of actual DNS data; after spending the better part of the day reading RFCs and looking at other implementations, I've decided that parsing actual DNS requests/responses is complex enough (and risky enough) that we should probably use an external library for parsing DNS data. It's one of those "getting it 80% working is 20% of the code" protocols.As a result of this, this PR just creates a DNS packet that just provides basic network metadata and and a buffer for the packet data.