Use UDP len when calculating UDP checksums

[thj]

Summary

The length of the IP payload is normally equal to the UDP length, UDP Options
(draft-ietf-tsvwg-udp-options-02) suggests using the difference between IP
length and UDP length to create space for trailing data.

Correct checksum length calculation to use the UDP length rather than the IP
length when not offloading UDP checksums.

Test Plan

On local links UDP checksums are frequently not validated. Wireshark can be
configured to force checking of UDP checksums via
Edit->Preferences->Protocols->UDP->Validate the UDP checksum if possible

To observe issue:

There is a patch here[1] that adds a simple trailing data protocol controlled
via the net.inet.udp.udp_trailing_data sysctl.

On a kernel patched with only the trailing data patch enable
udp_trailing_data, send some datagrams (i.e. nc -u host port). Wireshark will
show that UDP checksums fail with trailing data.

Fix tested with re, igb and vtnet devices. There is a small c tool here[2] to
UDP datagrams with IP Options.

Getting trailing data to work with checksum offload appears to be a large task,
the trailing data patch disables offload for datagrams with trailing data.

[1]: https://people.freebsd.org/~thj/diffs/trailingdata.diff
[2]: https://people.freebsd.org/~thj/examples/ipoptsend.c

[jtl]

Sorry it took me a while to look at this. See comments in-line.

sys/netinet/ip_output.c

936 ↗

(On Diff #41954)

style(9): alphabetical order (cklen, csum, offset)

Also, initializers should appear after the declarations. (Also, thanks to style(9).)

942 ↗

(On Diff #41954)

Is the UDP header guaranteed to be in the first mbuf? I don't think there is any explicit guarantee, although it might be true today.

You could be guaranteed to get the right bytes by either using m_pullup(m, offset + sizeof(struct udphdr)) or m_copydata(m, offset + offsetof(struct udphdr, uh_ulen), 2, cklen). Given the fact that this function has not previously modified mbufs, m_pullup() would require changing the calling functions. For that reason, m_copydata() is probably the better of these two choices.

(There are other ways to do this. For example, you should check m->m_len and only use m_copydata() if necessary. In any case, I think you need to ensure the data is in the first mbuf, rather than just assuming it to be true.)

944 ↗

(On Diff #41954)

Should the second argument just be cklen?

[thj]

sys/netinet/ip_output.c

944 ↗

(On Diff #41954)

Alas, in_cksum_skip starts by subtracting offset from len so we must included it in length field.

[thj]

• Check if udp header is in first mbuf, if not copy in udp len. Address style issues

[jtl]

I'm sorry it took me so long to review this.

Please add:
Approved by: jtl (mentor)
when you commit.

sys/netinet/ip_output.c

944 ↗

(On Diff #41954)

Ah, good call!