[PATCH mptcp-net 1/2] mptcp: fix subflow accounting on close

[PATCH mptcp-net 1/2] mptcp: fix subflow accounting on close

[Paolo Abeni]

If the PM closes a fully established MPJ subflow or the subflow
creation errors out in it's early stage the subflows counter is
not bumped accordingly.

This change adds the missing accouting, additionally taking care
of updating accordingly the 'accept_subflow' flag.

Fixes: a88c9e496937 ("mptcp: do not block subflows creation on errors")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm.c       |  5 ++---
 net/mptcp/protocol.h | 14 ++++++++++++++
 net/mptcp/subflow.c  | 12 +++++++++---
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index cdc2d79071f8..59fdab2d0c27 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -181,15 +181,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
 	struct mptcp_pm_data *pm = &msk->pm;
 	bool update_subflows;
 
-	update_subflows = (ssk->sk_state == TCP_CLOSE) &&
-			  (subflow->request_join || subflow->mp_join) &&
+	update_subflows = (subflow->request_join || subflow->mp_join) &&
 			  mptcp_pm_is_kernel(msk);
 	if (!READ_ONCE(pm->work_pending) && !update_subflows)
 		return;
 
 	spin_lock_bh(&pm->lock);
 	if (update_subflows)
-		pm->subflows--;
+		__mptcp_pm_close_subflow(msk);
 
 	/* Even if this subflow is not really established, tell the PM to try
 	 * to pick the next ones, if possible.
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 4672901d0dfe..06b8ebc15204 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -893,6 +893,20 @@ unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_local_addr_max(const struct mptcp_sock *msk);
 
+/* called under PM lock */
+static inline void __mptcp_pm_close_subflow(struct mptcp_sock *msk)
+{
+	if (--msk->pm.subflows < mptcp_pm_get_subflows_max(msk))
+		WRITE_ONCE(msk->pm.accept_subflow, true);
+}
+
+static inline void mptcp_pm_close_subflow(struct mptcp_sock *msk)
+{
+	spin_lock_bh(&msk->pm.lock);
+	__mptcp_pm_close_subflow(msk);
+	spin_unlock_bh(&msk->pm.lock);
+}
+
 void mptcp_sockopt_sync(struct mptcp_sock *msk, struct sock *ssk);
 void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk);
 
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 6d59336a8e1e..75c85ce9a19e 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1444,20 +1444,20 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	struct sockaddr_storage addr;
 	int remote_id = remote->id;
 	int local_id = loc->id;
+	int err = -ENOTCONN;
 	struct socket *sf;
 	struct sock *ssk;
 	u32 remote_token;
 	int addrlen;
 	int ifindex;
 	u8 flags;
-	int err;
 
 	if (!mptcp_is_fully_established(sk))
-		return -ENOTCONN;
+		goto err_out;
 
 	err = mptcp_subflow_create_socket(sk, &sf);
 	if (err)
-		return err;
+		goto err_out;
 
 	ssk = sf->sk;
 	subflow = mptcp_subflow_ctx(ssk);
@@ -1515,6 +1515,12 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 failed:
 	subflow->disposable = 1;
 	sock_release(sf);
+
+err_out:
+	/* we account subflows before the creation, and this failures will not
+	 * be catched by sk_state_change()
+	 */
+	mptcp_pm_close_subflow(msk);
 	return err;
 }
 
-- 
2.35.1

[PATCH mptcp-net 2/2] selftests: mptcp: add subflow limits test-cases

[Paolo Abeni]

Add and deletes a bunch of endpoints and verify the
respect of configured limits.

This covers the codepath intersted by the previous patch.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 48 ++++++++++++++++++-
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index b703b61562bf..a98fee834950 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1571,6 +1571,33 @@ chk_prio_nr()
 	[ "${dump_stats}" = 1 ] && dump_stats
 }
 
+chk_subflow_nr()
+{
+	local need_title="$1"
+	local msg="$2"
+	local subflow_nr=$3
+	local cnt1
+	local cnt2
+
+	if [ -n "${need_title}" ]; then
+		printf "%03u %-36s %s" "${TEST_COUNT}" "${TEST_NAME}" "${msg}"
+	else
+		printf "%-${nr_blank}s %s" " " "${msg}"
+	fi
+
+	cnt1=$(ss -N $ns1 -tOni | grep -c token)
+	cnt2=$(ss -N $ns2 -tOni | grep -c token)
+	if [ "$cnt1" != "$subflow_nr" -o "$cnt2" != "$subflow_nr" ]; then
+		echo "[fail] got $cnt1:$cnt2 subflows expected $subflow_nr"
+		fail_test
+		dump_stats=1
+	else
+		echo "[ ok ]"
+	fi
+
+	[ "${dump_stats}" = 1 ] && ( ss -N $ns1 -tOni ; ss -N $ns1 -tOni | grep token; ip -n $ns1 mptcp endpoint )
+}
+
 chk_link_usage()
 {
 	local ns=$1
@@ -2784,7 +2811,7 @@ userspace_tests()
 	fi
 }
 
