[PATCH net-next 0/3] tcp: refactor skb_cmp_decrypted() checks

[PATCH net-next 0/3] tcp: refactor skb_cmp_decrypted() checks

[Jakub Kicinski]

Refactor the input patch coalescing checks and wrap "EOR forcing"
logic into a helper. This will hopefully make the code easier to
follow. While at it throw some DEBUG_NET checks into skb_shift().

Jakub Kicinski (3):
  tcp: wrap mptcp and decrypted checks into tcp_skb_can_collapse_rx()
  tcp: add a helper for setting EOR on tail skb
  net: skb: add compatibility warnings to skb_shift()

 include/net/tcp.h    | 16 ++++++++++++++++
 net/core/skbuff.c    |  3 +++
 net/ipv4/tcp_input.c | 11 +++--------
 net/ipv4/tcp_ipv4.c  |  3 +--
 net/tls/tls_device.c | 11 ++---------
 5 files changed, 25 insertions(+), 19 deletions(-)

-- 
2.45.1

[PATCH net-next 1/3] tcp: wrap mptcp and decrypted checks into tcp_skb_can_collapse_rx()

[Jakub Kicinski]

tcp_skb_can_collapse() checks for conditions which don't make
sense on input. Because of this we ended up sprinkling a few
pairs of mptcp_skb_can_collapse() and skb_cmp_decrypted() calls
on the input path. Group them in a new helper. This should make
it less likely that someone will check mptcp and not decrypted
or vice versa when adding new code.

This implicitly adds a decrypted check early in tcp_collapse().
AFAIU this will very slightly increase our ability to collapse
packets under memory pressure, not a real bug.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/tcp.h    |  7 +++++++
 net/ipv4/tcp_input.c | 11 +++--------
 net/ipv4/tcp_ipv4.c  |  3 +--
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 32815a40dea1..32741856da01 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1071,6 +1071,13 @@ static inline bool tcp_skb_can_collapse(const struct sk_buff *to,
 		      skb_pure_zcopy_same(to, from));
 }
 
+static inline bool tcp_skb_can_collapse_rx(const struct sk_buff *to,
+					   const struct sk_buff *from)
+{
+	return likely(mptcp_skb_can_collapse(to, from) &&
+		      !skb_cmp_decrypted(to, from));
+}
+
 /* Events passed to congestion control interface */
 enum tcp_ca_event {
 	CA_EVENT_TX_START,	/* first transmit when no packets in flight */
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 5aadf64e554d..212b6fd0caf7 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4813,10 +4813,7 @@ static bool tcp_try_coalesce(struct sock *sk,
 	if (TCP_SKB_CB(from)->seq != TCP_SKB_CB(to)->end_seq)
 		return false;
 
-	if (!mptcp_skb_can_collapse(to, from))
-		return false;
-
-	if (skb_cmp_decrypted(from, to))
+	if (!tcp_skb_can_collapse_rx(to, from))
 		return false;
 
 	if (!skb_try_coalesce(to, from, fragstolen, &delta))
@@ -5372,7 +5369,7 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
 			break;
 		}
 
-		if (n && n != tail && mptcp_skb_can_collapse(skb, n) &&
+		if (n && n != tail && tcp_skb_can_collapse_rx(skb, n) &&
 		    TCP_SKB_CB(skb)->end_seq != TCP_SKB_CB(n)->seq) {
 			end_of_skbs = false;
 			break;
@@ -5423,11 +5420,9 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
 				skb = tcp_collapse_one(sk, skb, list, root);
 				if (!skb ||
 				    skb == tail ||
-				    !mptcp_skb_can_collapse(nskb, skb) ||
+				    !tcp_skb_can_collapse_rx(nskb, skb) ||
 				    (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)))
 					goto end;
-				if (skb_cmp_decrypted(skb, nskb))
-					goto end;
 			}
 		}
 	}
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 041c7eda9abe..228de0c95a9d 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2049,8 +2049,7 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb,
 	      TCP_SKB_CB(skb)->tcp_flags) & TCPHDR_ACK) ||
 	    ((TCP_SKB_CB(tail)->tcp_flags ^
 	      TCP_SKB_CB(skb)->tcp_flags) & (TCPHDR_ECE | TCPHDR_CWR)) ||
