[PATCH mptcp-next 0/5] fixes for CURRESTAB

[PATCH mptcp-next 0/5] fixes for CURRESTAB

[Geliang Tang]

Address Matt & Paolo's comments for CURRESTAB series.

Geliang Tang (5):
  Squash to "mptcp: use mptcp_set_state" 1
  mptcp: move inet_sk_state_store under lock
  Squash to "mptcp: use mptcp_set_state" 2
  Squash to "selftests: mptcp: check CURRESTAB counters"
  selftests: mptcp: diag: check CURRESTAB counters

 net/mptcp/pm_netlink.c                        | 16 +++++-----
 tools/testing/selftests/net/mptcp/diag.sh     | 30 +++++++++++++++++++
 .../testing/selftests/net/mptcp/mptcp_join.sh |  5 ++--
 3 files changed, 42 insertions(+), 9 deletions(-)

-- 
2.35.3

[PATCH mptcp-next 1/5] Squash to "mptcp: use mptcp_set_state" 1

[Geliang Tang]

Remove the comment.

Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/pm_netlink.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index b93683b5e618..bf4d96f6f99a 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1048,9 +1048,6 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	if (err)
 		return err;
 
-	/* avoid replacing inet_sk_state_store with mptcp_set_state here, as the
-	 * old status is known to be TCP_CLOSE, hence will not affect the count.
-	 */
 	inet_sk_state_store(newsk, TCP_LISTEN);
 	lock_sock(ssk);
 	err = __inet_listen_sk(ssk, backlog);
-- 
2.35.3

[PATCH mptcp-next 2/5] mptcp: move inet_sk_state_store under lock

[Geliang Tang]

This patch moves inet_sk_state_store() under the socket lock in
mptcp_pm_nl_create_listen_socket(). This is a pre-req patch for
using mptcp_set_state() instead of inet_sk_state_store().

Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/pm_netlink.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index bf4d96f6f99a..0e6da322c8f2 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1030,9 +1030,10 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 
 	lock_sock(newsk);
 	ssk = __mptcp_nmpc_sk(mptcp_sk(newsk));
-	release_sock(newsk);
-	if (IS_ERR(ssk))
+	if (IS_ERR(ssk)) {
+		release_sock(newsk);
 		return PTR_ERR(ssk);
+	}
 
 	mptcp_info2sockaddr(&entry->addr, &addr, entry->addr.family);
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
@@ -1045,10 +1046,14 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	else if (ssk->sk_family == AF_INET6)
 		err = inet6_bind_sk(ssk, (struct sockaddr *)&addr, addrlen);
 #endif
-	if (err)
+	if (err) {
+		release_sock(newsk);
 		return err;
+	}
 
 	inet_sk_state_store(newsk, TCP_LISTEN);
+	release_sock(newsk);
+
 	lock_sock(ssk);
 	err = __inet_listen_sk(ssk, backlog);
 	if (!err)
-- 
2.35.3

[PATCH mptcp-next 3/5] Squash to "mptcp: use mptcp_set_state" 2

[Geliang Tang]

Use mptcp_set_state() in mptcp_pm_nl_create_listen_socket().

Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/pm_netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 0e6da322c8f2..c84cc0908cfc 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1051,7 +1051,7 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 		return err;
 	}
 
-	inet_sk_state_store(newsk, TCP_LISTEN);
+	mptcp_set_state(newsk, TCP_LISTEN);
 	release_sock(newsk);
 
 	lock_sock(ssk);
-- 
2.35.3

[PATCH mptcp-next 4/5] Squash to "selftests: mptcp: check CURRESTAB counters"

[Geliang Tang]

Add a comment for check_cestab(), and use cestab_ns1/cestab_ns2 in it as
Matt suggested.

Please update the subject as:

'''
selftests: mptcp: join: check CURRESTAB counters
'''

Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 3cd066e6e2b0..3a5b63026191 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -995,13 +995,14 @@ chk_cestab_nr()
 	fi
 }
 
+# $1 namespace 1, $2 namespace 2
 check_cestab()
 {
 	if [ -n "${cestab_ns1}" ]; then
-		chk_cestab_nr $1 1
+		chk_cestab_nr ${1} ${cestab_ns1}
 	fi
 	if [ -n "${cestab_ns2}" ]; then
-		chk_cestab_nr $2 1
+		chk_cestab_nr ${2} ${cestab_ns2}
 	fi
 }
 