-implicit_tests()
+endpoint_tests()
 {
 	# userspace pm type prevents add_addr
 	if reset "implicit EP"; then
@@ -2806,6 +2833,23 @@ implicit_tests()
 			$ns2 10.0.2.2 id 1 flags signal
 		wait
 	fi
+
+	if reset "delete and re-add"; then
+		pm_nl_set_limits $ns1 1 1
+		pm_nl_set_limits $ns2 1 1
+		pm_nl_add_endpoint $ns2 10.0.2.2 id 2 dev ns2eth2 flags subflow
+		run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow &
+
+		wait_mpj $ns2
+		pm_nl_del_endpoint $ns2 2 10.0.2.2
+		sleep 0.5
+		chk_subflow_nr needtitle "after delete" 1
+
+		pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow
+		wait_mpj $ns2
+		chk_subflow_nr "" "after re-add" 2
+		wait
+	fi
 }
 
 # [$1: error message]
@@ -2854,7 +2898,7 @@ all_tests_sorted=(
 	z@fastclose_tests
 	F@fail_tests
 	u@userspace_tests
-	I@implicit_tests
+	I@endpoint_tests
 )
 
 all_tests_args=""
-- 
2.35.1

Re: [PATCH mptcp-net 1/2] mptcp: fix subflow accounting on close

[Mat Martineau]

On Thu, 28 Apr 2022, Paolo Abeni wrote:

> If the PM closes a fully established MPJ subflow or the subflow
> creation errors out in it's early stage the subflows counter is
> not bumped accordingly.
>
> This change adds the missing accouting, additionally taking care
> of updating accordingly the 'accept_subflow' flag.
>
> Fixes: a88c9e496937 ("mptcp: do not block subflows creation on errors")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/pm.c       |  5 ++---
> net/mptcp/protocol.h | 14 ++++++++++++++
> net/mptcp/subflow.c  | 12 +++++++++---
> 3 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index cdc2d79071f8..59fdab2d0c27 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -181,15 +181,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
> 	struct mptcp_pm_data *pm = &msk->pm;
> 	bool update_subflows;
>
> -	update_subflows = (ssk->sk_state == TCP_CLOSE) &&
> -			  (subflow->request_join || subflow->mp_join) &&
> +	update_subflows = (subflow->request_join || subflow->mp_join) &&
> 			  mptcp_pm_is_kernel(msk);
> 	if (!READ_ONCE(pm->work_pending) && !update_subflows)
> 		return;
>
> 	spin_lock_bh(&pm->lock);
> 	if (update_subflows)
> -		pm->subflows--;
> +		__mptcp_pm_close_subflow(msk);
>
> 	/* Even if this subflow is not really established, tell the PM to try
> 	 * to pick the next ones, if possible.
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 4672901d0dfe..06b8ebc15204 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -893,6 +893,20 @@ unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
> unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);
> unsigned int mptcp_pm_get_local_addr_max(const struct mptcp_sock *msk);
>
> +/* called under PM lock */
> +static inline void __mptcp_pm_close_subflow(struct mptcp_sock *msk)
> +{
> +	if (--msk->pm.subflows < mptcp_pm_get_subflows_max(msk))
> +		WRITE_ONCE(msk->pm.accept_subflow, true);
> +}
> +
> +static inline void mptcp_pm_close_subflow(struct mptcp_sock *msk)
> +{
> +	spin_lock_bh(&msk->pm.lock);
> +	__mptcp_pm_close_subflow(msk);
> +	spin_unlock_bh(&msk->pm.lock);
> +}
> +
> void mptcp_sockopt_sync(struct mptcp_sock *msk, struct sock *ssk);
> void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk);
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 6d59336a8e1e..75c85ce9a19e 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1444,20 +1444,20 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
> 	struct sockaddr_storage addr;
> 	int remote_id = remote->id;
> 	int local_id = loc->id;
> +	int err = -ENOTCONN;
> 	struct socket *sf;
> 	struct sock *ssk;
> 	u32 remote_token;
> 	int addrlen;
> 	int ifindex;
> 	u8 flags;
> -	int err;
>
> 	if (!mptcp_is_fully_established(sk))
> -		return -ENOTCONN;
> +		goto err_out;
>
> 	err = mptcp_subflow_create_socket(sk, &sf);
> 	if (err)
> -		return err;
> +		goto err_out;
>
> 	ssk = sf->sk;
> 	subflow = mptcp_subflow_ctx(ssk);
> @@ -1515,6 +1515,12 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
> failed:
> 	subflow->disposable = 1;
> 	sock_release(sf);
> +
> +err_out:
> +	/* we account subflows before the creation, and this failures will not
> +	 * be catched by sk_state_change()
> +	 */
> +	mptcp_pm_close_subflow(msk);