-	    !mptcp_skb_can_collapse(tail, skb) ||
-	    skb_cmp_decrypted(tail, skb) ||
+	    !tcp_skb_can_collapse_rx(tail, skb) ||
 	    thtail->doff != th->doff ||
 	    memcmp(thtail + 1, th + 1, hdrlen - sizeof(*th)))
 		goto no_coalesce;
-- 
2.45.1

Re: [PATCH net-next 1/3] tcp: wrap mptcp and decrypted checks into tcp_skb_can_collapse_rx()

[Eric Dumazet]

On Fri, May 31, 2024 at 1:36 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> tcp_skb_can_collapse() checks for conditions which don't make
> sense on input. Because of this we ended up sprinkling a few
> pairs of mptcp_skb_can_collapse() and skb_cmp_decrypted() calls
> on the input path. Group them in a new helper. This should make
> it less likely that someone will check mptcp and not decrypted
> or vice versa when adding new code.
>
> This implicitly adds a decrypted check early in tcp_collapse().
> AFAIU this will very slightly increase our ability to collapse
> packets under memory pressure, not a real bug.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next 1/3] tcp: wrap mptcp and decrypted checks into tcp_skb_can_collapse_rx()

[Matthieu Baerts]

Hi Jakub,

On 31/05/2024 01:36, Jakub Kicinski wrote:
> tcp_skb_can_collapse() checks for conditions which don't make
> sense on input. Because of this we ended up sprinkling a few
> pairs of mptcp_skb_can_collapse() and skb_cmp_decrypted() calls
> on the input path. Group them in a new helper. This should make
> it less likely that someone will check mptcp and not decrypted
> or vice versa when adding new code.
> 
> This implicitly adds a decrypted check early in tcp_collapse().
> AFAIU this will very slightly increase our ability to collapse
> packets under memory pressure, not a real bug.

Good idea! Thank you for this refactoring, and for having cc'd MPTCP ML:

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH net-next 1/3] tcp: wrap mptcp and decrypted checks into tcp_skb_can_collapse_rx()

[Willem de Bruijn]

Jakub Kicinski wrote:
> tcp_skb_can_collapse() checks for conditions which don't make
> sense on input. Because of this we ended up sprinkling a few
> pairs of mptcp_skb_can_collapse() and skb_cmp_decrypted() calls
> on the input path. Group them in a new helper. This should make
> it less likely that someone will check mptcp and not decrypted
> or vice versa when adding new code.
> 
> This implicitly adds a decrypted check early in tcp_collapse().
> AFAIU this will very slightly increase our ability to collapse
> packets under memory pressure, not a real bug.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Willem de Bruijn <willemb@google.com>

[PATCH net-next 2/3] tcp: add a helper for setting EOR on tail skb

[Jakub Kicinski]

TLS (and hopefully soon PSP will) use EOR to prevent skbs
with different decrypted state from getting merged, without
adding new tests to the skb handling. In both cases once
the connection switches to an "encrypted" state, all subsequent
skbs will be encrypted, so a single "EOR fence" is sufficient
to prevent mixing.

