From: Mat Martineau <hidden> Date: 2021-03-04 21:35:01
These patches from the MPTCP tree fix a few multipath TCP issues:
Patches 1 and 5 clear some stale pointers when subflows close.
Patches 2, 4, and 9 plug some memory leaks.
Patch 3 fixes a memory accounting error identified by syzkaller.
Patches 6 and 7 fix a race condition that slowed data transmission.
Patch 8 adds missing wakeups when write buffer space is freed.
Florian Westphal (4):
mptcp: reset last_snd on subflow close
mptcp: put subflow sock on connect error
mptcp: dispose initial struct socket when its subflow is closed
mptcp: reset 'first' and ack_hint on subflow close
Geliang Tang (1):
mptcp: free resources when the port number is mismatched
Paolo Abeni (4):
mptcp: fix memory accounting on allocation error
mptcp: factor out __mptcp_retrans helper()
mptcp: fix race in release_cb
mptcp: fix missing wakeup
net/mptcp/protocol.c | 165 +++++++++++++++++++++++++++----------------
net/mptcp/subflow.c | 14 ++--
2 files changed, 112 insertions(+), 67 deletions(-)
base-commit: a9ecb0cbf03746b17a7c13bd8e3464e6789f73e8
--
2.30.1
From: Mat Martineau <hidden> Date: 2021-03-04 21:35:01
From: Florian Westphal <fw@strlen.de>
Send logic caches last active subflow in the msk, so it needs to be
cleared when the cached subflow is closed.
Fixes: d5f49190def61c ("mptcp: allow picking different xmit subflows")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/155
Reported-by: Christoph Paasch <redacted>
Acked-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Matthieu Baerts <redacted>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Mat Martineau <redacted>
---
net/mptcp/protocol.c | 5 +++++
1 file changed, 5 insertions(+)
From: Mat Martineau <hidden> Date: 2021-03-04 21:35:01
From: Paolo Abeni <pabeni@redhat.com>
Will simplify the following patch, no functional change
intended.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Mat Martineau <redacted>
---
net/mptcp/protocol.c | 93 ++++++++++++++++++++++++--------------------
1 file changed, 50 insertions(+), 43 deletions(-)
@@ -2261,59 +2261,23 @@ static void mptcp_check_fastclose(struct mptcp_sock *msk)mptcp_close_wake_up(sk);}-staticvoidmptcp_worker(structwork_struct*work)+staticvoid__mptcp_retrans(structsock*sk){-structmptcp_sock*msk=container_of(work,structmptcp_sock,work);-structsock*ssk,*sk=&msk->sk.icsk_inet.sk;+structmptcp_sock*msk=mptcp_sk(sk);structmptcp_sendmsg_infoinfo={};structmptcp_data_frag*dfrag;size_tcopied=0;-intstate,ret;--lock_sock(sk);-state=sk->sk_state;-if(unlikely(state==TCP_CLOSE))-gotounlock;--mptcp_check_data_fin_ack(sk);-__mptcp_flush_join_list(msk);--mptcp_check_fastclose(msk);--if(msk->pm.status)-mptcp_pm_nl_work(msk);--if(test_and_clear_bit(MPTCP_WORK_EOF,&msk->flags))-mptcp_check_for_eof(msk);--__mptcp_check_send_data_fin(sk);-mptcp_check_data_fin(sk);--/* There is no point in keeping around an orphaned sk timedout or-*closed,butweneedthemskaroundtoreplytoincomingDATA_FIN,-*evenifitisorphanedandinFIN_WAIT2state-*/-if(sock_flag(sk,SOCK_DEAD)&&-(mptcp_check_close_timeout(sk)||sk->sk_state==TCP_CLOSE)){-inet_sk_state_store(sk,TCP_CLOSE);-__mptcp_destroy_sock(sk);-gotounlock;-}--if(test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW,&msk->flags))-__mptcp_close_subflow(msk);--if(!test_and_clear_bit(MPTCP_WORK_RTX,&msk->flags))-gotounlock;+structsock*ssk;+intret;__mptcp_clean_una(sk);dfrag=mptcp_rtx_head(sk);if(!dfrag)-gotounlock;+return;ssk=mptcp_subflow_get_retrans(msk);if(!ssk)-gotoreset_unlock;+gotoreset_timer;lock_sock(ssk);
@@ -2339,9 +2303,52 @@ static void mptcp_worker(struct work_struct *work)mptcp_set_timeout(sk,ssk);release_sock(ssk);-reset_unlock:+reset_timer:if(!mptcp_timer_pending(sk))mptcp_reset_timer(sk);+}++staticvoidmptcp_worker(structwork_struct*work)+{+structmptcp_sock*msk=container_of(work,structmptcp_sock,work);+structsock*sk=&msk->sk.icsk_inet.sk;+intstate;++lock_sock(sk);+state=sk->sk_state;+if(unlikely(state==TCP_CLOSE))+gotounlock;++mptcp_check_data_fin_ack(sk);+__mptcp_flush_join_list(msk);++mptcp_check_fastclose(msk);++if(msk->pm.status)+mptcp_pm_nl_work(msk);++if(test_and_clear_bit(MPTCP_WORK_EOF,&msk->flags))+mptcp_check_for_eof(msk);++__mptcp_check_send_data_fin(sk);+mptcp_check_data_fin(sk);++/* There is no point in keeping around an orphaned sk timedout or+*closed,butweneedthemskaroundtoreplytoincomingDATA_FIN,+*evenifitisorphanedandinFIN_WAIT2state+*/+if(sock_flag(sk,SOCK_DEAD)&&+(mptcp_check_close_timeout(sk)||sk->sk_state==TCP_CLOSE)){+inet_sk_state_store(sk,TCP_CLOSE);+__mptcp_destroy_sock(sk);+gotounlock;+}++if(test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW,&msk->flags))+__mptcp_close_subflow(msk);++if(test_and_clear_bit(MPTCP_WORK_RTX,&msk->flags))+__mptcp_retrans(sk);unlock:release_sock(sk);
From: Mat Martineau <hidden> Date: 2021-03-04 21:36:05
From: Florian Westphal <fw@strlen.de>
mptcp_add_pending_subflow() performs a sock_hold() on the subflow,
then adds the subflow to the join list.
Without a sock_put the subflow sk won't be freed in case connect() fails.
unreferenced object 0xffff88810c03b100 (size 3000):
[..]
sk_prot_alloc.isra.0+0x2f/0x110
sk_alloc+0x5d/0xc20
inet6_create+0x2b7/0xd30
__sock_create+0x17f/0x410
mptcp_subflow_create_socket+0xff/0x9c0
__mptcp_subflow_connect+0x1da/0xaf0
mptcp_pm_nl_work+0x6e0/0x1120
mptcp_worker+0x508/0x9a0
Fixes: 5b950ff4331ddda ("mptcp: link MPC subflow into msk only after accept")
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Mat Martineau <redacted>
---
net/mptcp/subflow.c | 1 +
1 file changed, 1 insertion(+)
From: Mat Martineau <hidden> Date: 2021-03-04 21:36:05
From: Florian Westphal <fw@strlen.de>
Just like with last_snd, we have to NULL 'first' on subflow close.
ack_hint isn't strictly required (its never dereferenced), but better to
clear this explicitly as well instead of making it an exception.
msk->first is dereferenced unconditionally at accept time, but
at that point the ssk is not on the conn_list yet -- this means
worker can't see it when iterating the conn_list.
Reported-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Matthieu Baerts <redacted>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Mat Martineau <redacted>
---
net/mptcp/protocol.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -3297,6 +3303,9 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,/* PM/worker can now acquire the first subflow socket*lockwithoutracingwithlistenerqueuecleanup,*wecannotifyit,ifneeded.+*+*Evenifremotehasresettheinitialsubflowbynow+*therefcntisstillatleastone.*/subflow=mptcp_subflow_ctx(msk->first);list_add(&subflow->node,&msk->conn_list);
@@ -2116,6 +2116,14 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk)returnbackup;}+staticvoidmptcp_dispose_initial_subflow(structmptcp_sock*msk)+{+if(msk->subflow){+iput(SOCK_INODE(msk->subflow));+msk->subflow=NULL;+}+}+/* subflow sockets can be either outgoing (connect) or incoming*(accept).*
@@ -2529,12 +2540,6 @@ static void __mptcp_destroy_sock(struct sock *sk)might_sleep();-/* dispose the ancillatory tcp socket, if any */-if(msk->subflow){-iput(SOCK_INODE(msk->subflow));-msk->subflow=NULL;-}-/* be sure to always acquire the join list lock, to sync vs*mptcp_finish_join().*/
From: Mat Martineau <hidden> Date: 2021-03-04 21:36:37
From: Paolo Abeni <pabeni@redhat.com>
If we receive a MPTCP_PUSH_PENDING even from a subflow when
mptcp_release_cb() is serving the previous one, the latter
will be delayed up to the next release_sock(msk).
Address the issue implementing a test/serve loop for such
event.
Additionally rename the push helper to __mptcp_push_pending()
to be more consistent with the existing code.
Fixes: 6e628cd3a8f7 ("mptcp: use mptcp release_cb for delayed tasks")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Mat Martineau <redacted>
---
net/mptcp/protocol.c | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
@@ -2959,13 +2959,14 @@ static void mptcp_release_cb(struct sock *sk){unsignedlongflags,nflags;-/* push_pending may touch wmem_reserved, do it before the later-*cleanup-*/-if(test_and_clear_bit(MPTCP_CLEAN_UNA,&mptcp_sk(sk)->flags))-__mptcp_clean_una(sk);-if(test_and_clear_bit(MPTCP_PUSH_PENDING,&mptcp_sk(sk)->flags)){-/* mptcp_push_pending() acquires the subflow socket lock+for(;;){+flags=0;+if(test_and_clear_bit(MPTCP_PUSH_PENDING,&mptcp_sk(sk)->flags))+flags|=MPTCP_PUSH_PENDING;+if(!flags)+break;++/* the following actions acquire the subflow socket lock**1)can'tbeinvokedinatomicscope*2)mustavoidABBAdeadlockwithmsksocketspinlock:theRX
@@ -2974,13 +2975,21 @@ static void mptcp_release_cb(struct sock *sk)*/spin_unlock_bh(&sk->sk_lock.slock);-mptcp_push_pending(sk,0);+if(flags&MPTCP_PUSH_PENDING)+__mptcp_push_pending(sk,0);++cond_resched();spin_lock_bh(&sk->sk_lock.slock);}++if(test_and_clear_bit(MPTCP_CLEAN_UNA,&mptcp_sk(sk)->flags))+__mptcp_clean_una(sk);if(test_and_clear_bit(MPTCP_ERROR_REPORT,&mptcp_sk(sk)->flags))__mptcp_error_report(sk);-/* clear any wmem reservation and errors */+/* push_pending may touch wmem_reserved, ensure we do the cleanup+*later+*/__mptcp_update_wmem(sk);__mptcp_update_rmem(sk);
From: Mat Martineau <hidden> Date: 2021-03-04 21:37:41
From: Paolo Abeni <pabeni@redhat.com>
__mptcp_clean_una() can free write memory and should wake-up
user-space processes when needed.
When such function is invoked by the MPTCP receive path, the wakeup
is not needed, as the TCP stack will later trigger subflow_write_space
which will do the wakeup as needed.
Other __mptcp_clean_una() call sites need an additional wakeup check
Let's bundle the relevant code in a new helper and use it.
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/165
Fixes: 6e628cd3a8f7 ("mptcp: use mptcp release_cb for delayed tasks")
Fixes: 64b9cea7a0af ("mptcp: fix spurious retransmissions")
Tested-by: Matthieu Baerts <redacted>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Mat Martineau <redacted>
---
net/mptcp/protocol.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
From: Mat Martineau <hidden> Date: 2021-03-04 21:37:41
From: Geliang Tang <redacted>
When the port number is mismatched with the announced ones, use
'goto dispose_child' to free the resources instead of using 'goto out'.
This patch also moves the port number checking code in
subflow_syn_recv_sock before mptcp_finish_join, otherwise subflow_drop_ctx
will fail in dispose_child.
Fixes: 5bc56388c74f ("mptcp: add port number check for MP_JOIN")
Reported-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <redacted>
Signed-off-by: Mat Martineau <redacted>
---
net/mptcp/subflow.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
Hello:
This series was applied to netdev/net.git (refs/heads/master):
On Thu, 4 Mar 2021 13:32:07 -0800 you wrote:
These patches from the MPTCP tree fix a few multipath TCP issues:
Patches 1 and 5 clear some stale pointers when subflows close.
Patches 2, 4, and 9 plug some memory leaks.
[...]