Page MenuHomeFreeBSD

LRO: Don't merge ACK and non-ACK packets together
ClosedPublic

Authored by rstone on Jan 6 2022, 10:55 PM.

Details

Summary

LRO was willing to merge ACK and non-ACK packets together. This
can cause incorrect th_ack values to be reported up the stack.
While non-ACKs are quite unlikely to appear in practice, LRO's
behaviour is against the spec. Make LRO unwilling to merge
packets with different TH_ACK flag values in order to fix the
issue.

Found by: Sysunit test

Test Plan

This unit test fails without the fix:
https://github.com/rysto32/freebsd/blob/sysunit/tests/sys/sysunit/tcp_lro_test.cc#L573

$ ./tcp_lro --gtest_filter=*TestNonAck
Running main() from /srcpool/src/rstone/worktree/sysunit/contrib/googletest/googletest/src/gtest_main.cc
Note: Google Test filter = *TestNonAck
[==========] Running 2 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from TcpLroTestSuite/0, where TypeParam = IPv4
[ RUN      ] TcpLroTestSuite/0.TestNonAck
unknown file: Failure

Unexpected mock function call - returning directly.
    Function call: if_input(0x15c4d634d000)
Google Mock tried the following 2 expectations, but none matched:

/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:600: tried expectation #0: EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(pkt1)))...
  Expected arg #0: Ethernet/IPv4/TCP/Payload
           Actual: 0x15c4d634d000, IPv4: ip_len field is 54 (expected 47)
         Expected: to be called once
           Actual: never called - unsatisfied and active
/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:603: tried expectation #1: EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(pkt2)))...
  Expected arg #0: Ethernet/IPv4/TCP/Payload
           Actual: 0x15c4d634d000, IPv4: ip_len field is 54 (expected 47)
         Expected: to be called once
           Actual: never called - unsatisfied and active
/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:600: Failure
Actual function call count doesn't match EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(pkt1)))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:603: Failure
Actual function call count doesn't match EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(pkt2)))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[  FAILED  ] TcpLroTestSuite/0.TestNonAck, where TypeParam = IPv4 (1 ms)
[----------] 1 test from TcpLroTestSuite/0 (1 ms total)

[----------] 1 test from TcpLroTestSuite/1, where TypeParam = IPv6
[ RUN      ] TcpLroTestSuite/1.TestNonAck
unknown file: Failure

Unexpected mock function call - returning directly.
    Function call: if_input(0x15c4d634d200)
Google Mock tried the following 2 expectations, but none matched:

/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:600: tried expectation #0: EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(pkt1)))...
  Expected arg #0: Ethernet/IPv6/TCP/Payload
           Actual: 0x15c4d634d200, IPv6: ip6_plen field is 34 (expected 27)
         Expected: to be called once
           Actual: never called - unsatisfied and active
/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:603: tried expectation #1: EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(pkt2)))...
  Expected arg #0: Ethernet/IPv6/TCP/Payload
           Actual: 0x15c4d634d200, IPv6: ip6_plen field is 34 (expected 27)
         Expected: to be called once
           Actual: never called - unsatisfied and active
/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:600: Failure
Actual function call count doesn't match EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(pkt1)))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:603: Failure
Actual function call count doesn't match EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(pkt2)))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[  FAILED  ] TcpLroTestSuite/1.TestNonAck, where TypeParam = IPv6 (0 ms)
[----------] 1 test from TcpLroTestSuite/1 (0 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 2 test cases ran. (1 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] TcpLroTestSuite/0.TestNonAck, where TypeParam = IPv4
[  FAILED  ] TcpLroTestSuite/1.TestNonAck, where TypeParam = IPv6

 2 FAILED TESTS

It passes with the fix:

$ ./tcp_lro --gtest_filter=*TestNonAck
Running main() from /srcpool/src/rstone/worktree/sysunit/contrib/googletest/googletest/src/gtest_main.cc
Note: Google Test filter = *TestNonAck
[==========] Running 2 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from TcpLroTestSuite/0, where TypeParam = IPv4
[ RUN      ] TcpLroTestSuite/0.TestNonAck
[       OK ] TcpLroTestSuite/0.TestNonAck (1 ms)
[----------] 1 test from TcpLroTestSuite/0 (1 ms total)

[----------] 1 test from TcpLroTestSuite/1, where TypeParam = IPv6
[ RUN      ] TcpLroTestSuite/1.TestNonAck
[       OK ] TcpLroTestSuite/1.TestNonAck (0 ms)
[----------] 1 test from TcpLroTestSuite/1 (0 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 2 test cases ran. (1 ms total)
[  PASSED  ] 2 tests.

Diff Detail

Repository
rG 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

Ping? I'd like to get this off of my queue by some time next week.

I'll bring it up on the transport call tomorrow.

This revision is now accepted and ready to land.Jan 13 2022, 3:22 PM