diff mbox series

[net-next] Squash-to: "mptcp: refactor shutdown and close"

Message ID fffd74384126974bb9f56303948000f11b0b291f.1602794093.git.pabeni@redhat.com
State Accepted, archived
Commit e3514ae223bd638000b6ae0fd9dd94e05dce637f
Delegated to: Matthieu Baerts
Headers show
Series [net-next] Squash-to: "mptcp: refactor shutdown and close" | expand

Commit Message

Paolo Abeni Oct. 15, 2020, 9:28 p.m. UTC 
After the refactor __mptcp_close_ssk() can be invoked on
orphaned TCP socket, so we can't wait a timeout, or
sk_stream_wait_close() will oops due to NULL waitqueue.

We can simply always use a 0 timeout (and clean-up the
related signature accordingly): the timeout, if any,
is used before by mptcp_close().

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
This should fix issue 100, but I only build test it ;)
---
 net/mptcp/pm_netlink.c |  6 ++----
 net/mptcp/protocol.c   | 19 +++++++++----------
 net/mptcp/protocol.h   |  3 +--
 3 files changed, 12 insertions(+), 16 deletions(-)

Comments

Mat Martineau Oct. 15, 2020, 11:54 p.m. UTC | #1 
On Thu, 15 Oct 2020, Paolo Abeni wrote:

> After the refactor __mptcp_close_ssk() can be invoked on
> orphaned TCP socket, so we can't wait a timeout, or
> sk_stream_wait_close() will oops due to NULL waitqueue.
>
> We can simply always use a 0 timeout (and clean-up the
> related signature accordingly): the timeout, if any,
> is used before by mptcp_close().
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> This should fix issue 100, but I only build test it ;)
> ---
> net/mptcp/pm_netlink.c |  6 ++----
> net/mptcp/protocol.c   | 19 +++++++++----------
> net/mptcp/protocol.h   |  3 +--
> 3 files changed, 12 insertions(+), 16 deletions(-)

Looks good to me from a code review perspective. I haven't confirmed that 
it fixes #100.


--
Mat Martineau
Intel
Christoph Paasch Oct. 16, 2020, 8:24 p.m. UTC | #2 
On 10/15/20 - 23:28, Paolo Abeni wrote:
> After the refactor __mptcp_close_ssk() can be invoked on
> orphaned TCP socket, so we can't wait a timeout, or
> sk_stream_wait_close() will oops due to NULL waitqueue.
> 
> We can simply always use a 0 timeout (and clean-up the
> related signature accordingly): the timeout, if any,
> is used before by mptcp_close().
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> This should fix issue 100, but I only build test it ;)

Yes, it does! :-)


Christoph
Matthieu Baerts Oct. 17, 2020, 7:56 a.m. UTC | #3 
Hi Paolo, Mat, Christoph,

On 15/10/2020 23:28, Paolo Abeni wrote:
> After the refactor __mptcp_close_ssk() can be invoked on
> orphaned TCP socket, so we can't wait a timeout, or
> sk_stream_wait_close() will oops due to NULL waitqueue.
> 
> We can simply always use a 0 timeout (and clean-up the
> related signature accordingly): the timeout, if any,
> is used before by mptcp_close().

Thank you for the patch, review and validation!

- e3514ae223bd: "squashed" (with conflicts) in "mptcp: refactor shutdown 
and close"
- a563fd15f543: conflict in 
t/mptcp-move-page-frag-allocation-in-mptcp_sendmsg
- Results: b160e00bdf28..a26726f4d29b

Tests + export are going to be started soon!

Cheers,
Matt
diff mbox series

Patch

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index ed60538df7b2..7599b0f14920 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -417,14 +417,13 @@  void mptcp_pm_nl_rm_addr_received(struct mptcp_sock *msk)
 	list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 		int how = RCV_SHUTDOWN | SEND_SHUTDOWN;
-		long timeout = 0;
 
 		if (msk->pm.rm_id != subflow->remote_id)
 			continue;
 
 		spin_unlock_bh(&msk->pm.lock);
 		mptcp_subflow_shutdown(sk, ssk, how);
-		__mptcp_close_ssk(sk, ssk, subflow, timeout);
+		__mptcp_close_ssk(sk, ssk, subflow);
 		spin_lock_bh(&msk->pm.lock);
 
 		msk->pm.add_addr_accepted--;
@@ -453,14 +452,13 @@  void mptcp_pm_nl_rm_subflow_received(struct mptcp_sock *msk, u8 rm_id)
 	list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 		int how = RCV_SHUTDOWN | SEND_SHUTDOWN;
