mptcp: add regression test for stale ADD_ADDR on id 0 removal (issue #620)

[k-jani-git]

Regression test for the kernel fix at:
https://lore.kernel.org/mptcp/ (v5 patch 1)

Issue: multipath-tcp/mptcp_net-next#620

The id 0 endpoint removal path used to skip cancelling a pending
ADD_ADDR announcement, leaving the anno_list entry alive. A subsequent
PM reselection would re-announce that address and trip WARN_ON_ONCE()
in mptcp_pm_announced_alloc().

The test establishes an MPC connection with a second MP_JOIN subflow,
signals the MPC address as id 0 (creating a pending anno_list entry),
withholds the echo, then deletes and re-adds id 0. It probes with a
data segment and expects a plain DSS ACK back with no ADD_ADDR
piggybacked. On an unfixed kernel a spurious ADD_ADDR(id=0) appears
on that ACK and the test fails.

Co-developed-by: Tao Cui cuitao@kylinos.cn
Signed-off-by: Kalpan Jani kalpan.jani@mpiricsoftware.com

[matttbe]

Thank you for the new test! Here is a first global comment

[k-jani-git]

Hi Matt,

Thanks for the review.

Fixed in the latest push:

• Reformatted to long single lines per .pkt style
• Removed TCP timestamps from all packet lines
• Changed tolerance_usecs to 100000 to match other regression tests;
  the +0~+0.5 window on the ADD_ADDR line handles the async PM timing
  independently

Cheers,
Kalpan

[matttbe]

mmh, how did you test this?

parse error at '`sysctl -wq net.mptcp.add_addr_timeout=1`': syntax error

after having fixed that, I get an error at line 35:

semantic error: TCP options are not padded correctly to ensure TCP header is a multiple of 4 bytes: 2 excess bytes

[matttbe]

Mmmh, I continued debugging this, and it really looks like this .pkt was generated by AI... It looks correct, but full of errors. Plus untested.

I just sent what I modified so far, but I didn't finish, and late for other meetings. I hope this is just a one off...

[k-jani-git]

Hi @matttbe

I apologize — the test was not validated locally before submission.
That was my mistake and I should not have submitted untested code.

I do have a working tested reproducer from an earlier version: a
selftest that reliably triggers the WARN on unpatched kernels and
passes cleanly on the patched one. Would it be acceptable to close
this PR and resubmit once I have a properly tested version — either
the selftest ported into mptcp_join.sh or a corrected packetdrill
test validated through the docker CI first?

Sorry for the wasted time.

[matttbe]

@k-jani-git : with what happened here, I do think it is best if you fix this packetdrill test.

[matttbe]

here, better add a small delay like .5 sec

[matttbe]

here, we should see the ADD_ADDR, the echo can be injected.

[matttbe]

the sequence can be removed: no longer correct, and it was redundant with the comments below

[matttbe]

@k-jani-git : did you manage to have a working reproducer with packetdrill? Note that the current implementation has some limitations, it is possible that it is not possible to reproduce the bug with the current version, and that's fine, but in this case, it would be good to know the limitations. Knowing them, I can also help to find workarounds (if possible).

[k-jani-git]

Hi @matttbe Not yet, I haven't been able to get back to this. I'll send an update once I have something tested.

Thanks for the offer — once I've had a proper look I'll come back with the specific point where packetdrill blocks it, so you can see whether there's a workaround. Happy to take any input you have at that stage.

[matttbe]

@k-jani-git thank you for your reply. No problem, we can hold this and the attached kernel patch a bit longer.

[k-jani-git]

Hi @matttbe

Pushed a rewritten test. This time I validated it in the virtme-docker
environment against both an unpatched and a patched kernel before pushing.

On export/20260622T173908, without the fix:

FAIL [/opt/packetdrill/gtests/net/mptcp/pm/kernel_id0_stale_anno.pkt (ipv4)]
stderr:
BUG: add_addr_signaled not decremented after delete id 0
kernel_id0_stale_anno.pkt:51: error executing `ss -inmHM | grep -q ...` command: non-zero status 1
Ran 3 tests: 0 passing, 3 failing, 0 timed out

With the "mptcp: pm: drop pending ADD_ADDR when removing id 0 endpoint" patch
applied:

OK [/opt/packetdrill/gtests/net/mptcp/pm/kernel_id0_stale_anno.pkt (ipv4)]
OK [/opt/packetdrill/gtests/net/mptcp/pm/kernel_id0_stale_anno.pkt (ipv4-mapped-v6)]
OK [/opt/packetdrill/gtests/net/mptcp/pm/kernel_id0_stale_anno.pkt (ipv6)]
Ran 3 tests: 3 passing, 0 failing, 0 timed out

While getting there I also confirmed the WARNING itself is reachable from this
sequence: adding a second 'signal' endpoint after the removal raises
endp_signal_max to 2, the PM re-enters the announce branch, and it hits
WARN_ON_ONCE() in mptcp_pm_announced_alloc() with the same call chain as the
syzbot report (mptcp_pm_nl_add_addr_doit ->
mptcp_pm_create_subflow_or_signal_addr). I left that step out of the final
test: packetdrill does not fail on a kernel WARNING, so the counter check is
what actually discriminates.

Your explanation about the endpoint linked to the initial subflow is what
unblocked this. The test adds the 'signal' endpoint after the connection is
established instead of deleting and re-adding it: mptcp_mpc_endpoint_setup()
has already run at that point, so the bit is never cleared and the address is
announced with ID 0. Same end state as the delete/re-add sequence, but it does
not generate an RM_ADDR, which matters because of point 1 below. Happy to
switch to the delete/re-add form if you prefer it.

Three limitations I hit, since you asked about them:

1. remove_address cannot be asserted in a .pkt today. same_mptcp_opt() in
   run_packet.c loops opt_a->length bytes starting at
   data.remove_addr.address_id instead of looping over the number of IDs. For a
   single-ID RM_ADDR that reads 3 bytes past the option in both buffers, so the
   comparison fails even when the two packets print identically. I can send a
   separate PR for this if you would like.

2. "ip mptcp endpoint delete id 0 " does not remove the pernet endpoint,
   so a subsequent "endpoint add" of the same address returns -EEXIST. That is
   why the announce is set up by adding the endpoint late rather than by
   delete/re-add. (iproute2 also rejects an address argument together with a
   non-zero id, so the delete has to be done by id alone.)

3. After "delete id 0" no second ADD_ADDR is emitted even on a fixed kernel, so
   I could not use the reselection as the assertion. The check is on the path
   manager counter via ss(8) instead of on a packet. The ADD_ADDR(id=0) that
   precedes it is still asserted at packet level. If you would rather this live
   in mptcp_join.sh as a chk_mptcp_info check, say so and I will move it.

Sorry again for the earlier untested submission, and thanks for the time you
spent fixing it up.