-- 
2.35.3

[PATCH mptcp-next 5/5] selftests: mptcp: diag: check CURRESTAB counters

[Geliang Tang]

This patch adds a new helper chk_msk_cestab() to check the current
established connections counter MIB_CURRESTAB in diag.sh. Invoke it
to check the counter during the connection after every chk_msk_inuse().

---
Note: chk_msk_cestab() and chk_cestab_nr() will be replaced by
mptcp_lib_chk_cestab_nr() in subsequent patches after the commit
"selftests: mptcp: diag: print colored output"
---

Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 tools/testing/selftests/net/mptcp/diag.sh | 30 +++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 95b498efacd1..aa341f40363c 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -182,6 +182,30 @@ chk_msk_inuse()
 	__chk_nr get_msk_inuse $expected "$msg" 0
 }
 
+# $1: ns, $2: cestab nr
+chk_msk_cestab()
+{
+	local ns=$1
+	local cestab=$2
+	local count
+	local msg="....chk $2 cestab"
+	test_cnt=$((test_cnt+1))
+
+	printf "%-50s" "$msg"
+
+	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtMPCurrEstab")
+	if [ -z "$count" ]; then
+		echo "[ skip ]"
+		mptcp_lib_result_skip "${msg}"
+	elif [ "$count" != "$cestab" ]; then
+		echo "[ fail ] got $count current establish[s] expected $cestab"
+		ret=${KSFT_FAIL}
+	else
+		echo "[  ok  ]"
+		mptcp_lib_result_pass "$msg"
+	fi
+}
+
 wait_connected()
 {
 	local listener_ns="${1}"
@@ -219,9 +243,11 @@ chk_msk_nr 2 "after MPC handshake "
 chk_msk_remote_key_nr 2 "....chk remote_key"
 chk_msk_fallback_nr 0 "....chk no fallback"
 chk_msk_inuse 2 "....chk 2 msk in use"
+chk_msk_cestab $ns 2
 flush_pids
 
 chk_msk_inuse 0 "....chk 0 msk in use after flush"
+chk_msk_cestab $ns 0
 
 echo "a" | \
 	timeout ${timeout_test} \
@@ -237,9 +263,11 @@ echo "b" | \
 wait_connected $ns 10001
 chk_msk_fallback_nr 1 "check fallback"
 chk_msk_inuse 1 "....chk 1 msk in use"
+chk_msk_cestab $ns 1
 flush_pids
 
 chk_msk_inuse 0 "....chk 0 msk in use after flush"
+chk_msk_cestab $ns 0
 
 NR_CLIENTS=100
 for I in `seq 1 $NR_CLIENTS`; do
@@ -261,9 +289,11 @@ done
 
 wait_msk_nr $((NR_CLIENTS*2)) "many msk socket present"
 chk_msk_inuse $((NR_CLIENTS*2)) "....chk many msk in use"
+chk_msk_cestab $ns 200
 flush_pids
 
 chk_msk_inuse 0 "....chk 0 msk in use after flush"
+chk_msk_cestab $ns 0
 
 mptcp_lib_result_print_all_tap
 exit $ret
-- 
2.35.3

Re: selftests: mptcp: diag: check CURRESTAB counters: Tests Results

[MPTCP CI]

Hi Geliang,

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):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5273686110896128
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5273686110896128/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/6118111041028096
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6118111041028096/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6399586017738752
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6399586017738752/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_diag 🔴:
  - Task: https://cirrus-ci.com/task/4992211134185472
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4992211134185472/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b3cc0559a864


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-debug

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-next 5/5] selftests: mptcp: diag: check CURRESTAB counters

[Matthieu Baerts]

Hi Geliang,

On 18/12/2023 09:47, Geliang Tang wrote:
> This patch adds a new helper chk_msk_cestab() to check the current
> established connections counter MIB_CURRESTAB in diag.sh. Invoke it
> to check the counter during the connection after every chk_msk_inuse().

Thank you for this new test!

> ---
> Note: chk_msk_cestab() and chk_cestab_nr() will be replaced by
> mptcp_lib_chk_cestab_nr() in subsequent patches after the commit
> "selftests: mptcp: diag: print colored output"

