Page MenuHomeFreeBSD

LRO: Fix lost packets when merging 1 payload with an ACK
ClosedPublic

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

Details

Summary

To check if it needed to regenerate a packet's header before
sending it up the stack, LRO was checking if more than one payload
had been merged into the packet. This failed in the case where
a single payload was merged with one or more pure ACKs. This
results in lost ACKs.

Fix this by precisely tracking whether header regeneration is
required instead of using an incorrect heuristic.

Found with: 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#L381

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

Unexpected mock function call - returning directly.
    Function call: if_input(0x38ca7d33e000)
Google Mock tried the following 1 expectation, but it didn't match:

/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:400: EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(expected)))...
  Expected arg #0: Ethernet/IPv4/TCP/Payload
           Actual: 0x38ca7d33e000, TCP: th_ack field is 965 (expected 3854)
         Expected: to be called once
           Actual: never called - unsatisfied and active
/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:400: Failure
Actual function call count doesn't match EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(expected)))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[  FAILED  ] TcpLroTestSuite/0.TestIncrPureAck, where TypeParam = IPv4 (1 ms)
[----------] 1 test from TcpLroTestSuite/0 (1 ms total)

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

Unexpected mock function call - returning directly.
    Function call: if_input(0x38ca7d33e000)
Google Mock tried the following 1 expectation, but it didn't match:

/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:400: EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(expected)))...
  Expected arg #0: Ethernet/IPv6/TCP/Payload
           Actual: 0x38ca7d33e000, TCP: th_ack field is 965 (expected 3854)
         Expected: to be called once
           Actual: never called - unsatisfied and active
/srcpool/src/rstone/worktree/sysunit/tests/sys/sysunit/tcp_lro_test.cc:400: Failure
Actual function call count doesn't match EXPECT_CALL(*this->mockIfp, if_input(PacketMatcher(expected)))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[  FAILED  ] TcpLroTestSuite/1.TestIncrPureAck, where TypeParam = IPv6 (1 ms)
[----------] 1 test from TcpLroTestSuite/1 (1 ms total)

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

 2 FAILED TESTS

It passes with the fix:

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

[----------] 1 test from TcpLroTestSuite/1, where TypeParam = IPv6
[ RUN      ] TcpLroTestSuite/1.TestIncrPureAck
[       OK ] TcpLroTestSuite/1.TestIncrPureAck (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

rstone edited the test plan for this revision. (Show Details)

Ping? I'd like to get this patch and the other LRO patch in by 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:23 PM