Add a helper for setting the EOR bit, to make this arrangement
more explicit.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/tcp.h    |  9 +++++++++
 net/tls/tls_device.c | 11 ++---------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 32741856da01..08c3b99501cf 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1066,6 +1066,7 @@ static inline bool tcp_skb_can_collapse_to(const struct sk_buff *skb)
 static inline bool tcp_skb_can_collapse(const struct sk_buff *to,
 					const struct sk_buff *from)
 {
+	/* skb_cmp_decrypted() not needed, use tcp_write_collapse_fence() */
 	return likely(tcp_skb_can_collapse_to(to) &&
 		      mptcp_skb_can_collapse(to, from) &&
 		      skb_pure_zcopy_same(to, from));
@@ -2102,6 +2103,14 @@ static inline void tcp_rtx_queue_unlink_and_free(struct sk_buff *skb, struct soc
 	tcp_wmem_free_skb(sk, skb);
 }
 
+static inline void tcp_write_collapse_fence(struct sock *sk)
+{
+	struct sk_buff *skb = tcp_write_queue_tail(sk);
+
+	if (skb)
+		TCP_SKB_CB(skb)->eor = 1;
+}
+
 static inline void tcp_push_pending_frames(struct sock *sk)
 {
 	if (tcp_send_head(sk)) {
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index ab6e694f7bc2..dc063c2c7950 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -231,14 +231,10 @@ static void tls_device_resync_tx(struct sock *sk, struct tls_context *tls_ctx,
 				 u32 seq)
 {
 	struct net_device *netdev;
-	struct sk_buff *skb;
 	int err = 0;
 	u8 *rcd_sn;
 
-	skb = tcp_write_queue_tail(sk);
-	if (skb)
-		TCP_SKB_CB(skb)->eor = 1;
-
+	tcp_write_collapse_fence(sk);
 	rcd_sn = tls_ctx->tx.rec_seq;
 
 	trace_tls_device_tx_resync_send(sk, seq, rcd_sn);
@@ -1067,7 +1063,6 @@ int tls_set_device_offload(struct sock *sk)
 	struct tls_prot_info *prot;
 	struct net_device *netdev;
 	struct tls_context *ctx;
-	struct sk_buff *skb;
 	char *iv, *rec_seq;
 	int rc;
 
@@ -1138,9 +1133,7 @@ int tls_set_device_offload(struct sock *sk)
 	 * SKBs where only part of the payload needs to be encrypted.
 	 * So mark the last skb in the write queue as end of record.
 	 */
-	skb = tcp_write_queue_tail(sk);
-	if (skb)
-		TCP_SKB_CB(skb)->eor = 1;
+	tcp_write_collapse_fence(sk);
 
 	/* Avoid offloading if the device is down
 	 * We don't want to offload new flows after
-- 
2.45.1

Re: [PATCH net-next 2/3] tcp: add a helper for setting EOR on tail skb

[Eric Dumazet]

On Fri, May 31, 2024 at 1:36 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> TLS (and hopefully soon PSP will) use EOR to prevent skbs
> with different decrypted state from getting merged, without
> adding new tests to the skb handling. In both cases once
> the connection switches to an "encrypted" state, all subsequent
> skbs will be encrypted, so a single "EOR fence" is sufficient
> to prevent mixing.
>
> Add a helper for setting the EOR bit, to make this arrangement
> more explicit.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next 2/3] tcp: add a helper for setting EOR on tail skb

[Willem de Bruijn]

Jakub Kicinski wrote:
> TLS (and hopefully soon PSP will) use EOR to prevent skbs
> with different decrypted state from getting merged, without
> adding new tests to the skb handling. In both cases once
> the connection switches to an "encrypted" state, all subsequent
> skbs will be encrypted, so a single "EOR fence" is sufficient
> to prevent mixing.
> 
> Add a helper for setting the EOR bit, to make this arrangement
> more explicit.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
>  include/net/tcp.h    |  9 +++++++++
>  net/tls/tls_device.c | 11 ++---------
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 32741856da01..08c3b99501cf 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1066,6 +1066,7 @@ static inline bool tcp_skb_can_collapse_to(const struct sk_buff *skb)
>  static inline bool tcp_skb_can_collapse(const struct sk_buff *to,
>  					const struct sk_buff *from)
>  {
> +	/* skb_cmp_decrypted() not needed, use tcp_write_collapse_fence() */
>  	return likely(tcp_skb_can_collapse_to(to) &&
>  		      mptcp_skb_can_collapse(to, from) &&
>  		      skb_pure_zcopy_same(to, from));
> @@ -2102,6 +2103,14 @@ static inline void tcp_rtx_queue_unlink_and_free(struct sk_buff *skb, struct soc
>  	tcp_wmem_free_skb(sk, skb);
>  }
>  
> +static inline void tcp_write_collapse_fence(struct sock *sk)
> +{

const struct ptr?

> +	struct sk_buff *skb = tcp_write_queue_tail(sk);
> +
> +	if (skb)
> +		TCP_SKB_CB(skb)->eor = 1;
> +}

Re: [PATCH net-next 2/3] tcp: add a helper for setting EOR on tail skb

[Jakub Kicinski]

On Fri, 31 May 2024 09:41:30 -0400 Willem de Bruijn wrote:
> > +static inline void tcp_write_collapse_fence(struct sock *sk)
> > +{  
> 
> const struct ptr?

Maybe just me, but feels kinda weird for the sole input to be const
if the function does modify the object it operates on.

Re: [PATCH net-next 2/3] tcp: add a helper for setting EOR on tail skb

[Willem de Bruijn]

Jakub Kicinski wrote:
> On Fri, 31 May 2024 09:41:30 -0400 Willem de Bruijn wrote:
> > > +static inline void tcp_write_collapse_fence(struct sock *sk)
> > > +{  
> > 
> > const struct ptr?
> 
> Maybe just me, but feels kinda weird for the sole input to be const
> if the function does modify the object it operates on.

Ok. I can see that.

[PATCH net-next 3/3] net: skb: add compatibility warnings to skb_shift()

[Jakub Kicinski]

According to current semantics we should never try to shift data
between skbs which differ on decrypted or pp_recycle status.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/skbuff.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 466999a7515e..c8ac79851cd6 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4139,6 +4139,9 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
 	if (skb_zcopy(tgt) || skb_zcopy(skb))
 		return 0;
 
+	DEBUG_NET_WARN_ON_ONCE(tgt->pp_recycle != skb->pp_recycle);
+	DEBUG_NET_WARN_ON_ONCE(skb_cmp_decrypted(tgt, skb));
+
 	todo = shiftlen;
 	from = 0;
 	to = skb_shinfo(tgt)->nr_frags;
-- 
2.45.1

Re: [PATCH net-next 3/3] net: skb: add compatibility warnings to skb_shift()

[Eric Dumazet]

On Fri, May 31, 2024 at 1:36 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> According to current semantics we should never try to shift data
> between skbs which differ on decrypted or pp_recycle status.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next 3/3] net: skb: add compatibility warnings to skb_shift()

[Willem de Bruijn]

Jakub Kicinski wrote:
> According to current semantics we should never try to shift data
> between skbs which differ on decrypted or pp_recycle status.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net-next 0/3] tcp: refactor skb_cmp_decrypted() checks

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Thu, 30 May 2024 16:36:13 -0700 you wrote:
> Refactor the input patch coalescing checks and wrap "EOR forcing"
> logic into a helper. This will hopefully make the code easier to
> follow. While at it throw some DEBUG_NET checks into skb_shift().
> 
> Jakub Kicinski (3):
>   tcp: wrap mptcp and decrypted checks into tcp_skb_can_collapse_rx()
>   tcp: add a helper for setting EOR on tail skb
>   net: skb: add compatibility warnings to skb_shift()
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] tcp: wrap mptcp and decrypted checks into tcp_skb_can_collapse_rx()
    https://git.kernel.org/netdev/net-next/c/071115301838
  - [net-next,2/3] tcp: add a helper for setting EOR on tail skb
    https://git.kernel.org/netdev/net-next/c/1be68a87ab33
  - [net-next,3/3] net: skb: add compatibility warnings to skb_shift()
    https://git.kernel.org/netdev/net-next/c/99b8add01f98

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html