Thank you for the note!

(a note should be after the last Signed-off-by or added with 'git notes'
+ 'git format-patch --notes')

> ---
> 
> Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
> ---
>  tools/testing/selftests/net/mptcp/diag.sh | 30 +++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index 95b498efacd1..aa341f40363c 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -182,6 +182,30 @@ chk_msk_inuse()
>  	__chk_nr get_msk_inuse $expected "$msg" 0
>  }
>  
> +# $1: ns, $2: cestab nr
> +chk_msk_cestab()> +{
> +	local ns=$1
> +	local cestab=$2
> +	local count
> +	local msg="....chk $2 cestab"
> +	test_cnt=$((test_cnt+1))

Not sure if it is useful, but usually we increment it at the end, and we
return it in case of failure.

> +
> +	printf "%-50s" "$msg"
> +
> +	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtMPCurrEstab")
> +	if [ -z "$count" ]; then
> +		echo "[ skip ]"
> +		mptcp_lib_result_skip "${msg}"
> +	elif [ "$count" != "$cestab" ]; then
> +		echo "[ fail ] got $count current establish[s] expected $cestab"
> +		ret=${KSFT_FAIL}
> +	else
> +		echo "[  ok  ]"
> +		mptcp_lib_result_pass "$msg"
> +	fi

Can you not use __chk_nr() instead?

> chk_msk_cestab()
> {
>     local ns=$1
>     local cestab=$2
> 
>     __chk_nr "mptcp_lib_get_counter ${ns} MPTcpExtMPCurrEstab" "${cestab}" "....chk ${cestab} cestab" ""
> }

I didn't check but I guess you will have to replace this line in
__chk_nr(), from:

  local skip="${4:-SKIP}"

to:

  local skip="${4-SKIP}"

(so it will only replace $4 by "SKIP" if it is not defined, even if it
is empty)

> +}
> +
>  wait_connected()
>  {
>  	local listener_ns="${1}"
> @@ -219,9 +243,11 @@ chk_msk_nr 2 "after MPC handshake "
>  chk_msk_remote_key_nr 2 "....chk remote_key"
>  chk_msk_fallback_nr 0 "....chk no fallback"
>  chk_msk_inuse 2 "....chk 2 msk in use"
> +chk_msk_cestab $ns 2
>  flush_pids
>  
>  chk_msk_inuse 0 "....chk 0 msk in use after flush"
> +chk_msk_cestab $ns 0
>  
>  echo "a" | \
>  	timeout ${timeout_test} \
> @@ -237,9 +263,11 @@ echo "b" | \
>  wait_connected $ns 10001
>  chk_msk_fallback_nr 1 "check fallback"
>  chk_msk_inuse 1 "....chk 1 msk in use"
> +chk_msk_cestab $ns 1
>  flush_pids
>  
>  chk_msk_inuse 0 "....chk 0 msk in use after flush"
> +chk_msk_cestab $ns 0
>  
>  NR_CLIENTS=100
>  for I in `seq 1 $NR_CLIENTS`; do
> @@ -261,9 +289,11 @@ done
>  
>  wait_msk_nr $((NR_CLIENTS*2)) "many msk socket present"
>  chk_msk_inuse $((NR_CLIENTS*2)) "....chk many msk in use"
> +chk_msk_cestab $ns 200

The CI complained about this test:

https://api.cirrus-ci.com/v1/artifact/task/4992211134185472/summary/summary.txt

But the previous test also failed:

>   ....chk many msk in use        [ fail ] expected 252 found 239
>   ....chk 200 cestab             [ fail ] got 176 current establish[s] expected 200

Maybe just unlucky this time?

>  flush_pids
>  
>  chk_msk_inuse 0 "....chk 0 msk in use after flush"
> +chk_msk_cestab $ns 0
>  
>  mptcp_lib_result_print_all_tap
>  exit $ret

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

Re: [PATCH mptcp-next 4/5] Squash to "selftests: mptcp: check CURRESTAB counters"

[Matthieu Baerts]

Hi Geliang,

On 18/12/2023 09:47, Geliang Tang wrote:
> Add a comment for check_cestab(), and use cestab_ns1/cestab_ns2 in it as
> Matt suggested.
> 
> Please update the subject as:
> 
> '''
> selftests: mptcp: join: check CURRESTAB counters
> '''

Thank you, I just applied this patch. I will wait for Paolo's ACK for
1-3/5 and I have a question on patch 5/5.

- c36edd021eb1: "squashed" patch 4/5 in "selftests: mptcp: check
CURRESTAB counters"
- f111e119a46d: tg:msg: add 'join:' prefix
- Results: 84d8fb414b2a..1097ead7bcb9 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20231218T162532

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

Re: [PATCH mptcp-next 2/5] mptcp: move inet_sk_state_store under lock

[Paolo Abeni]

On Mon, 2023-12-18 at 16:47 +0800, Geliang Tang wrote:
> This patch moves inet_sk_state_store() under the socket lock in
> mptcp_pm_nl_create_listen_socket(). This is a pre-req patch for
> using mptcp_set_state() instead of inet_sk_state_store().
> 
> Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
> ---
>  net/mptcp/pm_netlink.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index bf4d96f6f99a..0e6da322c8f2 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1030,9 +1030,10 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>  
>  	lock_sock(newsk);
>  	ssk = __mptcp_nmpc_sk(mptcp_sk(newsk));
> -	release_sock(newsk);
> -	if (IS_ERR(ssk))
> +	if (IS_ERR(ssk)) {
> +		release_sock(newsk);
>  		return PTR_ERR(ssk);
> +	}
>  
>  	mptcp_info2sockaddr(&entry->addr, &addr, entry->addr.family);
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> @@ -1045,10 +1046,14 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>  	else if (ssk->sk_family == AF_INET6)
>  		err = inet6_bind_sk(ssk, (struct sockaddr *)&addr, addrlen);
>  #endif
> -	if (err)
> +	if (err) {
> +		release_sock(newsk);
>  		return err;
> +	}
>  
>  	inet_sk_state_store(newsk, TCP_LISTEN);

If you move 'inet_sk_state_store(newsk, TCP_LISTEN);' just before the
'__mptcp_nmpc_sk()' call, the overall patch should be significantly
smaller.

Cheers,

Paolo

Re: [PATCH mptcp-next 2/5] mptcp: move inet_sk_state_store under lock

[Matthieu Baerts]

Hi Paolo,

On 18/12/2023 17:54, Paolo Abeni wrote:
> On Mon, 2023-12-18 at 16:47 +0800, Geliang Tang wrote:
>> This patch moves inet_sk_state_store() under the socket lock in
>> mptcp_pm_nl_create_listen_socket(). This is a pre-req patch for
>> using mptcp_set_state() instead of inet_sk_state_store().
>>
>> Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
>> ---
>>  net/mptcp/pm_netlink.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index bf4d96f6f99a..0e6da322c8f2 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -1030,9 +1030,10 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>>  
>>  	lock_sock(newsk);
>>  	ssk = __mptcp_nmpc_sk(mptcp_sk(newsk));
>> -	release_sock(newsk);
>> -	if (IS_ERR(ssk))
>> +	if (IS_ERR(ssk)) {
>> +		release_sock(newsk);
>>  		return PTR_ERR(ssk);
>> +	}
>>  
>>  	mptcp_info2sockaddr(&entry->addr, &addr, entry->addr.family);
>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>> @@ -1045,10 +1046,14 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>>  	else if (ssk->sk_family == AF_INET6)
>>  		err = inet6_bind_sk(ssk, (struct sockaddr *)&addr, addrlen);
>>  #endif
>> -	if (err)
>> +	if (err) {
>> +		release_sock(newsk);
>>  		return err;
>> +	}
>>  
>>  	inet_sk_state_store(newsk, TCP_LISTEN);
> 
> If you move 'inet_sk_state_store(newsk, TCP_LISTEN);' just before the
> '__mptcp_nmpc_sk()' call, the overall patch should be significantly
> smaller.

Thank you for having looked!

That's what I had in mind, but I was not sure if there would be an issue
in case of error (and I didn't check), etc.

But if there is no issue, best to move it there indeed!

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

Re: [PATCH mptcp-next 2/5] mptcp: move inet_sk_state_store under lock

[Paolo Abeni]

On Mon, 2023-12-18 at 18:29 +0100, Matthieu Baerts wrote:
> On 18/12/2023 17:54, Paolo Abeni wrote:
> > On Mon, 2023-12-18 at 16:47 +0800, Geliang Tang wrote:
> > > @@ -1045,10 +1046,14 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
> > >  	else if (ssk->sk_family == AF_INET6)
> > >  		err = inet6_bind_sk(ssk, (struct sockaddr *)&addr, addrlen);
> > >  #endif
> > > -	if (err)
> > > +	if (err) {
> > > +		release_sock(newsk);
> > >  		return err;
> > > +	}
> > >  
> > >  	inet_sk_state_store(newsk, TCP_LISTEN);
> > 
> > If you move 'inet_sk_state_store(newsk, TCP_LISTEN);' just before the
> > '__mptcp_nmpc_sk()' call, the overall patch should be significantly
> > smaller.
> 
> Thank you for having looked!
> 
> That's what I had in mind, but I was not sure if there would be an issue
> in case of error (and I didn't check), etc.
> 
> But if there is no issue, best to move it there indeed!

If __mptcp_nmpc_sk() errors out - or any other check in
mptcp_pm_nl_create_listen_socket() - the newsk will be released just
after by __mptcp_pm_release_addr_entry() so there should be any issues.

Cheers,

Paolo

Re: [PATCH mptcp-next 2/5] mptcp: move inet_sk_state_store under lock

[Paolo Abeni]

On Tue, 2023-12-19 at 08:57 +0100, Paolo Abeni wrote:
> On Mon, 2023-12-18 at 18:29 +0100, Matthieu Baerts wrote:
> > That's what I had in mind, but I was not sure if there would be an issue
> > in case of error (and I didn't check), etc.
> > 
> > But if there is no issue, best to move it there indeed!
> 
> If __mptcp_nmpc_sk() errors out - or any other check in
> mptcp_pm_nl_create_listen_socket() - the newsk will be released just
> after by __mptcp_pm_release_addr_entry() so there should be any issues.

... except that such release will call mptcp_close() on the mptcp
socket. If the error happens in inet_bind_sk() or inet6_bind_sk(), the
msk state will be TCP_LISTEN, the first subflow will be already created
- by __mptcp_nmpc_sk() - and the latter socket status will be
TCP_CLOSE, causing a warn in mptcp_close() ->
mptcp_check_listen_stop().

I *think* it's would be better special-case this status change here
using keep using inet_sk_state_store()

Cheers,

Paolo

Re: [PATCH mptcp-next 2/5] mptcp: move inet_sk_state_store under lock

[Matthieu Baerts]

Hi Paolo

On 19/12/2023 09:05, Paolo Abeni wrote:
> On Tue, 2023-12-19 at 08:57 +0100, Paolo Abeni wrote:
>> On Mon, 2023-12-18 at 18:29 +0100, Matthieu Baerts wrote:
>>> That's what I had in mind, but I was not sure if there would be an issue
>>> in case of error (and I didn't check), etc.
>>>
>>> But if there is no issue, best to move it there indeed!
>>
>> If __mptcp_nmpc_sk() errors out - or any other check in
>> mptcp_pm_nl_create_listen_socket() - the newsk will be released just
>> after by __mptcp_pm_release_addr_entry() so there should be any issues.
> 
> ... except that such release will call mptcp_close() on the mptcp
> socket. If the error happens in inet_bind_sk() or inet6_bind_sk(), the
> msk state will be TCP_LISTEN, the first subflow will be already created
> - by __mptcp_nmpc_sk() - and the latter socket status will be
> TCP_CLOSE, causing a warn in mptcp_close() ->
> mptcp_check_listen_stop().
> 
> I *think* it's would be better special-case this status change here
> using keep using inet_sk_state_store()

Thank you for having checked, it was feeling suspicious :)

I agree, we should then keep inet_sk_state_store() for the moment. Then,
I suggest updating the comment, from:

  avoid replacing inet_sk_state_store with mptcp_set_state here, as the
  old status is known to be TCP_CLOSE, hence will not affect the count.

to:

  We don't use mptcp_set_state() here because it needs to be called
  under the msk socket lock. For the moment, that will not bring
  anything more than only calling inet_sk_state_store(), because the old
  status is known (TCP_CLOSE).

(or something similar)

WDYT?

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