netdev - Re: [PATCH net-next v2] mptcp: add reset reason options in some places

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <25b2e5fd1d63fc21c6f52fe6adebdbd63f9907fb.camel@redhat.com>
Date: Mon, 08 Apr 2024 13:47:28 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jason Xing <kerneljasonxing@...il.com>, davem@...emloft.net, 
 edumazet@...gle.com, kuba@...nel.org, matttbe@...nel.org,
 martineau@...nel.org,  geliang@...nel.org
Cc: mptcp@...ts.linux.dev, netdev@...r.kernel.org, Jason Xing
	 <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v2] mptcp: add reset reason options in some
 places

On Sat, 2024-04-06 at 09:48 +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@...cent.com>
> 
> The reason codes are handled in two ways nowadays (quoting Mat Martineau):
> 1. Sending in the MPTCP option on RST packets when there is no subflow
> context available (these use subflow_add_reset_reason() and directly call
> a TCP-level send_reset function)
> 2. The "normal" way via subflow->reset_reason. This will propagate to both
> the outgoing reset packet and to a local path manager process via netlink
> in mptcp_event_sub_closed()
> 
> RFC 8684 defines the skb reset reason behaviour which is not required
> even though in some places:
> 
>     A host sends a TCP RST in order to close a subflow or reject
>     an attempt to open a subflow (MP_JOIN). In order to let the
>     receiving host know why a subflow is being closed or rejected,
>     the TCP RST packet MAY include the MP_TCPRST option (Figure 15).
>     The host MAY use this information to decide, for example, whether
>     it tries to re-establish the subflow immediately, later, or never.
> 
> Since the commit dc87efdb1a5cd ("mptcp: add mptcp reset option support")
> introduced this feature about three years ago, we can fully use it.
> There remains some places where we could insert reason into skb as
> we can see in this patch.
> 
> Many thanks to Mat and Paolo for help:)
> 
> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> ---
> v2
> Link: https://lore.kernel.org/all/5046e1867c65f39e07822beb0a19a22743b1064b.camel@redhat.com/
> 1. complete all the possible reset reasons in the subflow_check_req() (Paolo)
> ---
>  net/mptcp/subflow.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 1626dd20c68f..b7ce2ca1782c 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -150,8 +150,10 @@ static int subflow_check_req(struct request_sock *req,
>  	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
>  	 * TCP option space.
>  	 */
> -	if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info))
> +	if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info)) {
> +		subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
>  		return -EINVAL;
> +	}
>  #endif
>  
>  	mptcp_get_options(skb, &mp_opt);
> @@ -219,6 +221,7 @@ static int subflow_check_req(struct request_sock *req,
>  				 ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
>  			if (!mptcp_pm_sport_in_anno_list(subflow_req->msk, sk_listener)) {
>  				SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTSYNRX);
> +				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
>  				return -EPERM;
>  			}
>  			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINPORTSYNRX);
> @@ -227,10 +230,12 @@ static int subflow_check_req(struct request_sock *req,
>  		subflow_req_create_thmac(subflow_req);
>  
>  		if (unlikely(req->syncookie)) {
> -			if (mptcp_can_accept_new_subflow(subflow_req->msk))
> -				subflow_init_req_cookie_join_save(subflow_req, skb);
> -			else
> +			if (!mptcp_can_accept_new_subflow(subflow_req->msk)) {
> +				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
>  				return -EPERM;
> +			}
> +
> +			subflow_init_req_cookie_join_save(subflow_req, skb);
>  		}
>  
>  		pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token,
> @@ -873,13 +878,18 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>  					 ntohs(inet_sk((struct sock *)owner)->inet_sport));
>  				if (!mptcp_pm_sport_in_anno_list(owner, sk)) {
>  					SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTACKRX);
> +					subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
>  					goto dispose_child;
>  				}
>  				SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINPORTACKRX);
>  			}
>  
> -			if (!mptcp_finish_join(child))
> +			if (!mptcp_finish_join(child)) {
> +				struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(child);
> +
> +				subflow_add_reset_reason(skb, subflow->reset_reason);
>  				goto dispose_child;
> +			}
>  
>  			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX);
>  			tcp_rsk(req)->drop_req = true;

LGTM,

Acked-by: Paolo Abeni <pabeni@...hat.com>