Re: [PATCH mptcp-net] mptcp: move subflow cleanup in mptcp_destroy_common()

[Paolo Abeni <pabeni@redhat.com>]

On Fri, 2022-07-15 at 12:13 -0700, Mat Martineau wrote:
> On Fri, 15 Jul 2022, Paolo Abeni wrote:
> 
> > If the mptcp socket creation fails due to a CGROUP_INET_SOCK_CREATE
> > eBPF program, the MPTCP protocol ends-up leaking all the subflows:
> > the related cleanup happens in __mptcp_destroy_sock() that is not
> > invoked in such code path.
> > 
> > Address the issue moving the subflow sockets cleanup in the
> > mptcp_destroy_common() helper, which is invoked in every msk cleanup
> > path.
> > 
> > Additionally get rid of the intermediate list_splice_init step, which
> > is an unneeded relic from the past.
> > 
> > The issue is present since before the reported root cause commit, but
> > any attempt to backport the fix before that hash will require a complete
> > rewrite.
> > 
> > Fixes: e16163b6e2 ("mptcp: refactor shutdown and close")
> > Reported-by: Nguyen Dinh Phi <phind.uet@gmail.com>
> > Co-developed-by: Nguyen Dinh Phi <phind.uet@gmail.com>
> > Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > The tags are provisional, waiting for Nguyen feedback/preferences.
> > 
> > Sending out early to trigger the CI.
> 
> For some reason patchew didn't tag this commit, so the CI didn't trigger. 
> Patchew did tag my patch about a half hour ago, so the importer doesn't 
> seem to be completely offline.
> 
> I manually pushed a tag: patchew/20220715-pabeni-subflow-cleanup
> 
> That has started the github and cirrus-ci builds:
> 
> https://github.com/multipath-tcp/mptcp_net-next/actions/runs/2678690859
> https://cirrus-ci.com/build/4905612499746816
> 
> 
> I also noticed that this patch conflicts with Matthieu's "mptcp: add 
> mptcp_for_each_subflow_safe helper" patch, but that's only in the export 
> branch currently so we can easily wait on that until your -net fix 
> propagates to net-next.
> 
> 
> One more question below (about mptcp_dispose_initial_subflow), but 
> assuming the CI results are ok I think this would be good to apply to 
> export-net and squash further fixes if needed:
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> 
> > 
> > This will need likely some staging period.
> > 
> > "mptcp: add mptcp_for_each_subflow_safe helper" needs to be rebased
> > on top of this (we can replace another list_for_each_safe instance)
> > ---
> > net/mptcp/protocol.c | 39 +++++++++++++++------------------------
> > net/mptcp/protocol.h |  2 +-
> > net/mptcp/subflow.c  |  3 ++-
> > 3 files changed, 18 insertions(+), 26 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 21a3ed64226e..6f3324955355 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2804,30 +2804,16 @@ static void __mptcp_wr_shutdown(struct sock *sk)
> > 
> > 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", msk);
> > 
> > 	might_sleep();
> > 
> > -	/* join list will be eventually flushed (with rst) at sock lock release time*/
> > -	list_splice_init(&msk->conn_list, &conn_list);
> > -
> > 	mptcp_stop_timer(sk);
> > 	sk_stop_timer(sk, &sk->sk_timer);
> > 	msk->pm.status = 0;
> > 
> > -	/* clears msk->subflow, allowing the following loop to close
> > -	 * even the initial subflow
> > -	 */
> > -	mptcp_dispose_initial_subflow(msk);
> > -	list_for_each_entry_safe(subflow, tmp, &conn_list, node) {
> > -		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > -		__mptcp_close_ssk(sk, ssk, subflow, 0);
> > -	}
> > -
> > 	sk->sk_prot->destroy(sk);
> > 
> > 	WARN_ON_ONCE(msk->rmem_fwd_alloc);
> > @@ -2919,24 +2905,20 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
> > 
> > static int mptcp_disconnect(struct sock *sk, int flags)
> > {
> > -	struct mptcp_subflow_context *subflow, *tmp;
> > 	struct mptcp_sock *msk = mptcp_sk(sk);
> > 
> > 	inet_sk_state_store(sk, TCP_CLOSE);
> > 
> > -	list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
> > -		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > -
> > -		__mptcp_close_ssk(sk, ssk, subflow, MPTCP_CF_FASTCLOSE);
> > -	}
> > -
> > 	mptcp_stop_timer(sk);
> > 	sk_stop_timer(sk, &sk->sk_timer);
> > 
> > 	if (mptcp_sk(sk)->token)
> > 		mptcp_event(MPTCP_EVENT_CLOSED, mptcp_sk(sk), NULL, GFP_KERNEL);
> > 
> > -	mptcp_destroy_common(msk);
> > +	/* msk->subflow is still intact, the following will not free the first
> > +	 * subflow
> > +	 */
> > +	mptcp_destroy_common(msk, MPTCP_CF_FASTCLOSE);
> > 	msk->last_snd = NULL;
> > 	WRITE_ONCE(msk->flags, 0);
> > 	msk->cb_flags = 0;
> > @@ -3086,12 +3068,17 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
> > 	return newsk;
> > }
> > 
> > -void mptcp_destroy_common(struct mptcp_sock *msk)
> > +void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
> > {
> > +	struct mptcp_subflow_context *subflow, *tmp;
> > 	struct sock *sk = (struct sock *)msk;
> > 
> > 	__mptcp_clear_xmit(sk);
> > 
> > +	/* join list will be eventually flushed (with rst) at sock lock release time */
> > +	list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node)
> > +		__mptcp_close_ssk(sk, mptcp_subflow_tcp_sock(subflow), subflow, flags);
> > +
> > 	/* move to sk_receive_queue, sk_stream_kill_queues will purge it */
> > 	mptcp_data_lock(sk);
> > 	skb_queue_splice_tail_init(&msk->receive_queue, &sk->sk_receive_queue);
> > @@ -3113,7 +3100,11 @@ static void mptcp_destroy(struct sock *sk)
> > {
> > 	struct mptcp_sock *msk = mptcp_sk(sk);
> > 
> > -	mptcp_destroy_common(msk);
> > +	/* clears msk->subflow, allowing the following to close
> > +	 * even the initial subflow
> > +	 */
> > +	mptcp_dispose_initial_subflow(msk);
> 
> Was the missing call to mptcp_displose_initial_subflow() also causing a 
> problem? Or did this cleanup end up happening through a different path?
> 
> This seems different from the "leaking all subflows" case mentioned in the 
> commit message.

I'm unsure I read the question correctly.

This patch does not add a new mptcp_dispose_initial_subflow() call in
the cleanup/free code path. Such call was already there in
__mptcp_destroy_sock(), and is needed to ensure even the initial
subflow is deleted.

mptcp_dispose_initial_subflow() is placed outside
mptcp_destroy_common() to allow mptcp_disconnect() to re-use the latter
helper while preserving the initial subflow 

Please let me know if the above solves your doubts.

Paolo