[PATCH mptcp-net 0/2] mptcp: fix missing close transition

[PATCH mptcp-net 0/2] mptcp: fix missing close transition

[Paolo Abeni]

This is an attempt to address the 2nd issue reported in:

https://github.com/multipath-tcp/mptcp_net-next/issues/427

See the individual patch changelog for the details. The cover letter
is here mainly to trigger the CI.

Paolo Abeni (2):
  mptcp: move __mptcp_error_report in protocol.c
  mptcp: process pending subflow error on close

 net/mptcp/protocol.c | 41 +++++++++++++++++++++++++++++++++++++++++
 net/mptcp/subflow.c  | 36 ------------------------------------
 2 files changed, 41 insertions(+), 36 deletions(-)

-- 
2.41.0

[PATCH mptcp-net 1/2] mptcp: move __mptcp_error_report in protocol.c

[Paolo Abeni]

This will simplify the next patch. No functional change intended.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 36 ++++++++++++++++++++++++++++++++++++
 net/mptcp/subflow.c  | 36 ------------------------------------
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ab3aaf6cacc5..12d3b575ceba 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -764,6 +764,42 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
 	return moved;
 }
 
+void __mptcp_error_report(struct sock *sk)
+{
+	struct mptcp_subflow_context *subflow;
+	struct mptcp_sock *msk = mptcp_sk(sk);
+
+	mptcp_for_each_subflow(msk, subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+		int err = sock_error(ssk);
+		int ssk_state;
+
+		if (!err)
+			continue;
+
+		/* only propagate errors on fallen-back sockets or
+		 * on MPC connect
+		 */
+		if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(msk))
+			continue;
+
+		/* We need to propagate only transition to CLOSE state.
+		 * Orphaned socket will see such state change via
+		 * subflow_sched_work_if_closed() and that path will properly
+		 * destroy the msk as needed.
+		 */
+		ssk_state = inet_sk_state_load(ssk);
+		if (ssk_state == TCP_CLOSE && !sock_flag(sk, SOCK_DEAD))
+			inet_sk_state_store(sk, ssk_state);
+		WRITE_ONCE(sk->sk_err, -err);
+
+		/* This barrier is coupled with smp_rmb() in mptcp_poll() */
+		smp_wmb();
+		sk_error_report(sk);
+		break;
+	}
+}
+
 /* In most cases we will be able to lock the mptcp socket.  If its already
  * owned, we need to defer to the work queue to avoid ABBA deadlock.
  */
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 9bf3c7bc1762..2f40c23fdb0d 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1362,42 +1362,6 @@ void mptcp_space(const struct sock *ssk, int *space, int *full_space)
 	*full_space = mptcp_win_from_space(sk, READ_ONCE(sk->sk_rcvbuf));
 }
 
-void __mptcp_error_report(struct sock *sk)
-{
-	struct mptcp_subflow_context *subflow;
-	struct mptcp_sock *msk = mptcp_sk(sk);
-
-	mptcp_for_each_subflow(msk, subflow) {
-		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
-		int err = sock_error(ssk);
-		int ssk_state;
-
-		if (!err)
-			continue;
-
-		/* only propagate errors on fallen-back sockets or
-		 * on MPC connect
-		 */
-		if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(msk))
-			continue;
-
-		/* We need to propagate only transition to CLOSE state.
-		 * Orphaned socket will see such state change via
-		 * subflow_sched_work_if_closed() and that path will properly
-		 * destroy the msk as needed.
-		 */
-		ssk_state = inet_sk_state_load(ssk);
-		if (ssk_state == TCP_CLOSE && !sock_flag(sk, SOCK_DEAD))
-			inet_sk_state_store(sk, ssk_state);
-		WRITE_ONCE(sk->sk_err, -err);
-
-		/* This barrier is coupled with smp_rmb() in mptcp_poll() */
-		smp_wmb();
-		sk_error_report(sk);
-		break;
-	}
-}
-
 static void subflow_error_report(struct sock *ssk)
 {
 	struct sock *sk = mptcp_subflow_ctx(ssk)->conn;
-- 
2.41.0

[PATCH mptcp-net 2/2] mptcp: process pending subflow error on close

[Paolo Abeni]

On incoming TCP reset, subflow closing could happen before error
propagation. That in turn could cause the socket error being ignored,
and a missing socket state transition, as reported by Daire-Byrne.

Address the issues explicitly checking for subflow socket error at
close time. To avoid code duplication, factor-out of __mptcp_error_report()
a new helper implementing the relevant bits.

Reported-by: Daire-Byrne
Fixes: 15cc10453398 ("mptcp: deliver ssk errors to msk")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
Note: the fixes tag need a real email address...
---
 net/mptcp/protocol.c | 63 ++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 12d3b575ceba..7ab5391b2778 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -764,40 +764,44 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
 	return moved;
 }
 
-void __mptcp_error_report(struct sock *sk)
+bool __mptcp_subflow_error_report(struct sock *sk, struct sock *ssk)
 {
-	struct mptcp_subflow_context *subflow;
-	struct mptcp_sock *msk = mptcp_sk(sk);
+	int err = sock_error(ssk);
+	int ssk_state;
 
-	mptcp_for_each_subflow(msk, subflow) {
-		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
-		int err = sock_error(ssk);
-		int ssk_state;
+	if (!err)
+		return false;
 
-		if (!err)
-			continue;
+	/* only propagate errors on fallen-back sockets or
+	 * on MPC connect
+	 */
+	if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(mptcp_sk(sk)))
+		return false;
 
-		/* only propagate errors on fallen-back sockets or
-		 * on MPC connect
-		 */
-		if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(msk))
-			continue;
+	/* We need to propagate only transition to CLOSE state.
+	 * Orphaned socket will see such state change via
+	 * subflow_sched_work_if_closed() and that path will properly
+	 * destroy the msk as needed.
+	 */
+	ssk_state = inet_sk_state_load(ssk);
+	if (ssk_state == TCP_CLOSE && !sock_flag(sk, SOCK_DEAD))
+		inet_sk_state_store(sk, ssk_state);
+	WRITE_ONCE(sk->sk_err, -err);
 
