[PATCH v1 net-next 0/2] tcp: Let non-PSP and PSP share mtu and advmss.

[PATCH v1 net-next 0/2] tcp: Let non-PSP and PSP share mtu and advmss.

[Kuniyuki Iwashima]

Since psp_sk_overhead() is added to icsk->icsk_ext_hdr_len,
which is subtracted after clamping in __tcp_mtu_to_mss(),
it is impossible to configure a route that both PSP and
non-PSP achieve 4K MSS for TCP_ZEROCOPY_RECEIVE.

  mtu 4220 advmss 4108
  -> non-PSP gets 4K MSS, but PSP gets 4056, breaking
     TCP_ZEROCOPY_RECEIVE

  mtu 4220 advmss 4148
  -> PSP gets 4K MSS, but non-PSP gets 4136, spilling over
     4K and also breaking TCP_ZEROCOPY_RECEIVE

This series subtracts the PSP overhead before clamping to
let both flows share the same mtu and advmss to get 4K MSS.

Google's original PSP implementation has been doing that for
years to allow both flows to coexist on the same machine.


Kuniyuki Iwashima (2):
  psp: Check pas->tx.spi in psp_sk_overhead().
  tcp: Subtract psp_sk_overhead() in __tcp_mtu_to_mss() and
    tcp_mss_to_mtu().

 include/net/psp/functions.h |  4 ++--
 net/ipv4/tcp_output.c       | 14 +++++++++-----
 net/ipv6/ipv6_sockglue.c    |  2 --
 net/psp/psp_sock.c          |  2 +-
 4 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.56.0.385.gac671c819a-goog

[PATCH v1 net-next 1/2] psp: Check pas->tx.spi in psp_sk_overhead().

[Kuniyuki Iwashima]

The next patch moves psp_sk_overhead(sk) from icsk->icsk_ext_hdr_len
to __tcp_mtu_to_mss() and tcp_mss_to_mtu(), which can be called
between psp_sock_assoc_set_rx() and psp_sock_set_tx_key() while Tx
packets are still sent in cleartext.

Currently, psp_sk_overhead() only checks if sk->psp_assoc is set,
and even without the next patch, if ipv6_update_options() is called
between psp_sock_assoc_set_rx() and psp_sock_set_tx_key(),
psp_sk_overhead(sk) is added to icsk->icsk_ext_hdr_len twice.

Let's check pas->tx.spi in psp_sk_overhead().

Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
not worth backporting, no one does that, only theoretical issue.
---
 include/net/psp/functions.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/net/psp/functions.h b/include/net/psp/functions.h
index b23c30898389..410adb6718c1 100644
--- a/include/net/psp/functions.h
+++ b/include/net/psp/functions.h
@@ -145,9 +145,9 @@ static inline struct psp_assoc *psp_skb_get_assoc_rcu(struct sk_buff *skb)
 static inline unsigned int psp_sk_overhead(const struct sock *sk)
 {
 	int psp_encap = sizeof(struct udphdr) + PSP_HDR_SIZE + PSP_TRL_SIZE;
-	bool has_psp = rcu_access_pointer(sk->psp_assoc);
+	struct psp_assoc *pas = psp_sk_assoc(sk);
 
-	return has_psp ? psp_encap : 0;
+	return pas && pas->tx.spi ? psp_encap : 0;
 }
 #else
 static inline void psp_sk_assoc_free(struct sock *sk) { }
-- 
2.56.0.385.gac671c819a-goog

[PATCH v1 net-next 2/2] tcp: Subtract psp_sk_overhead() in __tcp_mtu_to_mss() and tcp_mss_to_mtu().

[Kuniyuki Iwashima]

Currently, psp_sk_overhead() is added to icsk->icsk_ext_hdr_len.

__tcp_mtu_to_mss() subtracts icsk->icsk_ext_hdr_len from mss
after clamping to tp->rx_opt.mss_clamp, which comes from the
peer's MSS option configured via advmss.

Since PSP is upgraded after TCP_ESTABLISHED, PSP and non-PSP
flows share the same route mtu and advmss.

This makes it impossible to configure a route so that both PSP
and non-PSP flows achieve 4K MSS for TCP_ZEROCOPY_RECEIVE.

With mtu 4220 and advmss 4108 (4096 + 12 TS), non-PSP flows
get 4K MSS, but PSP flows are clamped to 4108 first and then
reduced by 40 bytes to 4056, breaking TCP_ZEROCOPY_RECEIVE.

With mtu 4220 and advmss 4148 to compensate for PSP, PSP flows
get 4K MSS, but non-PSP flows get 4136 bytes, spilling over
4K page and also breaking TCP_ZEROCOPY_RECEIVE.

