[MPTCP] Re: [PATCH net 2/2] mptcp: subflows garbage collection

[MPTCP] Re: [PATCH net 2/2] mptcp: subflows garbage collection

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3130 bytes --]


Hi Paolo,

On Wed, 7 Oct 2020, Paolo Abeni wrote:

> The msk can close MP_JOIN subflows if the initial handshake
> fails. Currently such subflows are kept alive in the
> conn_list untill the msk itself is closed.
>
> Beyond the wasted memory, we could end-up sending the
> DATA_FIN and the DATA_FIN ack on such socket, even after a
> reset.
>
> Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> net/mptcp/protocol.c | 18 ++++++++++++++++++
> net/mptcp/protocol.h |  1 +
> net/mptcp/subflow.c  |  5 +++++
> 3 files changed, 24 insertions(+)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 82b324ed3c4c..92e0152f831b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1701,6 +1701,20 @@ static void pm_work(struct mptcp_sock *msk)
> 	spin_unlock_bh(&msk->pm.lock);
> }
>
> +static void __mptcp_close_subflow(struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow, *tmp;
> +
> +	list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +		if (inet_sk_state_load(ssk) != TCP_CLOSE)
> +			continue;
> +
> +		__mptcp_close_ssk((struct sock *)msk, ssk, subflow, 0);
> +	}
> +}
> +
> static void mptcp_worker(struct work_struct *work)
> {
> 	struct mptcp_sock *msk = container_of(work, struct mptcp_sock, work);
> @@ -1718,11 +1732,15 @@ static void mptcp_worker(struct work_struct *work)
> 	mptcp_clean_una(sk);
> 	mptcp_check_data_fin_ack(sk);
> 	__mptcp_flush_join_list(msk);
> +	if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
> +		__mptcp_close_subflow(msk);
> +
> 	__mptcp_move_skbs(msk);
>
> 	if (msk->pm.status)
> 		pm_work(msk);
>
> +

Some whitespace noise here.

> 	if (test_and_clear_bit(MPTCP_WORK_EOF, &msk->flags))
> 		mptcp_check_for_eof(msk);
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index de5219fd3570..3462c00afd9e 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -90,6 +90,7 @@
> #define MPTCP_WORK_RTX		2
> #define MPTCP_WORK_EOF		3
> #define MPTCP_FALLBACK_DONE	4
> +#define MPTCP_WORK_CLOSE_SUBFLOW 5
>
> struct mptcp_options_received {
> 	u64	sndr_key;
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index d2eb5f2abe5a..5ff806dc4b83 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -273,9 +273,14 @@ static bool subflow_thmac_valid(struct mptcp_subflow_context *subflow)
>
> void mptcp_subflow_reset(struct sock *ssk)
> {
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +	struct sock *sk = subflow->conn;
> +
> 	tcp_set_state(ssk, TCP_CLOSE);
> 	tcp_send_active_reset(ssk, GFP_ATOMIC);
> 	tcp_done(ssk);
> +	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags))
> +		mptcp_schedule_work(sk);

There's a build problem here, mptcp_schedule() is introduced later in the 
export branch and isn't on -net or net-next.

Otherwise, the series looks good.

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH net 2/2] mptcp: subflows garbage collection

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

On Thu, 8 Oct 2020, Paolo Abeni wrote:

> The msk can close MP_JOIN subflows if the initial handshake
> fails. Currently such subflows are kept alive in the
> conn_list untill the msk itself is closed.
>
> Beyond the wasted memory, we could end-up sending the
> DATA_FIN and the DATA_FIN ack on such socket, even after a
> reset.
>
> Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> net/mptcp/protocol.c | 18 ++++++++++++++++++
> net/mptcp/protocol.h |  1 +
> net/mptcp/subflow.c  |  6 ++++++
> 3 files changed, 25 insertions(+)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 82b324ed3c4c..92e0152f831b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1701,6 +1701,20 @@ static void pm_work(struct mptcp_sock *msk)
> 	spin_unlock_bh(&msk->pm.lock);
> }
>
> +static void __mptcp_close_subflow(struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow, *tmp;
> +
> +	list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +		if (inet_sk_state_load(ssk) != TCP_CLOSE)
> +			continue;
> +
> +		__mptcp_close_ssk((struct sock *)msk, ssk, subflow, 0);
> +	}
> +}
> +
> static void mptcp_worker(struct work_struct *work)
> {
> 	struct mptcp_sock *msk = container_of(work, struct mptcp_sock, work);
> @@ -1718,11 +1732,15 @@ static void mptcp_worker(struct work_struct *work)
> 	mptcp_clean_una(sk);
> 	mptcp_check_data_fin_ack(sk);
> 	__mptcp_flush_join_list(msk);
> +	if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
> +		__mptcp_close_subflow(msk);
> +
> 	__mptcp_move_skbs(msk);
>
> 	if (msk->pm.status)
> 		pm_work(msk);
>
> +
> 	if (test_and_clear_bit(MPTCP_WORK_EOF, &msk->flags))
> 		mptcp_check_for_eof(msk);

Still this extra whitespace, but looks good other than that:

Reviewed-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>


I applied both (resolving conflict on patch 1) to net/master and the self 
tests look ok on my system. diag.sh has some failures, but those are the 
same with or without these patches.

--
Mat Martineau
Intel