Re: [PATCH net v4 2/2] selftests: mptcp: add regression test for stale ADD_ADDR on id 0 removal

[Matthieu Baerts <matttbe@kernel.org>]

Hi Kalpan,

On 22/06/2026 11:28, Kalpan Jani wrote:
> Add a kselftest that reproduces the bug fixed in the previous patch:
> removing the id 0 endpoint while a pending ADD_ADDR echo is outstanding
> left a stale anno_list entry alive, and a subsequent PM reselection
> re-announced that address and tripped the WARN_ON_ONCE() in
> mptcp_pm_alloc_anno_list().
> 
> The sequence is deterministic:
>   1. Establish a fully-established MPTCP connection kept alive by a
>      bidirectional /dev/zero stream.
>   2. Signal 10.0.2.1 so the peer joins: the second subflow keeps the
>      connection alive across the id 0 removal.
>   3. Signal the MPC address 10.0.1.1: an anno_list entry is created.
>   4. Delete id 0 (10.0.1.1): on an unfixed kernel the stale entry
>      survives.
>   5. Signal 10.0.3.1 to force a PM reselection: on an unfixed kernel
>      this hits the stale entry and fires the WARN.

Thank you for having added a reproducer, but in general:

- We try to avoid new "main" tests for maintainability reasons.

- If possible, please prefer packetdrill [1] for the reproducers: that's
easier to read (shorter, focussed on the essential to reproducer an
issue), faster to execute, and easier to maintain. Please try to use it,
and do not hesitate to ask for help if needed. Mastering packetdrill is
really helpful in this area of the kernel.

- If not, please check if adding a subtest to an existing selftest is
not possible, e.g. mptcp_join.sh

- We try to avoid using "sleep": it is either waiting for a too long
time, or not enough depending on the kernel config and test environment.
Prefer waiting for an action instead, e.g. checking if a connection has
been established, the number of subflows, etc.

[1] https://github.com/multipath-tcp/packetdrill/

> The test counts WARNING: net/mptcp/pm lines in dmesg before and after
> the sequence and fails if new ones appear.

That's not needed, the executor (i.e. CIs) will monitor that.

A few more requests about this series, please:

- base your patches on top of the export branch (or "for-review" or
"t/upstream" branches), but not the "export-net" one: the CI will always
try to apply them on top of the "export" branch, and that's easier for
me when I have to resolve conflicts. (Note that the "export" branch
contains both "net-next" and "net", plus all patches applied in our tree.)

- ideally, avoid renaming the patches, or at least the cover-letter,
between versions, so it is easier for reviewers and bots to follow. Here
for example, you switched to one patch to multiple ones: it would be
easier if the cover-letter has same title as your previous single patch.
If renaming is required, please add a note in the patch/cover-letter.
Note that b4 automatically adds a link to the previous version, that's
also helpful.

- use "mptcp-ne(x)t" prefixes instead of "ne(x)t" when you send patches
only to the MPTCP ML (which is what we prefer).

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.