Re: [PATCH mptcp-next v4 2/2] mptcp: trace MP_FAIL subflow in mptcp_sock

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Thu, 9 Jun 2022, Geliang Tang wrote:

> This patch adds fail_ssk struct member in struct mptcp_sock to record
> the MP_FAIL subsocket. It can replace the mp_fail_response_expect flag
> in struct mptcp_subflow_context.
>
> Drop mp_fail_response_expect_subflow() helper too, just use this fail_ssk
> in mptcp_mp_fail_no_response() to reset the subflow.
>
> Acked-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/pm.c       |  2 +-
> net/mptcp/protocol.c | 35 +++++++++--------------------------
> net/mptcp/protocol.h |  2 +-
> net/mptcp/subflow.c  |  2 +-
> 4 files changed, 12 insertions(+), 29 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 45a9e02abf24..2a57d95d5492 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -305,7 +305,7 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
> 	if (!READ_ONCE(msk->allow_infinite_fallback))
> 		return;
>
> -	if (!READ_ONCE(subflow->mp_fail_response_expect)) {
> +	if (!msk->fail_ssk) {
> 		pr_debug("send MP_FAIL response and infinite map");
>
> 		subflow->send_mp_fail = 1;
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 917df5fb9708..58427fabb061 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2167,21 +2167,6 @@ static void mptcp_retransmit_timer(struct timer_list *t)
> 	sock_put(sk);
> }
>
> -static struct mptcp_subflow_context *
> -mp_fail_response_expect_subflow(struct mptcp_sock *msk)
> -{
> -	struct mptcp_subflow_context *subflow, *ret = NULL;
> -
> -	mptcp_for_each_subflow(msk, subflow) {
> -		if (READ_ONCE(subflow->mp_fail_response_expect)) {
> -			ret = subflow;
> -			break;
> -		}
> -	}
> -
> -	return ret;
> -}
> -
> static void mptcp_timeout_timer(struct timer_list *t)
> {
> 	struct sock *sk = from_timer(sk, t, sk_timer);
> @@ -2507,19 +2492,16 @@ static void __mptcp_retrans(struct sock *sk)
>
> static void mptcp_mp_fail_no_response(struct mptcp_sock *msk)
> {
> -	struct mptcp_subflow_context *subflow;
> -	struct sock *ssk;
> +	struct sock *ssk = msk->fail_ssk;
> 	bool slow;
>
> -	subflow = mp_fail_response_expect_subflow(msk);
> -	if (subflow) {
> -		pr_debug("MP_FAIL doesn't respond, reset the subflow");
> +	pr_debug("MP_FAIL doesn't respond, reset the subflow");
>
> -		ssk = mptcp_subflow_tcp_sock(subflow);
> -		slow = lock_sock_fast(ssk);
> -		mptcp_subflow_reset(ssk);
> -		unlock_sock_fast(ssk, slow);
> -	}
> +	slow = lock_sock_fast(ssk);
> +	mptcp_subflow_reset(ssk);
> +	unlock_sock_fast(ssk, slow);
> +
> +	msk->fail_ssk = NULL;
> }
>
> static void mptcp_worker(struct work_struct *work)
> @@ -2562,7 +2544,7 @@ static void mptcp_worker(struct work_struct *work)
> 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
> 		__mptcp_retrans(sk);
>
> -	if (time_after(jiffies, msk->fail_tout))

Hi Geliang -

This condition could be unexpectedly true depending on how close 'jiffies' 
is to wrapping around, if msk->fail_tout remains at its default 0 
value...

> +	if (msk->fail_ssk && time_after(jiffies, msk->fail_tout))

...so it's important to fix it like this!

I think the two patches should be re-squashed to avoid introducing the 
issue in the previous commit. Sound good?

Also, I think the change should be for mptcp-net so we can get the fix in 
to 5.19-rcX

Thanks!

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>


> 		mptcp_mp_fail_no_response(msk);
>
> unlock:
> @@ -2590,6 +2572,7 @@ static int __mptcp_init_sock(struct sock *sk)
> 	WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
> 	WRITE_ONCE(msk->allow_infinite_fallback, true);
> 	msk->recovery = false;
> +	msk->fail_ssk = NULL;
> 	msk->fail_tout = 0;
>
> 	mptcp_pm_data_init(msk);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index f7b01275af94..bef7dea9f358 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -306,6 +306,7 @@ struct mptcp_sock {
>
> 	u32 setsockopt_seq;
> 	char		ca_name[TCP_CA_NAME_MAX];
> +	struct sock	*fail_ssk;
> 	unsigned long	fail_tout;
> };
>
> @@ -469,7 +470,6 @@ struct mptcp_subflow_context {
> 		local_id_valid : 1, /* local_id is correctly initialized */
> 		valid_csum_seen : 1;        /* at least one csum validated */
> 	enum mptcp_data_avail data_avail;
> -	bool	mp_fail_response_expect;
> 	bool	scheduled;
> 	u32	remote_nonce;
> 	u64	thmac;
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 866d54a0e83c..5351d54e514a 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1235,7 +1235,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
> 				while ((skb = skb_peek(&ssk->sk_receive_queue)))
> 					sk_eat_skb(ssk, skb);
> 			} else {
> -				WRITE_ONCE(subflow->mp_fail_response_expect, true);
> +				msk->fail_ssk = ssk;
> 				msk->fail_tout = jiffies + TCP_RTO_MAX;
> 			}
> 			WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel