netdev - Re: [RFC net-next 07/15] net: psp: update the TCP MSS to reflect PSP packet overhead

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]

Date: Sun, 12 May 2024 21:47:16 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, 
 netdev@...r.kernel.org
Cc: pabeni@...hat.com, 
 willemdebruijn.kernel@...il.com, 
 borisp@...dia.com, 
 gal@...dia.com, 
 cratiu@...dia.com, 
 rrameshbabu@...dia.com, 
 steffen.klassert@...unet.com, 
 tariqt@...dia.com, 
 Jakub Kicinski <kuba@...nel.org>
Subject: Re: [RFC net-next 07/15] net: psp: update the TCP MSS to reflect PSP
 packet overhead

Jakub Kicinski wrote:
> PSP eats 32B of header space. Adjust MSS appropriately.
> 
> We can either modify tcp_mtu_to_mss() / tcp_mss_to_mtu()
> or reuse icsk_ext_hdr_len. The former option is more TCP
> specific and has runtime overhead. The latter is a bit
> of a hack as PSP is not an ext_hdr. If one squints hard
> enough, UDP encap is just a more practical version of
> IPv6 exthdr, so go with the latter. Happy to change.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

> +static inline unsigned int psp_sk_overhead(const struct sock *sk)
> +{
> +	bool has_psp = rcu_access_pointer(sk->psp_assoc);
> +
> +	return has_psp ? PSP_HDR_SIZE + PSP_TRL_SIZE : 0;
> +}

> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 6991464511c3..c67700fc49a1 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -299,10 +299,10 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
>  	sk->sk_gso_type = SKB_GSO_TCPV6;
>  	ip6_dst_store(sk, dst, NULL, NULL);
>  
> -	icsk->icsk_ext_hdr_len = 0;
> +	icsk->icsk_ext_hdr_len = psp_sk_overhead(sk);
>  	if (opt)
> -		icsk->icsk_ext_hdr_len = opt->opt_flen +
> -					 opt->opt_nflen;
> +		icsk->icsk_ext_hdr_len += opt->opt_flen +
> +					  opt->opt_nflen;
>  
>  	tp->rx_opt.mss_clamp = IPV6_MIN_MTU - sizeof(struct tcphdr) - sizeof(struct ipv6hdr);
>  
> @@ -1500,10 +1500,10 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
>  		opt = ipv6_dup_options(newsk, opt);
>  		RCU_INIT_POINTER(newnp->opt, opt);
>  	}
> -	inet_csk(newsk)->icsk_ext_hdr_len = 0;
> +	inet_csk(newsk)->icsk_ext_hdr_len = psp_sk_overhead(sk);
>  	if (opt)
> -		inet_csk(newsk)->icsk_ext_hdr_len = opt->opt_nflen +
> -						    opt->opt_flen;
> +		inet_csk(newsk)->icsk_ext_hdr_len += opt->opt_nflen +
> +						     opt->opt_flen;
>  
>  	tcp_ca_openreq_child(newsk, dst);

The below code adjusts ext_hdr_len and recalculates mss when
setting the tx association.

Why already include it at connect and syn_recv, above?

My assumption was that the upgrade to PSP only happens during
TCP_ESTABLISHED. But perhaps I'm wrong.

Is it allowed to set rx and tx association even from as early as the
initial socket(), when still in TCP_CLOSE, client-side?

Server-side, there is no connection fd to pass to netlink commands
before TCP_ESTABLISHED.

> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
> index 42b881e681b9..bcef042cb8a5 100644
> --- a/net/psp/psp_sock.c
> +++ b/net/psp/psp_sock.c
> @@ -170,6 +170,7 @@ int psp_sock_assoc_set_tx(struct sock *sk, struct psp_dev *psd,
>  			  u32 version, struct psp_key_parsed *key,
>  			  struct netlink_ext_ack *extack)
>  {
> +	struct inet_connection_sock *icsk;
>  	struct psp_assoc *pas, *dummy;
>  	int err;
>  
> @@ -220,6 +221,10 @@ int psp_sock_assoc_set_tx(struct sock *sk, struct psp_dev *psd,
>  
>  	WRITE_ONCE(sk->sk_validate_xmit_skb, psp_validate_xmit);
>  
> +	icsk = inet_csk(sk);
> +	icsk->icsk_ext_hdr_len += psp_sk_overhead(sk);
> +	icsk->icsk_sync_mss(sk, icsk->icsk_pmtu_cookie);
> +
>  exit_free_dummy:
>  	kfree(dummy);
>  exit_clear_rx:
> -- 
> 2.45.0
>