mptcp: add sync_join_fail packetdrill test

[chrisocean716-star]

Add a packetdrill test for passive MP_JOIN failure on the sync path after MP_FAIL disables further joins. Verify extra_subflows is rolled back via a new check_mptcp_info.sh helper that asserts MPTCP socket subflow counts using ss(8).

Test Usage:

cd gtests/net/mptcp/syscalls
../../packetdrill/packetdrill sync_join_fail.pkt

If there is no output, the test passes; if output
appears as shown below, the test fails.

    expected subflows=0, got 1
    ESTAB      960          0                  [fd3d:fa7b:d17d::d6ca:1e4f]:8080             [2001:db8::1]:36921
	    skmem:(r960,rb540000,t0,tb266240,f0,w0,o0,bl0,d0) subflows:1 subflows_max:8 add_addr_accepted_max:8 remote_key token:f75ec550 write_seq:6910891285460792251 snd_una:6910891285460792251 rcv_nxt:13263177308786788775 bytes_received:2
    sync_join_fail.pkt:51: error executing `../common/check_mptcp_info.sh subflows 0` command: non-zero status 1

Assisted-by: Cursor

[matttbe]

thank you for this new test, that's very useful to understand the modification you did on the kernel side.

I just wonder if there are no other kernel bugs here: I would expect the MP_FAIL to cause a fallback, and not seeing any other MPTCP option after the MP_FAIL exchange.

[matttbe]

Interesting technique. I guess having the 3rd or 4th ACK without any MPTCP options would have done the same job, no?

(still, I think it is interesting to have (another?) test validating this case with MP_FAIL, see below)

[matttbe]

I guess having the 3rd or 4th ACK without any MPTCP options would have done the same job, no?

@chrisocean716-star : is MP_FAIL a hard requirement here?

[chrisocean716-star]

A plain 3rd/4th ACK without MPTCP options would not exercise the same kernel path.

Such an ACK triggers mptcp_try_fallback() from check_fully_established(). That sets allow_subflows = false, but also sets MPTCP_FALLBACK_DONE and returns without calling __mptcp_subflow_fully_established(). The MPTCP socket therefore stays not fully established.

When a passive MP_JOIN arrives in that state, mptcp_finish_join() fails early with MPTCP_RST_EMPTCP because !mptcp_is_fully_established(). It never reaches mptcp_pm_allow_new_subflow() nor
__mptcp_finish_join(), so the extra_subflows leak we are testing is not hit.

MP_FAIL is needed here because it disables further joins (allow_subflows = false) without immediately completing TCP fallback, so the connection can still become fully established on the subsequent 4th-ACK path. The passive join then fails at __mptcp_finish_join(), which is exactly the path fixed by the kernel patch.

The DSS+MP_FAIL on the same inbound packet is a packetdrill encoding requirement, not a kernel one.

Agreed that a separate test for the plain-ACK fallback case could still be useful — it would cover a different rejection path (EMPTCP at join time).

[matttbe]

Thank you for your reply, that's clearer!

MP_FAIL is needed here because it disables further joins (allow_subflows = false) without immediately completing TCP fallback, so the connection can still become fully established on the subsequent 4th-ACK path.

The kernel behaviour here feels wrong. My understanding of the RFC8684 is that the MP_FAIL is typically sent in two cases:

• Either when there are multiple subflows, and there is an issue on one of them.
• Or an issue with the checksum.

