| Message ID | 20240510030435.120935-8-kuba@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add basic PSP encryption for TCP connections | expand |
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@kernel.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 >
On Sun, 12 May 2024 21:47:16 -0400 Willem de Bruijn wrote: > > - 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. Mostly for symmetry, really. IDK what's worse, the dead code or that someone may be surprised it's not there.. Should I delete it?
Jakub Kicinski wrote: > On Sun, 12 May 2024 21:47:16 -0400 Willem de Bruijn wrote: > > > - 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. > > Mostly for symmetry, really. IDK what's worse, the dead code or that > someone may be surprised it's not there.. Should I delete it? Symmetry with what? This dead code had me scratching my head what it was doing, so my vote to drop it. If you want something, maybe a code comment instead?
diff --git a/include/net/psp/functions.h b/include/net/psp/functions.h index ad81322dd4ed..be4f78dec425 100644 --- a/include/net/psp/functions.h +++ b/include/net/psp/functions.h @@ -90,6 +90,13 @@ static inline struct psp_assoc *psp_skb_get_assoc_rcu(struct sk_buff *skb) return NULL; return rcu_dereference(skb->sk->psp_assoc); } + +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; +} #else static inline void psp_sk_assoc_free(struct sock *sk) { } static inline void @@ -127,6 +134,11 @@ static inline struct psp_assoc *psp_skb_get_assoc_rcu(struct sk_buff *skb) { return NULL; } + +static inline unsigned int psp_sk_overhead(const struct sock *sk) +{ + return 0; +} #endif static inline unsigned long diff --git a/include/net/psp/types.h b/include/net/psp/types.h index e39abf10c03c..aad836c1c2ca 100644 --- a/include/net/psp/types.h +++ b/include/net/psp/types.h @@ -95,6 +95,9 @@ struct psp_dev_caps { #define PSP_V1_KEY 32 #define PSP_MAX_KEY 32 +#define PSP_HDR_SIZE 16 /* We don't support optional fields, yet */ +#define PSP_TRL_SIZE 16 /* AES-GCM/GMAC trailer size */ + struct psp_skb_ext { __be32 spi; /* generation and version are 8b but we don't want holes */ diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 9539c4a7b55d..2a602cf51009 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -279,9 +279,9 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) inet->inet_dport = usin->sin_port; sk_daddr_set(sk, daddr); - inet_csk(sk)->icsk_ext_hdr_len = 0; + inet_csk(sk)->icsk_ext_hdr_len = psp_sk_overhead(sk); if (inet_opt) - inet_csk(sk)->icsk_ext_hdr_len = inet_opt->opt.optlen; + inet_csk(sk)->icsk_ext_hdr_len += inet_opt->opt.optlen; tp->rx_opt.mss_clamp = TCP_MSS_DEFAULT; diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c index d4c28ec1bc51..b4505bbb9e2c 100644 --- a/net/ipv6/ipv6_sockglue.c +++ b/net/ipv6/ipv6_sockglue.c @@ -49,6 +49,7 @@ #include <net/xfrm.h> #include <net/compat.h> #include <net/seg6.h> +#include <net/psp.h> #include <linux/uaccess.h> @@ -107,7 +108,10 @@ struct ipv6_txoptions *ipv6_update_options(struct sock *sk, !((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) && inet_sk(sk)->inet_daddr != LOOPBACK4_IPV6) { struct inet_connection_sock *icsk = inet_csk(sk); - icsk->icsk_ext_hdr_len = opt->opt_flen + opt->opt_nflen; + + icsk->icsk_ext_hdr_len = + psp_sk_overhead(sk) + + opt->opt_flen + opt->opt_nflen; icsk->icsk_sync_mss(sk, icsk->icsk_pmtu_cookie); } } 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); 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:
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@kernel.org> --- include/net/psp/functions.h | 12 ++++++++++++ include/net/psp/types.h | 3 +++ net/ipv4/tcp_ipv4.c | 4 ++-- net/ipv6/ipv6_sockglue.c | 6 +++++- net/ipv6/tcp_ipv6.c | 12 ++++++------ net/psp/psp_sock.c | 5 +++++ 6 files changed, 33 insertions(+), 9 deletions(-)