[PATCH, mptcp-net] mptcp: fix address removal logic in mptcp_pm_nl_rm_addr

[PATCH, mptcp-net] mptcp: fix address removal logic in mptcp_pm_nl_rm_addr

[Gang Yan]

From: Gang Yan <yangang@kylinos.cn>

Fix inverted WARN_ON_ONCE condition that prevented normal address
removal counter updates. The current code only executes decrement
logic when the counter is already 0 (abnormal state), while
normal removals (counter > 0) are ignored.

Signed-off-by: Gang Yan <yangang@kylinos.cn>
---
 net/mptcp/pm_kernel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index a3f654ba9dbe..cb3ebb2efd00 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -682,7 +682,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 
 void mptcp_pm_nl_rm_addr(struct mptcp_sock *msk, u8 rm_id)
 {
-	if (rm_id && WARN_ON_ONCE(msk->pm.add_addr_accepted == 0)) {
+	if (rm_id && !WARN_ON_ONCE(msk->pm.add_addr_accepted == 0)) {
 		u8 limit_add_addr_accepted =
 			mptcp_pm_get_limit_add_addr_accepted(msk);
 
-- 
2.43.0

Re: [PATCH, mptcp-net] mptcp: fix address removal logic in mptcp_pm_nl_rm_addr

[MPTCP CI]

Hi Gang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Unstable: 1 failed test(s): packetdrill_fastclose 🔴
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/19069305465

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/6546bf8ea1b2
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1019421


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH, mptcp-net] mptcp: fix address removal logic in mptcp_pm_nl_rm_addr

[Matthieu Baerts]

Hi Gang,

On 04/11/2025 13:34, Gang Yan wrote:
> From: Gang Yan <yangang@kylinos.cn>
> 
> Fix inverted WARN_ON_ONCE condition that prevented normal address
> removal counter updates. The current code only executes decrement
> logic when the counter is already 0 (abnormal state), while
> normal removals (counter > 0) are ignored.

Good catch!

For fixes, we need a Fixes tag:

Fixes: 636113918508 ("mptcp: pm: remove '_nl' from
mptcp_pm_nl_rm_addr_received")

Apart from that, it looks good to me:

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

How did you find out about this? I'm surprised out test suite didn't
spot it. By chance, do you have a test that can be used to reproduce
this issue?

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

Re: [PATCH, mptcp-net] mptcp: fix address removal logic in mptcp_pm_nl_rm_addr

[GangYan]

> On Tue, Nov 04, 2025 at 03:34:38PM +0100, Matthieu Baerts wrote:
> Hi Gang,
> 
> On 04/11/2025 13:34, Gang Yan wrote:
> > From: Gang Yan <yangang@kylinos.cn>
> > 
> > Fix inverted WARN_ON_ONCE condition that prevented normal address
> > removal counter updates. The current code only executes decrement
> > logic when the counter is already 0 (abnormal state), while
> > normal removals (counter > 0) are ignored.
> 
> Good catch!
> 
> For fixes, we need a Fixes tag:
> 
> Fixes: 636113918508 ("mptcp: pm: remove '_nl' from
> mptcp_pm_nl_rm_addr_received")
> 
> Apart from that, it looks good to me:
> 
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> How did you find out about this? I'm surprised out test suite didn't
> spot it. By chance, do you have a test that can be used to reproduce
> this issue?

I've been looking into the work_pending-related issue described in
#ISSUE 440 and have designed several potentially affected scenarios
based on mptcp_join tests. Except for the case where endpoints are
deleted one by one and then re-added via ADD_ADDR (which fails), all
other related tests have passed. (I'll share some conclusions in the
corresponding discussion later.)

By tracing the relevant code, I found that the value of
'add_addr_accepted' here does not decrease, which subsequently blocks
further ADD_ADDR operations. We can verify the value of
'add_addr_accepted' in 'delete re-add signal' test, which can be found
in mptcp_join.sh, using either 'ss' or 'mptcp_diag'. The reason this
test didn't fail previously is that when the 'add_addr_accepted_limit'
was set to 3, the test's add_addr operations never reached the limit,
so no overflow occurred.

I lean toward using 'ss' to obtain the token and then retrieving the
'add_addr_accepted' value via 'mptcp_diag', because when the value is
zero, ss does not display it. Moreover, using 'mptcp_diag' minimizes
potential inconsistencies caused by different versions of iproute2,
making maintenance easier. WDYT?

Thanks,
Gang
> 
> Cheers,
> Matt
> -- 
> Sponsored by the NGI0 Core fund.
> 

Re: [PATCH, mptcp-net] mptcp: fix address removal logic in mptcp_pm_nl_rm_addr

[GangYan]

On Wed, Nov 05, 2025 at 10:13:56AM +0800, GangYan wrote:
> > On Tue, Nov 04, 2025 at 03:34:38PM +0100, Matthieu Baerts wrote:
> > Hi Gang,
> > 
> > On 04/11/2025 13:34, Gang Yan wrote:
> > > From: Gang Yan <yangang@kylinos.cn>
> > > 
> > > Fix inverted WARN_ON_ONCE condition that prevented normal address
> > > removal counter updates. The current code only executes decrement
> > > logic when the counter is already 0 (abnormal state), while
> > > normal removals (counter > 0) are ignored.
> > 
> > Good catch!
> > 
> > For fixes, we need a Fixes tag:
> > 
> > Fixes: 636113918508 ("mptcp: pm: remove '_nl' from
> > mptcp_pm_nl_rm_addr_received")
> > 
> > Apart from that, it looks good to me:
> > 
> > Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > 
> > How did you find out about this? I'm surprised out test suite didn't
> > spot it. By chance, do you have a test that can be used to reproduce
> > this issue?
> 
> I've been looking into the work_pending-related issue described in
> #ISSUE 440 and have designed several potentially affected scenarios
Sorry for the wrong number of issue, it should be #ISSUE 549.
> based on mptcp_join tests. Except for the case where endpoints are
> deleted one by one and then re-added via ADD_ADDR (which fails), all
> other related tests have passed. (I'll share some conclusions in the
> corresponding discussion later.)
> 
> By tracing the relevant code, I found that the value of
> 'add_addr_accepted' here does not decrease, which subsequently blocks
> further ADD_ADDR operations. We can verify the value of
> 'add_addr_accepted' in 'delete re-add signal' test, which can be found
> in mptcp_join.sh, using either 'ss' or 'mptcp_diag'. The reason this
> test didn't fail previously is that when the 'add_addr_accepted_limit'
> was set to 3, the test's add_addr operations never reached the limit,
> so no overflow occurred.
> 
> I lean toward using 'ss' to obtain the token and then retrieving the
> 'add_addr_accepted' value via 'mptcp_diag', because when the value is
> zero, ss does not display it. Moreover, using 'mptcp_diag' minimizes
> potential inconsistencies caused by different versions of iproute2,
> making maintenance easier. WDYT?
> 
> Thanks,
> Gang
> > 
> > Cheers,
> > Matt
> > -- 
> > Sponsored by the NGI0 Core fund.
> > 

Re: [PATCH, mptcp-net] mptcp: fix address removal logic in mptcp_pm_nl_rm_addr

[Matthieu Baerts]

Hi Gang,

On 05/11/2025 03:13, GangYan wrote:
>> On Tue, Nov 04, 2025 at 03:34:38PM +0100, Matthieu Baerts wrote:
>> Hi Gang,
>>
>> On 04/11/2025 13:34, Gang Yan wrote:
>>> From: Gang Yan <yangang@kylinos.cn>
>>>
>>> Fix inverted WARN_ON_ONCE condition that prevented normal address
>>> removal counter updates. The current code only executes decrement
>>> logic when the counter is already 0 (abnormal state), while
>>> normal removals (counter > 0) are ignored.
>>
>> Good catch!
>>
>> For fixes, we need a Fixes tag:
>>
>> Fixes: 636113918508 ("mptcp: pm: remove '_nl' from
>> mptcp_pm_nl_rm_addr_received")
>>
>> Apart from that, it looks good to me:
>>
>> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>
>> How did you find out about this? I'm surprised out test suite didn't
>> spot it. By chance, do you have a test that can be used to reproduce
>> this issue?
> 
> I've been looking into the work_pending-related issue described in
> #ISSUE 440 and have designed several potentially affected scenarios
> based on mptcp_join tests. Except for the case where endpoints are
> deleted one by one and then re-added via ADD_ADDR (which fails), all
> other related tests have passed. (I'll share some conclusions in the
> corresponding discussion later.)
> 
> By tracing the relevant code, I found that the value of
> 'add_addr_accepted' here does not decrease, which subsequently blocks
> further ADD_ADDR operations. We can verify the value of
> 'add_addr_accepted' in 'delete re-add signal' test, which can be found
> in mptcp_join.sh, using either 'ss' or 'mptcp_diag'. The reason this
> test didn't fail previously is that when the 'add_addr_accepted_limit'
> was set to 3, the test's add_addr operations never reached the limit,
> so no overflow occurred.

OK, so the WARN_ON_ONCE() is not shown because add_addr_accepted is
never decremented, but it should have been decremented (not below the
limit).

> I lean toward using 'ss' to obtain the token and then retrieving the
> 'add_addr_accepted' value via 'mptcp_diag', because when the value is
> zero, ss does not display it. Moreover, using 'mptcp_diag' minimizes
> potential inconsistencies caused by different versions of iproute2,
> making maintenance easier. WDYT?

I think you can use 'ss', because it is already used in this test, see
chk_mptcp_info().

I suggest applying the fix now, the test can be added later.

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