-		long timeout = 0;
 
 		if (rm_id != subflow->local_id)
 			continue;
 
 		spin_unlock_bh(&msk->pm.lock);
 		mptcp_subflow_shutdown(sk, ssk, how);
-		__mptcp_close_ssk(sk, ssk, subflow, timeout);
+		__mptcp_close_ssk(sk, ssk, subflow);
 		spin_lock_bh(&msk->pm.lock);
 
 		msk->pm.local_addr_used--;
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0f3dae1dabc9..a6205a940977 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -42,7 +42,7 @@  struct mptcp_skb_cb {
 
 static struct percpu_counter mptcp_sockets_allocated;
 
-static void __mptcp_destroy_sock(struct sock *sk, int timeout);
+static void __mptcp_destroy_sock(struct sock *sk);
 
 /* If msk has an initial subflow socket, and the MP_CAPABLE handshake has not
  * completed yet or has failed, return the subflow socket.
@@ -1711,8 +1711,7 @@  static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk)
  * parent socket.
  */
 void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
-		       struct mptcp_subflow_context *subflow,
-		       long timeout)
+		       struct mptcp_subflow_context *subflow)
 {
 	struct socket *sock = READ_ONCE(ssk->sk_socket);
 
@@ -1733,7 +1732,7 @@  void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	} else {
 		/* otherwise ask tcp do dispose of ssk and subflow ctx */
 		subflow->disposable = 1;
-		__tcp_close(ssk, timeout);
+		__tcp_close(ssk, 0);
 
 		/* close acquired an extra ref */
 		__sock_put(ssk);
@@ -1784,7 +1783,7 @@  static void __mptcp_close_subflow(struct mptcp_sock *msk)
 		if (inet_sk_state_load(ssk) != TCP_CLOSE)
 			continue;
 
-		__mptcp_close_ssk((struct sock *)msk, ssk, subflow, 0);
+		__mptcp_close_ssk((struct sock *)msk, ssk, subflow);
 	}
 }
 
@@ -1850,7 +1849,7 @@  static void mptcp_worker(struct work_struct *work)
 	    (mptcp_check_close_timeout(sk) ||
 	    (state != sk->sk_state && sk->sk_state == TCP_CLOSE))) {
 		inet_sk_state_store(sk, TCP_CLOSE);
-		__mptcp_destroy_sock(sk, 0);
+		__mptcp_destroy_sock(sk);
 		goto unlock;
 	}
 
@@ -2077,13 +2076,13 @@  static void __mptcp_wr_shutdown(struct sock *sk)
 	__mptcp_check_send_data_fin(sk);
 }
 
-static void __mptcp_destroy_sock(struct sock *sk, int timeout)
+static void __mptcp_destroy_sock(struct sock *sk)
 {
 	struct mptcp_subflow_context *subflow, *tmp;
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	LIST_HEAD(conn_list);
 
-	pr_debug("msk=%p timeout=%d", msk, timeout);
+	pr_debug("msk=%p", msk);
 
 	/* be sure to always acquire the join list lock, to sync vs
 	 * mptcp_finish_join().
@@ -2099,7 +2098,7 @@  static void __mptcp_destroy_sock(struct sock *sk, int timeout)
 
 	list_for_each_entry_safe(subflow, tmp, &conn_list, node) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
-		__mptcp_close_ssk(sk, ssk, subflow, timeout);
+		__mptcp_close_ssk(sk, ssk, subflow);
 	}
 
 	sk->sk_prot->destroy(sk);
@@ -2151,7 +2150,7 @@  static void mptcp_close(struct sock *sk, long timeout)
 	sock_hold(sk);
 	pr_debug("msk=%p state=%d", sk, sk->sk_state);
 	if (sk->sk_state == TCP_CLOSE) {
-		__mptcp_destroy_sock(sk, timeout);
+		__mptcp_destroy_sock(sk);
 		do_cancel_work = true;
 	} else {
 		sk_reset_timer(sk, &sk->sk_timer, jiffies + TCP_TIMEWAIT_LEN);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 408c212e8e1c..4a434689c63c 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -412,8 +412,7 @@  void __init mptcp_subflow_init(void);
 void mptcp_subflow_reset(struct sock *ssk);
 void mptcp_subflow_shutdown(struct sock *sk, struct sock *ssk, int how);
 void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
-		       struct mptcp_subflow_context *subflow,
-		       long timeout);
+		       struct mptcp_subflow_context *subflow);
 
 /* called with sk socket lock held */
 int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,