Let's subtract psp_sk_overhead(sk) before clamping in
__tcp_mtu_to_mss() and add it in tcp_mss_to_mtu() and
to icsk->icsk_mtup.search_high.

Note that tp->rx_opt.mss_clamp now bounds only the inner TCP
segment rather than the outer PSP packet, which is consistent
with xfrm and assumes that the peer's MTU is at least as large
as the local route MTU or that PMTUD is enabled.

Also, in psp_sock_set_tx_key(), icsk->icsk_mtup.search_high is
simply incremented by psp_sk_overhead(sk) because PSP is upgraded
right after TCP_ESTABLISHED before any MTU probe fails in practice.

Reported-by: Thaison Phan <thaisonphan@google.com>
Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
 net/ipv4/tcp_output.c    | 14 +++++++++-----
 net/ipv6/ipv6_sockglue.c |  2 --
 net/psp/psp_sock.c       |  2 +-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index cf3ddc055d48..08d0d4a639fe 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2060,6 +2060,8 @@ static inline int __tcp_mtu_to_mss(struct sock *sk, int pmtu)
 	 */
 	mss_now = pmtu - icsk->icsk_af_ops->net_header_len - sizeof(struct tcphdr);
 
+	mss_now -= psp_sk_overhead(sk);
+
 	/* Clamp it (mss_clamp does not include tcp options) */
 	if (mss_now > tp->rx_opt.mss_clamp)
 		mss_now = tp->rx_opt.mss_clamp;
@@ -2088,9 +2090,10 @@ int tcp_mss_to_mtu(struct sock *sk, int mss)
 	const struct inet_connection_sock *icsk = inet_csk(sk);
 
 	return mss +
-	      tp->tcp_header_len +
-	      icsk->icsk_ext_hdr_len +
-	      icsk->icsk_af_ops->net_header_len;
+		tp->tcp_header_len +
+		psp_sk_overhead(sk) +
+		icsk->icsk_ext_hdr_len +
+		icsk->icsk_af_ops->net_header_len;
 }
 EXPORT_SYMBOL(tcp_mss_to_mtu);
 
@@ -2103,7 +2106,7 @@ void tcp_mtup_init(struct sock *sk)
 
 	icsk->icsk_mtup.enabled = READ_ONCE(net->ipv4.sysctl_tcp_mtu_probing) > 1;
 	icsk->icsk_mtup.search_high = tp->rx_opt.mss_clamp + sizeof(struct tcphdr) +
-			       icsk->icsk_af_ops->net_header_len;
+		psp_sk_overhead(sk) + icsk->icsk_af_ops->net_header_len;
 	icsk->icsk_mtup.search_low = tcp_mss_to_mtu(sk, READ_ONCE(net->ipv4.sysctl_tcp_base_mss));
 	icsk->icsk_mtup.probe_size = 0;
 	if (icsk->icsk_mtup.enabled)
@@ -2643,7 +2646,8 @@ static inline void tcp_mtu_check_reprobe(struct sock *sk)
 		icsk->icsk_mtup.probe_size = 0;
 		icsk->icsk_mtup.search_high = tp->rx_opt.mss_clamp +
 			sizeof(struct tcphdr) +
-			icsk->icsk_af_ops->net_header_len;
+			icsk->icsk_af_ops->net_header_len +
+			psp_sk_overhead(sk);
 		icsk->icsk_mtup.search_low = tcp_mss_to_mtu(sk, mss);
 
 		/* Update probe time stamp */
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 4b3536571c98..ea03fcffd147 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -48,7 +48,6 @@
 #include <net/xfrm.h>
 #include <net/compat.h>
 #include <net/seg6.h>
-#include <net/psp.h>
 
 #include <linux/uaccess.h>
 
@@ -109,7 +108,6 @@ struct ipv6_txoptions *ipv6_update_options(struct sock *sk,
 			struct inet_connection_sock *icsk = inet_csk(sk);
 
 			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/psp/psp_sock.c b/net/psp/psp_sock.c
index a6b1c42dd626..f2cba3fa3a23 100644
--- a/net/psp/psp_sock.c
+++ b/net/psp/psp_sock.c
@@ -243,7 +243,7 @@ psp_sock_set_tx_key(struct sock *sk, struct psp_dev *psd, struct psp_assoc *pas,
 	pas->upgrade_seq = tcp_sk(sk)->rcv_nxt;
 
 	icsk = inet_csk(sk);
-	icsk->icsk_ext_hdr_len += psp_sk_overhead(sk);
+	icsk->icsk_mtup.search_high += psp_sk_overhead(sk);
 	icsk->icsk_sync_mss(sk, icsk->icsk_pmtu_cookie);
 
 	return err;
-- 
2.56.0.385.gac671c819a-goog