-		/* We need to propagate only transition to CLOSE state.
-		 * Orphaned socket will see such state change via
-		 * subflow_sched_work_if_closed() and that path will properly
-		 * destroy the msk as needed.
-		 */
-		ssk_state = inet_sk_state_load(ssk);
-		if (ssk_state == TCP_CLOSE && !sock_flag(sk, SOCK_DEAD))
-			inet_sk_state_store(sk, ssk_state);
-		WRITE_ONCE(sk->sk_err, -err);
-
-		/* This barrier is coupled with smp_rmb() in mptcp_poll() */
-		smp_wmb();
-		sk_error_report(sk);
-		break;
-	}
+	/* This barrier is coupled with smp_rmb() in mptcp_poll() */
+	smp_wmb();
+	sk_error_report(sk);
+	return true;
+}
+
+void __mptcp_error_report(struct sock *sk)
+{
+	struct mptcp_subflow_context *subflow;
+	struct mptcp_sock *msk = mptcp_sk(sk);
+
+	mptcp_for_each_subflow(msk, subflow)
+		if (__mptcp_subflow_error_report(sk, mptcp_subflow_tcp_sock(subflow)))
+			break;
 }
 
 /* In most cases we will be able to lock the mptcp socket.  If its already
@@ -2422,6 +2426,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	}
 
 out_release:
+	__mptcp_subflow_error_report(sk, ssk);
 	release_sock(ssk);
 
 	sock_put(ssk);
-- 
2.41.0

Re: mptcp: process pending subflow error on close: Build Failure

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://patchwork.kernel.org/project/mptcp/patch/065ed2c95b4ceef822232e4d0641427af6e27092.1692690157.git.pabeni@redhat.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/5936024982

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/dffd1aea5f08

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: mptcp: process pending subflow error on close: Tests Results

[MPTCP CI]

Hi Paolo,

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/4696377448464384
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4696377448464384/summary/summary.txt

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

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

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

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


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 (Tessares)

Re: [PATCH mptcp-net 2/2] mptcp: process pending subflow error on close

[kernel test robot]

Hi Paolo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on next-20230821]
[also build test WARNING on v6.5-rc7]
[cannot apply to linus/master v6.5-rc7 v6.5-rc6 v6.5-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paolo-Abeni/mptcp-move-__mptcp_error_report-in-protocol-c/20230822-154511
base:   next-20230821
patch link:    https://lore.kernel.org/r/065ed2c95b4ceef822232e4d0641427af6e27092.1692690157.git.pabeni%40redhat.com
patch subject: [PATCH mptcp-net 2/2] mptcp: process pending subflow error on close
config: sparc64-randconfig-r033-20230823 (https://download.01.org/0day-ci/archive/20230823/202308230746.DvdUqzMi-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230823/202308230746.DvdUqzMi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308230746.DvdUqzMi-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/mptcp/protocol.c:767:6: warning: no previous prototype for '__mptcp_subflow_error_report' [-Wmissing-prototypes]
     767 | bool __mptcp_subflow_error_report(struct sock *sk, struct sock *ssk)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/__mptcp_subflow_error_report +767 net/mptcp/protocol.c

   766	
 > 767	bool __mptcp_subflow_error_report(struct sock *sk, struct sock *ssk)
   768	{
   769		int err = sock_error(ssk);
   770		int ssk_state;
   771	
   772		if (!err)
   773			return false;
   774	
   775		/* only propagate errors on fallen-back sockets or
   776		 * on MPC connect
   777		 */
   778		if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(mptcp_sk(sk)))
   779			return false;
   780	
   781		/* We need to propagate only transition to CLOSE state.
   782		 * Orphaned socket will see such state change via
   783		 * subflow_sched_work_if_closed() and that path will properly
   784		 * destroy the msk as needed.
   785		 */
   786		ssk_state = inet_sk_state_load(ssk);
   787		if (ssk_state == TCP_CLOSE && !sock_flag(sk, SOCK_DEAD))
   788			inet_sk_state_store(sk, ssk_state);
   789		WRITE_ONCE(sk->sk_err, -err);
   790	
   791		/* This barrier is coupled with smp_rmb() in mptcp_poll() */
   792		smp_wmb();
   793		sk_error_report(sk);
   794		return true;
   795	}
   796	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH mptcp-net 0/2] mptcp: fix missing close transition

[Matthieu Baerts]

Hi Paolo, Mat,

On 22/08/2023 09:43, Paolo Abeni wrote:
> This is an attempt to address the 2nd issue reported in:
> 
> https://github.com/multipath-tcp/mptcp_net-next/issues/427
> 
> See the individual patch changelog for the details. The cover letter
> is here mainly to trigger the CI.
> 
> Paolo Abeni (2):
>   mptcp: move __mptcp_error_report in protocol.c
>   mptcp: process pending subflow error on close

Thank you for the patches and the reviews!

Sorry for the delay, now in our tree (fixes for -net):

New patches for t/upstream-net and t/upstream:
- 9e7269dca15f: mptcp: move __mptcp_error_report in protocol.c
- 6106d8bdbfcf: mptcp: process pending subflow error on close
- Results: fa580ac35599..d415340e7fb3 (export-net)
- Results: 54ea4fcf4272..93aaf3d1e3d6 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230829T170007
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230829T170007

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