__mptcp_subflow_connect() is also called by the userspace PM, which 
doesn't use this accounting. How about having the caller be responsible 
for doing mptcp_pm_close_subflow() if __mptcp_subflow_connect() returns an 
error other than -EINPROGRESS?

> 	return err;
> }
>
> -- 
> 2.35.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-net 1/2] mptcp: fix subflow accounting on close

[Mat Martineau]

On Fri, 29 Apr 2022, Mat Martineau wrote:

> On Thu, 28 Apr 2022, Paolo Abeni wrote:
>
>> If the PM closes a fully established MPJ subflow or the subflow
>> creation errors out in it's early stage the subflows counter is
>> not bumped accordingly.
>> 
>> This change adds the missing accouting, additionally taking care
>> of updating accordingly the 'accept_subflow' flag.
>> 
>> Fixes: a88c9e496937 ("mptcp: do not block subflows creation on errors")
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>> net/mptcp/pm.c       |  5 ++---
>> net/mptcp/protocol.h | 14 ++++++++++++++
>> net/mptcp/subflow.c  | 12 +++++++++---
>> 3 files changed, 25 insertions(+), 6 deletions(-)
>> 
>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index cdc2d79071f8..59fdab2d0c27 100644
>> --- a/net/mptcp/pm.c
>> +++ b/net/mptcp/pm.c
>> @@ -181,15 +181,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock 
>> *msk, const struct sock *ssk,
>> 	struct mptcp_pm_data *pm = &msk->pm;
>> 	bool update_subflows;
>> 
>> -	update_subflows = (ssk->sk_state == TCP_CLOSE) &&
>> -			  (subflow->request_join || subflow->mp_join) &&
>> +	update_subflows = (subflow->request_join || subflow->mp_join) &&
>> 			  mptcp_pm_is_kernel(msk);
>> 	if (!READ_ONCE(pm->work_pending) && !update_subflows)
>> 		return;
>>
>> 	spin_lock_bh(&pm->lock);
>> 	if (update_subflows)
>> -		pm->subflows--;
>> +		__mptcp_pm_close_subflow(msk);
>>
>> 	/* Even if this subflow is not really established, tell the PM to try
>> 	 * to pick the next ones, if possible.
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index 4672901d0dfe..06b8ebc15204 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -893,6 +893,20 @@ unsigned int mptcp_pm_get_add_addr_accept_max(const 
>> struct mptcp_sock *msk);
>> unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);
>> unsigned int mptcp_pm_get_local_addr_max(const struct mptcp_sock *msk);
>> 
>> +/* called under PM lock */
>> +static inline void __mptcp_pm_close_subflow(struct mptcp_sock *msk)
>> +{
>> +	if (--msk->pm.subflows < mptcp_pm_get_subflows_max(msk))
>> +		WRITE_ONCE(msk->pm.accept_subflow, true);
>> +}
>> +
>> +static inline void mptcp_pm_close_subflow(struct mptcp_sock *msk)
>> +{
>> +	spin_lock_bh(&msk->pm.lock);
>> +	__mptcp_pm_close_subflow(msk);
>> +	spin_unlock_bh(&msk->pm.lock);
>> +}
>> +
>> void mptcp_sockopt_sync(struct mptcp_sock *msk, struct sock *ssk);
>> void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk);
>> 
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 6d59336a8e1e..75c85ce9a19e 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -1444,20 +1444,20 @@ int __mptcp_subflow_connect(struct sock *sk, const 
>> struct mptcp_addr_info *loc,
>> 	struct sockaddr_storage addr;
>> 	int remote_id = remote->id;
>> 	int local_id = loc->id;
>> +	int err = -ENOTCONN;
>> 	struct socket *sf;
>> 	struct sock *ssk;
>> 	u32 remote_token;
>> 	int addrlen;
>> 	int ifindex;
>> 	u8 flags;
>> -	int err;
>>
>> 	if (!mptcp_is_fully_established(sk))
>> -		return -ENOTCONN;
>> +		goto err_out;
>>
>> 	err = mptcp_subflow_create_socket(sk, &sf);
>> 	if (err)
>> -		return err;
>> +		goto err_out;
>>
>> 	ssk = sf->sk;
>> 	subflow = mptcp_subflow_ctx(ssk);
>> @@ -1515,6 +1515,12 @@ int __mptcp_subflow_connect(struct sock *sk, const 
>> struct mptcp_addr_info *loc,
>> failed:
>> 	subflow->disposable = 1;
>> 	sock_release(sf);
>> +
>> +err_out:
>> +	/* we account subflows before the creation, and this failures will 
>> not
>> +	 * be catched by sk_state_change()
>> +	 */
>> +	mptcp_pm_close_subflow(msk);
>
> __mptcp_subflow_connect() is also called by the userspace PM, which doesn't 
> use this accounting. How about having the caller be responsible for doing 
> mptcp_pm_close_subflow() if __mptcp_subflow_connect() returns an error other 
> than -EINPROGRESS?
>

Hi Paolo,

One other thing - these patches applied cleanly to the export branch, but 
not export-net. The conflicts were not complicated, but in the future 
could you make sure mptcp-net patches apply to export-net?

Thanks,

--
Mat Martineau
Intel

Re: [PATCH mptcp-net 1/2] mptcp: fix subflow accounting on close

[Paolo Abeni]

On Fri, 2022-04-29 at 12:03 -0700, Mat Martineau wrote:
> On Thu, 28 Apr 2022, Paolo Abeni wrote:
> 
> > If the PM closes a fully established MPJ subflow or the subflow
> > creation errors out in it's early stage the subflows counter is
> > not bumped accordingly.
> > 
> > This change adds the missing accouting, additionally taking care
> > of updating accordingly the 'accept_subflow' flag.
> > 
> > Fixes: a88c9e496937 ("mptcp: do not block subflows creation on errors")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/pm.c       |  5 ++---
> > net/mptcp/protocol.h | 14 ++++++++++++++
> > net/mptcp/subflow.c  | 12 +++++++++---
> > 3 files changed, 25 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index cdc2d79071f8..59fdab2d0c27 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -181,15 +181,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
> > 	struct mptcp_pm_data *pm = &msk->pm;
> > 	bool update_subflows;
> > 
> > -	update_subflows = (ssk->sk_state == TCP_CLOSE) &&
> > -			  (subflow->request_join || subflow->mp_join) &&
> > +	update_subflows = (subflow->request_join || subflow->mp_join) &&
> > 			  mptcp_pm_is_kernel(msk);
> > 	if (!READ_ONCE(pm->work_pending) && !update_subflows)
> > 		return;
> > 
> > 	spin_lock_bh(&pm->lock);
> > 	if (update_subflows)
> > -		pm->subflows--;
> > +		__mptcp_pm_close_subflow(msk);
> > 
> > 	/* Even if this subflow is not really established, tell the PM to try
> > 	 * to pick the next ones, if possible.
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 4672901d0dfe..06b8ebc15204 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -893,6 +893,20 @@ unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
> > unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);
> > unsigned int mptcp_pm_get_local_addr_max(const struct mptcp_sock *msk);
> > 
> > +/* called under PM lock */
> > +static inline void __mptcp_pm_close_subflow(struct mptcp_sock *msk)
> > +{
> > +	if (--msk->pm.subflows < mptcp_pm_get_subflows_max(msk))
> > +		WRITE_ONCE(msk->pm.accept_subflow, true);
> > +}
> > +
> > +static inline void mptcp_pm_close_subflow(struct mptcp_sock *msk)
> > +{
> > +	spin_lock_bh(&msk->pm.lock);
> > +	__mptcp_pm_close_subflow(msk);
> > +	spin_unlock_bh(&msk->pm.lock);
> > +}
> > +
> > void mptcp_sockopt_sync(struct mptcp_sock *msk, struct sock *ssk);
> > void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk);
> > 
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 6d59336a8e1e..75c85ce9a19e 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -1444,20 +1444,20 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
> > 	struct sockaddr_storage addr;
> > 	int remote_id = remote->id;
> > 	int local_id = loc->id;
> > +	int err = -ENOTCONN;
> > 	struct socket *sf;
> > 	struct sock *ssk;
> > 	u32 remote_token;
> > 	int addrlen;
> > 	int ifindex;
> > 	u8 flags;
> > -	int err;
> > 
> > 	if (!mptcp_is_fully_established(sk))
> > -		return -ENOTCONN;
> > +		goto err_out;
> > 
> > 	err = mptcp_subflow_create_socket(sk, &sf);
> > 	if (err)
> > -		return err;
> > +		goto err_out;
> > 
> > 	ssk = sf->sk;
> > 	subflow = mptcp_subflow_ctx(ssk);
> > @@ -1515,6 +1515,12 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
> > failed:
> > 	subflow->disposable = 1;
> > 	sock_release(sf);
> > +
> > +err_out:
> > +	/* we account subflows before the creation, and this failures will not
> > +	 * be catched by sk_state_change()
> > +	 */
> > +	mptcp_pm_close_subflow(msk);
> 
> __mptcp_subflow_connect() is also called by the userspace PM, which 
> doesn't use this accounting. How about having the caller be responsible 
> for doing mptcp_pm_close_subflow() if __mptcp_subflow_connect() returns an 
> error other than -EINPROGRESS?

Additionally we probably want to take care of  to take care of
mptcp_pm_subflow_check_next(), which is called regardless of the active
PM and can touch subflows, too.

On the flip side, touching 'subflows' when the active PM is the user-
space one should not have any effect - beyond likely be confusing.

I propose to handle all the above with a cleanup net-next patch after
the relevant -net merge, so we can leverage the mptcp_pm_is_userspace()
helper and keep the code as simple as possible.

WDYT?

Thanks!

Paolo

Re: [PATCH mptcp-net 1/2] mptcp: fix subflow accounting on close

[Mat Martineau]

On Mon, 2 May 2022, Paolo Abeni wrote:

> On Fri, 2022-04-29 at 12:03 -0700, Mat Martineau wrote:
>> On Thu, 28 Apr 2022, Paolo Abeni wrote:
>>
>>> If the PM closes a fully established MPJ subflow or the subflow
>>> creation errors out in it's early stage the subflows counter is
>>> not bumped accordingly.
>>>
>>> This change adds the missing accouting, additionally taking care
>>> of updating accordingly the 'accept_subflow' flag.
>>>
>>> Fixes: a88c9e496937 ("mptcp: do not block subflows creation on errors")
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> net/mptcp/pm.c       |  5 ++---
>>> net/mptcp/protocol.h | 14 ++++++++++++++
>>> net/mptcp/subflow.c  | 12 +++++++++---
>>> 3 files changed, 25 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>> index cdc2d79071f8..59fdab2d0c27 100644
>>> --- a/net/mptcp/pm.c
>>> +++ b/net/mptcp/pm.c
>>> @@ -181,15 +181,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
>>> 	struct mptcp_pm_data *pm = &msk->pm;
>>> 	bool update_subflows;
>>>
>>> -	update_subflows = (ssk->sk_state == TCP_CLOSE) &&
>>> -			  (subflow->request_join || subflow->mp_join) &&
>>> +	update_subflows = (subflow->request_join || subflow->mp_join) &&
>>> 			  mptcp_pm_is_kernel(msk);
>>> 	if (!READ_ONCE(pm->work_pending) && !update_subflows)
>>> 		return;
>>>
>>> 	spin_lock_bh(&pm->lock);
>>> 	if (update_subflows)
>>> -		pm->subflows--;
>>> +		__mptcp_pm_close_subflow(msk);
>>>
>>> 	/* Even if this subflow is not really established, tell the PM to try
>>> 	 * to pick the next ones, if possible.
>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>> index 4672901d0dfe..06b8ebc15204 100644
>>> --- a/net/mptcp/protocol.h
>>> +++ b/net/mptcp/protocol.h
>>> @@ -893,6 +893,20 @@ unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
>>> unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);
>>> unsigned int mptcp_pm_get_local_addr_max(const struct mptcp_sock *msk);
>>>
>>> +/* called under PM lock */
>>> +static inline void __mptcp_pm_close_subflow(struct mptcp_sock *msk)
>>> +{
>>> +	if (--msk->pm.subflows < mptcp_pm_get_subflows_max(msk))
>>> +		WRITE_ONCE(msk->pm.accept_subflow, true);
>>> +}
>>> +
>>> +static inline void mptcp_pm_close_subflow(struct mptcp_sock *msk)
>>> +{
>>> +	spin_lock_bh(&msk->pm.lock);
>>> +	__mptcp_pm_close_subflow(msk);
>>> +	spin_unlock_bh(&msk->pm.lock);
>>> +}
>>> +
>>> void mptcp_sockopt_sync(struct mptcp_sock *msk, struct sock *ssk);
>>> void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk);
>>>
>>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>>> index 6d59336a8e1e..75c85ce9a19e 100644
>>> --- a/net/mptcp/subflow.c
>>> +++ b/net/mptcp/subflow.c
>>> @@ -1444,20 +1444,20 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
>>> 	struct sockaddr_storage addr;
>>> 	int remote_id = remote->id;
>>> 	int local_id = loc->id;
>>> +	int err = -ENOTCONN;
>>> 	struct socket *sf;
>>> 	struct sock *ssk;
>>> 	u32 remote_token;
>>> 	int addrlen;
>>> 	int ifindex;
>>> 	u8 flags;
>>> -	int err;
>>>
>>> 	if (!mptcp_is_fully_established(sk))
>>> -		return -ENOTCONN;
>>> +		goto err_out;
>>>
>>> 	err = mptcp_subflow_create_socket(sk, &sf);
>>> 	if (err)
>>> -		return err;
>>> +		goto err_out;
>>>
>>> 	ssk = sf->sk;
>>> 	subflow = mptcp_subflow_ctx(ssk);
>>> @@ -1515,6 +1515,12 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
>>> failed:
>>> 	subflow->disposable = 1;
>>> 	sock_release(sf);
>>> +
>>> +err_out:
>>> +	/* we account subflows before the creation, and this failures will not
>>> +	 * be catched by sk_state_change()
>>> +	 */
>>> +	mptcp_pm_close_subflow(msk);
>>
>> __mptcp_subflow_connect() is also called by the userspace PM, which
>> doesn't use this accounting. How about having the caller be responsible
>> for doing mptcp_pm_close_subflow() if __mptcp_subflow_connect() returns an
>> error other than -EINPROGRESS?
>
> Additionally we probably want to take care of  to take care of
> mptcp_pm_subflow_check_next(), which is called regardless of the active
> PM and can touch subflows, too.
>
> On the flip side, touching 'subflows' when the active PM is the user-
> space one should not have any effect - beyond likely be confusing.
>
> I propose to handle all the above with a cleanup net-next patch after
> the relevant -net merge, so we can leverage the mptcp_pm_is_userspace()
> helper and keep the code as simple as possible.
>
> WDYT?
>

