Re: [PATCH mptcp-next v2 1/2] mptcp: pm: userspace: drop delete_local_addr helper

[Mat Martineau <martineau@kernel.org>]

On Wed, 16 Apr 2025, Geliang Tang wrote:

> Hi Matt,
>
> On Tue, 2025-04-15 at 13:00 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 15/04/2025 11:18, Geliang Tang wrote:
>>> On Tue, 2025-04-15 at 11:09 +0800, Geliang Tang wrote:
>>>> On Fri, 2025-04-11 at 17:26 -0700, Mat Martineau wrote:
>>>>> On Thu, 10 Apr 2025, Geliang Tang wrote:
>>>>>
>>>>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>>>>
>>>>>> Address entries should not be removed from local_addr_list
>>>>>> when a
>>>>>> subflow
>>>>>> is deleted by the userspace PM, should only be removed when
>>>>>> sending
>>>>>> a
>>>>>> REMOVE_ADDR.
>>>>>>
>>>>>> So mptcp_userspace_pm_delete_local_addr() helper shouldn't be
>>>>>> called in
>>>>>> mptcp_pm_nl_subflow_create_doit() and
>>>>>> mptcp_pm_nl_subflow_destroy_doit().
>>>>>>
>>>>>> Since this helper is open-coding in
>>>>>> mptcp_pm_nl_remove_doit(), it
>>>>>> can be
>>>>>> dropped now.
>>>>>>
>>>>>> Address entries are removed from local_addr_list when sending
>>>>>> a
>>>>>> REMOVE_ADDR
>>>>>> by the userspace PM, the local_addr_used counter of PM should
>>>>>> also
>>>>>> be
>>>>>> decremented accordingly.
>>>>>>
>>>>>
>>>>> Hi Geliang -
>>>>>
>>>>> Have you tried this patch with mptcpd? Does it affect any tests
>>>>> there?
>>>>
>>>> I haven't tested it. To be honest, I don't know how to run the
>>>> test
>>>> items of mptcpd. Can you give me some guidance?
>>
>> 'make check' should execute all the tests. Do you mind trying this
>> with
>> and without your modifications?
>
> This is the output with and without my modifications:
>
> make  check-TESTS
> make[3]: Entering directory '/home/tgl/mptcpd/tests'
> make[4]: Entering directory '/home/tgl/mptcpd/tests'
> PASS: test-plugin
> PASS: test-network-monitor
> PASS: test-path-manager
> PASS: test-commands
> PASS: test-configuration
> PASS: test-id-manager
> PASS: test-listener-manager
> PASS: test-sockaddr
> PASS: test-addr-info
> PASS: test-murmur-hash
> PASS: test-cxx-build
> PASS: test-bad-log-empty
> PASS: test-bad-log-long
> PASS: test-bad-log-short
> PASS: test-bad-option
> PASS: test-bad-path-manager
> PASS: test-bad-plugin-dir
> SKIP: test-start-stop
> PASS: test-mptcpwrap
> =======================================================================
> =====
> Testsuite summary for mptcpd 0.13
> =======================================================================
> =====
> # TOTAL: 19
> # PASS:  18
> # SKIP:  1
> # XFAIL: 0
> # FAIL:  0
> # XPASS: 0
> # ERROR: 0
> =======================================================================
> =====
> make[4]: Leaving directory '/home/tgl/mptcpd/tests'
> make[3]: Leaving directory '/home/tgl/mptcpd/tests'
> make[2]: Leaving directory '/home/tgl/mptcpd/tests'
> make[1]: Leaving directory '/home/tgl/mptcpd/tests'
> Making check in scripts
> make[1]: Entering directory '/home/tgl/mptcpd/scripts'
> make[1]: Nothing to be done for 'check'.
> make[1]: Leaving directory '/home/tgl/mptcpd/scripts'
> make[1]: Entering directory '/home/tgl/mptcpd'
> make[1]: Nothing to be done for 'check-am'.
> make[1]: Leaving directory '/home/tgl/mptcpd'
>

Hi Geliang -

Thanks for running these tests. There is some coverage of the affected 
functionality by the 'test-commands' program, where the "remove_addr - 
user" subtest does send the netlink command. It only checks for whether 
the command worked or wasn't supported though, so not any verification of 
behavior. The only use of the userspace PM "remove subflow" command in the 
existing path manager plugins is in the SSPI plugin where new subflows are 
closed if there's already an existing subflow on a network interface. It 
doesn't look like mptcpd is very sensitive to specific kernel behavior 
here.

>>
>> Please make sure relevant ones are not skipped because of missing
>> dependences or tools.
>>
>>> I just tested it this way, and here are the test results:
>>
>> (...)
>>
>>> These results appear to be unchanged from before. And in fact these
>>> tests do not test userspace pm.
>>
>> I don't remember what is being tested, but I guess some parts of the
>> userspace PM are tested somewhere, no?
>>
>> Coveralls [1] seems to indicate it is tested, but I don't see the
>> details.
>>
>> [1] https://coveralls.io/github/multipath-tcp/mptcpd
>>
>>> So it seems that this patch will not affect the tests of mptcpd.
>>
>> Regarding your patch 2/2 of this series: did you have to modify the
>> selftests to make the tests passing again due to a changed behaviour,
>> or
>> did you modify them to extend the behaviour.
>
> The first one, I tried to make the tests passing again. I updated these
> userspace selftests in v3 to make sure does not change the behaviour of
> these tests.
>
> Thanks,
> -Geliang
>
>>
>> It looks like the behaviour has changed, but it is unclear to me if it 
>> is to fix something or add a new feature.

I do think the behavior is changed to fix a problem - with the existing 
kernel code, mptcpd with the SSPI plugin would un-advertise an address if 
the peer attempted to create a second subflow on that address. This would 
prevent any additional connection attempts by a well-behaved peer, but a 
well-behaved peer would also disconnect its existing subflow in response 
to the remove_addr, right?

The main risk here is that an existing MPTCP user with custom plugin code 
might be assuming the old buggy behavior, and they would have to adjust 
their plugin to handle the new "mptcpd_pm_remove_subflow doesn't 
un-advertise addresses" behavior.


- Mat