When a valid MP_FAIL is received and there is only one subflow, there should be a fallback to TCP, and the connection can no longer continue with MPTCP support. Is this not correct? (I don't remember all the details by heart, it is possible I missed something).

If my understanding is correct, then the kernel should be fixed (and this test here should fail and be adapted), right? If yes, are you looking into this? (see the suggestion below)

When a passive MP_JOIN arrives in that state, mptcp_finish_join() fails early with MPTCP_RST_EMPTCP because !mptcp_is_fully_established()

What if mptcp_is_fully_established() also checks && !__mptcp_check_fallback()?

EDIT: and also && msk->allow_subflows (+ it looks like all access and writes to allow_subflows should be done via READ/WRITE_ONCE())

The MPTCP socket therefore stays not fully established.

What if the MPTCP options are dropped after the switch to fully established? That's what is done in dss_drop_after_data_fallback_server.pkt. In this case, the server switched to fully established, but then did a fallback. I guess you should have the same behaviour which feels wrong: the MP_JOIN should fail directly.

So at the end, after the fix discussed here, the error path you are fixing with your kernel patch should only happen in case of race conditions: in case of fallback during the MP_JOIN exchange, no? If yes, your kernel patch is still valid, but harder to validate. But at least we can validate the MP_JOIN fails early, not like what we have now with your current test with MP_FAIL or with a fallback after the 4WHS.

[Dwyane-Yan]

Hi @matttbe

I hope the comments from me can help you.

The kernel behaviour here feels wrong. My understanding of the RFC8684 is that the MP_FAIL is typically sent in two cases:

Either when there are multiple subflows, and there is an issue on one of them.
Or an issue with the checksum.

I confirm your analysis and I think you are right. RFC8684 clearly states: ‘A special case is when there is a single subflow and it fails with a checksum error’. It seems MP_FAIL is indeed only sent when there is a checksum error. I even considered a corner case: what if the server's 4th ACK carries data (MP_CAPABLE carrying data) and the checksum fails? Could that trigger this issue? But based on Chenguang's description (the MP_FAIL should recv before the 4th ack), even in that scenario, it should not trigger the extra_subflows leak that his patch is fixing.

Therefore, I share the same view as you: this issue seems unlikely to occur in practice.

But based on this PR and your comments, I think @chrisocean716-star could expand his work in a few directions:

• Use check_mptcp_info.sh to help with other scripts, like sockopt_sndbuf_server.pkt:

  // Check the sk_sndbuf
+0.0   `SF_SNDBUF=$(ss -tm  | grep -oP 'tb\K\d+' | tr -d ' ')
        # in '__mptcp_sync_sndbuf': EXPECTED = tcp_wmem[0] + first->sk_sndbuf
        EXPECTED=$((SF_SNDBUF + 1024))
        REAL=$(ss -Mm  | grep -oP 'tb\K\d+')

        if [ "$EXPECTED" != "$REAL" ]; then
                echo "ERROR: expected ${EXPECTED}, got ${REAL}" >&2
                exit 1
        fi`

• Add some test scripts to cover 'MP_FAIL' test cases.

WDYT？

@chrisocean716-star Please note that these are just my personal opinions.
Thanks
Gang

[matttbe]

this issue seems unlikely to occur in practice.

Indeed, but only after having modified the kernel. Just to avoid a deadlock here, are you or @chrisocean716-star looking at fixing it with the suggested modification (maybe it is not enough, or "too much", to be analysed and tested)? If not, that's OK, but it is just not to forget about it :)

I think the current test can still be added, but it will need to be adapted to the new behaviour after having modified the kernel. It will no longer validate the error path that was targeted at the beginning, but it will still cover something that was not validated before.

Use check_mptcp_info.sh to help with other scripts, like sockopt_sndbuf_server.pkt:

Good idea!

Add some test scripts to cover 'MP_FAIL' test cases.

Only if they help to cover new cases. Maybe that's not needed.

I hope the comments from me can help you.

Yes they are, thanks!

[matttbe]

If a fallback was done before, can you remind me why the kernel is replying with a SYN+ACK here, and only do the reset later (that's maybe normal due to some constraints, but if you could point me to the code (I guess you already looked at that), that will be helpful)

[matttbe]

If a fallback was done before, can you remind me why the kernel is replying with a SYN+ACK here, and only do the reset later (that's maybe normal due to some constraints, but if you could point me to the code (I guess you already looked at that), that will be helpful)

Don't forget this request here, please.

[chrisocean716-star]

This SYN+ACK followed by MP_RST on the 3rd ACK is expected with the current upstream kernel, and it is what this test is exercising.

Note: in this test the parent socket received MP_FAIL, not a full TCP fallback (MPTCP_FALLBACK_DONE). MP_FAIL only sets allow_subflows = false; the MPTCP connection is still fully established. The same SYN+ACK-then-reset pattern applies though, because passive MP_JOIN is not rejected at SYN time.

Why SYN+ACK on SYN

On SYN, the listener handles MP_JOIN in subflow_check_req() /mptcp_subflow_init_req() (subflow.c):

• looks up the token via subflow_token_join_request() and prepares the SYN-ACK THMAC with subflow_req_create_thmac()
• does not check allow_subflows (nor MPTCP_FALLBACK_DONE)
• the only early SYN-time rejection is on the syncookie path via mptcp_can_accept_new_subflow(), which checks PM accept_subflow, not allow_subflows

The SYN-ACK MP_JOIN option is emitted unconditionally by mptcp_synack_options() when subflow_req->mp_join is set (options.c).

Why reset only on the 3rd ACK

For passive join, the ACK is processed in subflow_syn_recv_sock()
(subflow.c):

1. validate the client HMAC (subflow_hmac_valid())
2. optionally check mptcp_can_accept_new_subflow() (still not allow_subflows)
3. call mptcp_finish_join()

After MP_FAIL, step (2) can still succeed because the socket is fully established and PM accept_subflow is still true. The join is rejected in step (3):

• mptcp_pm_allow_new_subflow() increments extra_subflows
• __mptcp_finish_join() returns false because allow_subflows == false (protocol.c)
• mptcp_finish_join() sets reset_reason = MPTCP_RST_EPROHIBIT
• subflow_syn_recv_sock() sends the reset via dispose_child

So the rejection is deferred until join completion, not at SYN parsing. That is current kernel design (crypto needs the full 3-way handshake), and the kernel patch under test fixes the missing extra_subflows rollback on that failing completion path.

Whether joins should be rejected earlier (e.g. at SYN when allow_subflows == false) could be a separate discussion, but it is outside the scope of this test/bugfix.

[matttbe]

Thank you for the explanation!

Whether joins should be rejected earlier (e.g. at SYN when allow_subflows == false) could be a separate discussion, but it is outside the scope of this test/bugfix.

I think it shouldn't be a separate discussion, because the behaviour you are validating happens to be wrong: OK it validates the error path you want, but I think the kernel shouldn't reach this error path in this case. In other words, there is a bug on the kernel side, see my comment here (with a suggestion of fix)

I guess we should check for fallback and if extra subflows are allowed (temp state with MP_FAIL), similar to what is done in __mptcp_push_retrans().

[matttbe]

Thank you for the update!

Can you please split in different commits, the modifications in the C code is hard to review.

Also, don't forget the questions from the previous review. It is helpful if you mark all previous comments as done and/or send a reply to help reviewers who cannot remember all previous discussions ;-)

[matttbe]

I guess having the 3rd or 4th ACK without any MPTCP options would have done the same job, no?

@chrisocean716-star : is MP_FAIL a hard requirement here?

[matttbe]

If a fallback was done before, can you remind me why the kernel is replying with a SYN+ACK here, and only do the reset later (that's maybe normal due to some constraints, but if you could point me to the code (I guess you already looked at that), that will be helpful)

Don't forget this request here, please.

[chrisocean716-star]

Thank you for the detailed review and for pointing out the RFC and kernel behaviour issues. I agree with your analysis, and here is how I plan to proceed:

1. Kernel fix

I will fix the kernel so that receiving MP_FAIL before the 4th ACK properly triggers fallback to regular TCP. After the MP_FAIL exchange, the connection should no longer continue in MPTCP mode and no further MPTCP options should be processed on that path.

Concretely, I plan to follow your suggestions, including:

making MP_FAIL handling trigger a full fallback (not just setting allow_subflows = false);
tightening mptcp_is_fully_established() with !__mptcp_check_fallback() and an allow_subflows check;
using READ_ONCE/WRITE_ONCE for allow_subflows;
rejecting passive MP_JOIN earlier when fallback has occurred or new subflows are no longer allowed, rather than completing the SYN/SYN-ACK exchange and only failing on the 3rd ACK.
I also understand that after this fix, the extra_subflows rollback path I originally targeted may mainly remain relevant for race conditions during the MP_JOIN exchange, rather than the artificial MP_FAIL scenario in the current test.

2. Packetdrill test rewrite

I agree that the current test scenario is not realistic according to RFC 8684. As you noted, MP_FAIL should normally only appear in two cases:

when there are multiple subflows and one of them has a problem, or
when a checksum failure is detected.
Directly injecting MP_FAIL in packetdrill, as in the current test, does not represent a valid on-the-wire scenario. After the kernel fix, I will rewrite the test accordingly, using a realistic trigger such as a checksum failure (similar to the existing fail_tests() coverage in mptcp_join.sh), and adapt the expectations to the new fallback behaviour.

Once the kernel changes are ready, I will update the packetdrill test.

[matttbe]

Thank you for the detailed review and for pointing out the RFC and kernel behaviour issues. I agree with your analysis, and here is how I plan to proceed:

1. Kernel fix

I will fix the kernel so that receiving MP_FAIL before the 4th ACK properly triggers fallback to regular TCP. After the MP_FAIL exchange, the connection should no longer continue in MPTCP mode and no further MPTCP options should be processed on that path.

Concretely, I plan to follow your suggestions, including:

making MP_FAIL handling trigger a full fallback (not just setting allow_subflows = false); tightening mptcp_is_fully_established() with !__mptcp_check_fallback() and an allow_subflows check; using READ_ONCE/WRITE_ONCE for allow_subflows; rejecting passive MP_JOIN earlier when fallback has occurred or new subflows are no longer allowed, rather than completing the SYN/SYN-ACK exchange and only failing on the 3rd ACK. I also understand that after this fix, the extra_subflows rollback path I originally targeted may mainly remain relevant for race conditions during the MP_JOIN exchange, rather than the artificial MP_FAIL scenario in the current test.

Sounds good to me, thanks.

Your patch "mptcp: fix extra_subflows leak on failed passive join" can then be applied without a test. I will probably slightly modify your commit message to reflect what we saw here.

2. Packetdrill test rewrite

I agree that the current test scenario is not realistic according to RFC 8684. As you noted, MP_FAIL should normally only appear in two cases:

when there are multiple subflows and one of them has a problem, or when a checksum failure is detected. Directly injecting MP_FAIL in packetdrill, as in the current test, does not represent a valid on-the-wire scenario.

Indeed, but another network stack could still inject an MP_FAIL in this case, so I think it is important to keep your test, but adapt it after the kernel modification.

Similarly, I think it would be good to extend dss_drop_after_data_fallback_server.pkt to make sure an MP_JOIN added at the end after the fallback will be rejected.

Do you mind looking at that too, please?

After the kernel fix, I will rewrite the test accordingly, using a realistic trigger such as a checksum failure (similar to the existing fail_tests() coverage in mptcp_join.sh), and adapt the expectations to the new fallback behaviour.

Once the kernel changes are ready, I will update the packetdrill test.

Thanks!

[chrisocean716-star]

Hi matttbe,
I extended dss_drop_after_data_fallback_server.pkt, but after fixing the kernel bug described in https://lore.kernel.org/all/20260713064134.914507-1-chenguang.zhao@linux.dev/, I can no longer reproduce the extra_subflows leak.