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

[Geliang Tang <geliang.tang@suse.com>]

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:

        # remove id 0 subflow
        if reset "remove id 0 subflow"; then
                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 \
                        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 \
                        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.

Thanks,
-Geliang

> 
> I guess we are not handling this case properly on the kernel side. We
> should close the initial subflow normally (with FIN) like we would do
> with any other subflow IDs (and here, we cannot fully destroy it and we
> still need to keep a valid ref for msk->first).
> 
> > So chk_rst_nr() is added in this test, and
> > chk_all_subflows after closing the initial subflow is '1 1', not '2 1'.
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net