Re: [PATCH mptcp-next v3 12/29] selftests: mptcp: userspace pm remove id 0 subflow

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 05/10/2023 10:32, Geliang Tang wrote:
> Hi Matt,
> 
> On Thu, Sep 28, 2023 at 10:58:33PM +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 25/09/2023 10:41, Geliang Tang wrote:
>>> This patch adds a selftest for userpsace PM to remove id 0 subflow. Use
>>> userspace_pm_add_sf() to add a subflow, and pass initial ip address to
>>> userspace_pm_rm_sf() to remove id 0 subflow.
>>>
>>> When closing the initial subflow in __mptcp_close_ssk(), dispose_it is
>>> false, then tcp_disconnect is invoked. This will send a MP_RST to close
>>> a subflow on the peer too.
>>
>> I don't think we should have a RST (with MP_TCPRST) when we close the
>> initial subflow, no? We don't have that when we remove subflows with
>> other IDs.
> 
> We do have a RST in "in-kernel remove id 0 subflow and address" tests,
> we can test them like this:

Thank you for having checked!

Do we have a RST if we remove another subflow than the initial one or if
we send a RM_ADDR for another id than 0? We should have the same
behaviour as with the other subflows/ID, no exception for the initial
subflow/address ID 0.

Is the RST sent by the host which received the RM_ADDR?

> 
>         # remove id 0 subflow
>         if reset "remove id 0 subflow"; then

(I think we should change the name of this test, it is confusing because
there is no 'id 0 subflow', an ID is linked to an address. Maybe:
"remove initial subflow")

>                 pm_nl_set_limits $ns1 0 1
>                 pm_nl_set_limits $ns2 0 1
>                 pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
>                 addr_nr_ns2=-9 speed=slow \

Just to be sure (because 'addr_nr_ns2=-9' is really not clear): it will
delete the endpoint linked to the address used by the initial subflow,
right? And this should stop the subflow (TCP FIN) and send a RM_ADDR, right?

>                         run_tests $ns1 $ns2 10.0.1.1
>                 chk_join_nr 1 1 1
>                 chk_rm_nr 1 1
> +               chk_rst_nr 1 1
>         fi
> 
>         # remove id 0 address
>         if reset "remove id 0 address"; then
>                 pm_nl_set_limits $ns1 0 1
>                 pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
>                 pm_nl_set_limits $ns2 1 1
>                 addr_nr_ns1=-9 speed=slow \

But then here, the behaviour should be the same as the previous test,
no? Why is it not?

>                         run_tests $ns1 $ns2 10.0.1.1
>                 chk_join_nr 1 1 1
>                 chk_add_nr 1 1
>                 chk_rm_nr 1 1 invert
> +               chk_rst_nr 1 1 invert
>         fi
> 
> Maybe it's OK with the RSTs.

As long as the behaviour is the same as when removing another subflow
than the initial one or sending a RM_ADDR for another ID then 0.

Cheers,
Matt