| Message ID | 20240530233616.85897-3-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | tcp: refactor skb_cmp_decrypted() checks | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 53 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
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>
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; > +}
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.
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.
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
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(-)