Page MenuHomeFreeBSD

Add UDP receive probes
ClosedPublic

Authored by tuexen on Jun 28 2018, 2:28 PM.

Details

Summary

When a UDP packet is received and there is no endpoint to consume it, no UDP receive probe is fired. This is fixed by this patch. The corresponding dtrace test has been updated to verify this.

When a UDP/IPv4 packet is received and the TTL is smaller than the minimum TTL required by the endpoint, no UDP receive probe is fired. This is different from the TCP/IPv4 case. To improve consistency, fix this also by this patch.

Test Plan

Use the attached dtrace script udpio.d from the udp_dtrace man page and use nc -u 127.0.0.1 1234 and nc -u ::1 1234 to generate UDP packets assuming there is not endpoint on port 1234. The dtrace script should show the packets as sent and received.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tuexen created this revision.Jun 28 2018, 2:28 PM
markj accepted this revision.Jun 28 2018, 9:00 PM

It would be nice to add a test for this to cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip, but I don't think that should block the commit of this change. I'm happy to help with that if needed.

tuexen added a comment.EditedJun 28 2018, 9:13 PM

Thanks for pointing me to the tests. I wasn't aware of them. Is there any documentation available how to run and/or write them?

Adding tests makes sense since I'm also adding some probes to TCP and finally will add support for SCTP and UDPLite for which
I'll add then tests, too.

markj added a comment.Jun 28 2018, 9:18 PM

Thanks for pointing me to the tests. I wasn't aware pf them. Is there any documentation available how to run and/or write them?
Adding tests makes sense since I'm also adding some probes to TCP and finally will add support for SCTP and UDPLite for which
I'll add then tests, too.

For the time being you need to set WITH_DTRACE_TESTS=YES in src.conf. Then, they get built and installed from cddl/usr.sbin/dtrace/tests. After adding tests, you need to run cddl/usr.sbin/dtrace/tests/tools/genmakefiles.sh to update the makefiles.

The tests can be run using kyua. They get installed to /usr/tests/cddl/usr.sbin/dtrace/tests. Most of them depend on the shells/pdksh port.

Simple question: And how do I run these test using kyua?

markj added a comment.Jun 28 2018, 9:32 PM

Simple question: And how do I run these test using kyua?

As root (required for dtrace), just run "kyua test" in the subdirectory containing the tests you wish to run. To run all dtrace tests, run it from /usr/tests/cddl/usr.sbin/dtrace.

tuexen updated this revision to Diff 45357.Jul 16 2018, 9:41 AM

Update dtrace test for UDP to reflect this change. In particular, the reception of the UDP packet should be counted.

tuexen edited the summary of this revision. (Show Details)Jul 16 2018, 9:43 AM
markj accepted this revision.Jul 16 2018, 1:28 PM

This looks ok to me. It would be nice to amend the udp dtrace man page to state under what conditions the receive probe may fire, but I think that can be done as a separate revision.

tuexen updated this revision to Diff 45378.Jul 16 2018, 5:50 PM

Add explicit text to the man page describing when the receive probe fires.

Herald added 1 blocking reviewer(s): gnn. · View Herald TranscriptJul 16 2018, 5:50 PM
Herald added a reviewer: manpages. · View Herald Transcript

@markj I'll commit the changes to the dtrace scripts separately from the changes to the UDP source code. Should the change of the man page go with the first commit, with the second commit or be in a third commit?

markj accepted this revision.Jul 16 2018, 5:54 PM

@markj I'll commit the changes to the dtrace scripts separately from the changes to the UDP source code. Should the change of the man page go with the first commit, with the second commit or be in a third commit?

I think the man page change and kernel change can be committed together. Thanks!

share/man/man4/dtrace_udp.4
27 ↗(On Diff #45378)

I think the "th" suffix is supposed to be omitted.

dteske added inline comments.Jul 16 2018, 6:43 PM
share/man/man4/dtrace_udp.4
51 ↗(On Diff #45378)

I would like to request an Oxford comma after "invalid", followed by a line-break before "or" (e.g., below):

the length is invalid,
or the checksum is wrong.

tuexen updated this revision to Diff 45388.Jul 16 2018, 8:04 PM

Address comments related to the man-page.

tuexen added inline comments.Jul 16 2018, 8:06 PM
share/man/man4/dtrace_udp.4
27 ↗(On Diff #45378)

Fixed.

51 ↗(On Diff #45378)

Fixed.

If it's OK, I'd like to run some tests with dwatch to make sure that I don't need to patch my code for any change in functionality. I don't suspect this to be the case, but just want to make sure. If you're fine to wait another day or two for my testing, I'll definitely stamp approval

@dteske I'm fine, go ahead and test.

@dteske Did you had a chance to test it?

rrs accepted this revision.Jul 19 2018, 3:36 PM

@dteske Did you had a chance to test it?

Not yet. Working on it today. It's not just a 30-second test considering I have to recompile the kernel in the middle of the A/B test.

dteske added a comment.EditedJul 19 2018, 6:52 PM

I have used both your attached udpio.d (taken from dtrace_udp(4) in the EXAMPLES section) as well as:

dwatch -X udp

in the base system to confirm the pre-patch state. A probe fires when nc in the given test case of:

nc -u 127.0.0.1 1234

sends the datagram but no probe fires for the receive.

Going further, I compared UDP to TCP. The command:

dwatch -t 'this->lport==1234 || this->rport==1234' -FX tcp tcp:::send,tcp:::receive

combined with the test:

nc 127.0.0.1 1234

shows that the TCP receive probe fires in the context of the kernel interrupt thread (intr, commonly pid 12).

After patch, I will expect dwatch to catch intr processing the datagram. Now I have to recompile my kernel for the next test.

dteske added inline comments.Jul 19 2018, 7:02 PM
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4localudp.ksh
48 ↗(On Diff #45388)

It looks like the original (pre-patch) code had a typo. Could you do us a favor and fix the typo while you're here? s/elicts/elicits/

tuexen added inline comments.Jul 19 2018, 7:06 PM
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4localudp.ksh
48 ↗(On Diff #45388)

Have it in my local copy...

dteske added a comment.EditedJul 19 2018, 7:11 PM

Aside from testing with what comes with FreeBSD base, I also wanted to make sure that this change would be handled well by my DTrace network visualizer (sysutils/dwatch-gource in ports).

dteske@vm11 ~ $ dwatch -t 'this->family=="udp"' -X gource-net-raw
INFO Sourcing gource-net-raw profile [found in /usr/local/libexec/dwatch]
INFO Watching 'tcp:::debug-user, tcp:::state-change, udp:::send, fbt::soreceive_dgram:entry' ...
INFO Setting test: this->family=="udp"
2018 Jul 17 09:02:24 1001.1001 nc[98845]: 1531843344 udp 127.0.0.1:62235 SEND 127.0.0.1:1234 12 bytes

Here, I expect that after the patch I will still not see the RECV (desired) because it is not processed by an application (my network visualizer wants to know the application that is receiving the datagram and since udp:::receive only fires in the context of execname == "intr", the gource-net-raw profile instsead watches fbt::soreceive_dgram:entry with a predicate that indicates it is talking to the same socket).

dteske accepted this revision.Jul 20 2018, 3:40 AM

Testing was successful. Everything worked as expected. Cheers.

Worth mentioning, I tested on stable/11 (for MFC consideration).

This revision was not accepted when it landed; it landed in state Needs Review.Jul 20 2018, 3:32 PM
This revision was automatically updated to reflect the committed changes.