That plan works for me, thanks.

--
Mat Martineau
Intel

Re: [PATCH mptcp-net 1/2] mptcp: fix subflow accounting on close

[Matthieu Baerts]

Hi Paolo, Mat,

On 28/04/2022 18:58, Paolo Abeni wrote:
> If the PM closes a fully established MPJ subflow or the subflow
> creation errors out in it's early stage the subflows counter is
> not bumped accordingly.
> 
> This change adds the missing accouting, additionally taking care
> of updating accordingly the 'accept_subflow' flag.

Thank you for the patches and the reviews!

Now in our tree (fix for -net) with Mat's RvB tag and without some typos
reported by checkpatch.pl --codespell.

I indeed had 2 small conflicts to apply them (and the same later when
propagating the modifications):


New patches for t/upstream:
- 7434c8d5e0c2: mptcp: fix subflow accounting on close
- f61a56256a0a: selftests: mptcp: add subflow limits test-cases
- d6c3691c8ed4: conflict in
top-bases/t/DO-NOT-MERGE-git-markup-fixes-net-next
- 138248ead67a: conflict in
t/mptcp-bypass-in-kernel-PM-restrictions-for-non-kernel-PMs
- Results: 6e5da9dfbfac..bea7cea0ffcd (export)

New patches for t/upstream-net:
- 7434c8d5e0c2: mptcp: fix subflow accounting on close
- f61a56256a0a: selftests: mptcp: add subflow limits test-cases
- d6c3691c8ed4: conflict in
top-bases/t/DO-NOT-MERGE-git-markup-fixes-net-next
- 138248ead67a: conflict in
t/mptcp-bypass-in-kernel-PM-restrictions-for-non-kernel-PMs
- Results: 9f40b4259885..d3d6ac28f754 (export-net)


Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220503T140124
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20220503T140124
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export-net


Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net