Page MenuHomeFreeBSD

pf: Fix TSO issues
ClosedPublic

Authored by kristof on Oct 2 2015, 10:05 AM.

Details

Reviewers
eri
sbruno
Group Reviewers
network
Commits
rS289316: pf: Fix TSO issues
Summary

In certain configurations (mostly but not exclusively as a VM on Xen) pf
produced packets with an invalid TCP checksum.

The problem was that pf could only handle packets with a full checksum. The
FreeBSD IP stack produces TCP packets with a pseudo-header checksum (only
addresses, length and protocol).
Certain network interfaces expect to see the pseudo-header checksum, so they end
up producing packets with invalid checksums.

To fix this stop calculating the full checksum and teach pf to only update TCP
checksums if TSO is disabled or the change affects the pseudo-header checksum.

Sponsored by: RootBSD

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

kristof retitled this revision from to pf: Fix TSO issues.Oct 2 2015, 10:05 AM
kristof updated this object.
kristof edited the test plan for this revision. (Show Details)
kristof set the repository for this revision to rS FreeBSD src repository.
kristof updated this revision to Diff 9044.
feld added a subscriber: feld.Oct 2 2015, 12:54 PM
kristof updated this revision to Diff 9057.Oct 2 2015, 3:19 PM
  • Fixed style issues
  • Fixed AF_INET6 case in pf_change_ap()
sbruno added a subscriber: sbruno.Oct 2 2015, 4:58 PM

This appears to break the ability to use PF with jails on lo(4). I have rxcsum and txcsum disabled to allow this to work, but enabling these features does not help or fix the failure to start the jails. I use the following with ezjail to have multiple jails on a single IP in a RootBSD VM:
rc.conf:

cloned_interfaces="lo1 lo2 lo3 lo4 lo5 lo6 lo7"
ifconfig_lo1="inet 10.0.0.2/32"
ifconfig_lo2="inet 10.0.0.3/32"
ifconfig_lo3="inet 10.0.0.4/32"
ifconfig_lo4="inet 10.0.0.5/32"
ifconfig_lo5="inet 10.0.0.6/32"
ifconfig_lo6="inet 10.0.0.7/32"
ifconfig_lo7="inet 10.0.0.8/32"

pf.conf:

redir_if = "lo2"
redirnet = $redir_if:network

mail_if = "lo3"
mailnet = $mail_if:network

gallery_if = "lo4"
gallerynet = $gallery_if:network



shout_if = "lo5"
shoutnet = $shout_if:network

blog="10.0.0.2"
redir="10.0.0.3"
mail="10.0.0.4"
gallery="10.0.0.5"
shout="10.0.0.6"

Please petition for an EN after this makes it to stable/10!

Thank you for working on this!

eri added a reviewer: eri.Oct 2 2015, 6:08 PM
eri edited edge metadata.Oct 2 2015, 6:15 PM

The easiest way to do this is check if you have an inp and check that the proto is TCP from that and skip cksum update.
Otherwise you will end up with complex code as this which is easier to understand and maintain.
It certainly is not documented properly but this makes it even more.

To me just a s/pf_cksum_fixup/PF_CKSUM_FIXUP/ and inp != NULL ? cksum : pf_cksum_fixup()

TSO applies only to local generated packets since forwarded packets will never hit it.

In D3779#78074, @eri wrote:

The easiest way to do this is check if you have an inp and check that the proto is TCP from that and skip cksum update.

I'm not sure I understand your point.

The checksum update has to be done whenever we don't have a pseudo-header checksum (so no TSO, e.g. on inbound or forwarded packets), so we can't just say that we don't update TCP checksums.
Even worse, sometimes (like when we're changing a source or destination IP address) we do need to update the TCP checksum, even in the TSO case.

kristof edited edge metadata.Oct 8 2015, 10:55 PM
kristof updated this revision to Diff 9260.

Fix the updating the pseudo-header checksum.

In D3779#78057, @sbruno wrote:

This appears to break the ability to use PF with jails on lo(4). I have rxcsum and txcsum disabled to allow this to work, but enabling these features does not help or fix the failure to start the jails.

Thanks for testing. I've managed to reproduce the problem. This version fixes it for me. Can you give this one a try?

sbruno requested changes to this revision.

I didn't see any changes to my failure case. The first loX interface seems to come up and is useable by one of my jails, but the other 4 jails and interfaces fail to work correctly.

This revision now requires changes to proceed.Oct 9 2015, 2:41 PM
kristof edited edge metadata.Oct 10 2015, 7:36 PM
kristof updated this revision to Diff 9306.

Fix UDP. It turns out that people actually use that from time to time!

kristof edited edge metadata.Oct 12 2015, 10:01 PM
kristof updated this revision to Diff 9340.

No functional change.

Added a comment block explaining the checksum policy

Retested this morning in my RootBSD VM. I have enabled TSO and CheckSumming Support and all jails are up and running. I think this is working as you intend now! :-)

sbruno edited edge metadata.Oct 13 2015, 5:41 PM
sbruno accepted this revision.
This revision is now accepted and ready to land.Oct 13 2015, 5:41 PM

Hurray! I intend to commit this tomorrow evening (so in ~24 hours).

This revision was automatically updated to reflect the committed changes.