Netdev List
 help / color / mirror / Atom feed
* [PATCH net] tls: device: keep tls_sw_fallback() from encrypting its own output
@ 2026-10-19 21:26 Rishikesh Jethwani
  2026-10-21  9:04 ` Sabrina Dubroca
  2026-10-23 21:39 ` netdev-bot+sashiko
  0 siblings, 2 replies; 5+ messages in thread
From: Rishikesh Jethwani @ 2026-10-19 21:26 UTC (permalink / raw)
  To: netdev
  Cc: john.fastabend, kuba, sd, davem, edumazet, pabeni, horms,
	linux-kernel, stable, Rishikesh Jethwani

A socket offloaded through a bond has every skb validated twice, once
at the master and again at the slave. tls_validate_xmit_skb() passes
the master-level call through for that reason (netif_is_bond_master()),
but tls_validate_xmit_skb_sw(), the validator tls_device_down()
installs, calls tls_sw_fallback() unconditionally, so it encrypts the
master-level ciphertext a second time with the same rcd_sn and IV. This
cancels the CTR keystream and exposes plaintext on the wire. For a
complete record encrypted twice in one skb, the second pass also
generates a valid GCM tag, so the peer accepts corrupted data without
an authentication failure.

Guard on the content rather than the device. Offload plaintext always
carries the decrypted bit (tls_push_data() and
tls_device_write_space() send with MSG_SENDPAGE_DECRYPTED), so
tls_sw_fallback() can pass through any skb without it: such a skb needs
no offload encryption. complete_skb() now clears the bit that
skb_copy_header() copied from the plaintext original, so the fallback's
own output passes through any later validation pass untouched. This
also does not rely on every path validating a skb at both the master
and the slave.

Tested on two hosts back to back over a mlx5 port, the only slave of
a bond0 (balance-xor, layer3+4), so each transmit is validated at the
master and again at the slave. Run from each host as sender, a TLS 1.2
AES-GCM-128 TX socket kept open across an administrative bounce of the
slave leaked every later record as plaintext before the fix; after it
each record is encrypted exactly once and the kTLS peer reads the
stream intact.

Fixes: c55dcdd435aa ("net/tls: Fix use-after-free after the TLS device goes down and up")
Cc: stable@vger.kernel.org # needs commit fc6d80eb5044 ("tcp: prevent collapsing skbs across boundary in rtx queue")
Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
---
 net/tls/tls_device_fallback.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/net/tls/tls_device_fallback.c b/net/tls/tls_device_fallback.c
index 3b7d0ab2bcf1..24fba12febd1 100644
--- a/net/tls/tls_device_fallback.c
+++ b/net/tls/tls_device_fallback.c
@@ -192,6 +192,14 @@ static void complete_skb(struct sk_buff *nskb, struct sk_buff *skb, int headln)
 	skb->destructor = NULL;
 	skb->sk = NULL;
 
+	/* nskb carries ciphertext now, but skb_copy_header() inherited the
+	 * plaintext original's decrypted bit. Clear it so the bit keeps meaning
+	 * "still-plaintext, needs an encryptor": a requeued nskb would otherwise
+	 * be re-validated and encrypted a second time on any re-entry into
+	 * tls_sw_fallback().
+	 */
+	nskb->decrypted = 0;
+
 	update_chksum(nskb, headln);
 
 	/* sock_efree means skb must gone through skb_orphan_partial() */
@@ -371,6 +379,17 @@ static struct sk_buff *tls_sw_fallback(struct sock *sk, struct sk_buff *skb)
 	s32 sync_size = 0;
 	u64 rcd_sn;
 
+	/* Only skbs carrying the decrypted bit hold plaintext that awaits
+	 * offload encryption; anything else needs no work here. That
+	 * includes this function's own output (complete_skb() clears the
+	 * bit skb_copy_header() inherited), which is seen again when a skb
+	 * is validated a second time, e.g. a bond master handing it to a
+	 * slave via bond_dev_queue_xmit(). fill_sg_in() would map it back
+	 * onto its own record by tcp_seq, reusing rcd_sn and IV.
+	 */
+	if (!skb_is_decrypted(skb))
+		return skb;
+
 	/* worst case is:
 	 * MAX_SKB_FRAGS in tls_record_info
 	 * MAX_SKB_FRAGS + 1 in SKB head and frags.
-- 
2.50.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net] tls: device: keep tls_sw_fallback() from encrypting its own output
  2026-10-19 21:26 [PATCH net] tls: device: keep tls_sw_fallback() from encrypting its own output Rishikesh Jethwani
@ 2026-10-21  9:04 ` Sabrina Dubroca
  2026-10-22 22:36   ` Rishikesh Jethwani
  2026-10-23 21:39 ` netdev-bot+sashiko
  1 sibling, 1 reply; 5+ messages in thread
From: Sabrina Dubroca @ 2026-10-21  9:04 UTC (permalink / raw)
  To: Rishikesh Jethwani
  Cc: netdev, john.fastabend, kuba, davem, edumazet, pabeni, horms,
	linux-kernel, stable

2026-10-19, 15:26:51 -0600, Rishikesh Jethwani wrote:
> A socket offloaded through a bond has every skb validated twice, once
> at the master and again at the slave. tls_validate_xmit_skb() passes
> the master-level call through for that reason (netif_is_bond_master()),
> but tls_validate_xmit_skb_sw(), the validator tls_device_down()
> installs, calls tls_sw_fallback() unconditionally, so it encrypts the
> master-level ciphertext a second time with the same rcd_sn and IV. This
> cancels the CTR keystream and exposes plaintext on the wire. For a
> complete record encrypted twice in one skb, the second pass also
> generates a valid GCM tag, so the peer accepts corrupted data without
> an authentication failure.
> 
> Guard on the content rather than the device. Offload plaintext always
> carries the decrypted bit (tls_push_data() and
> tls_device_write_space() send with MSG_SENDPAGE_DECRYPTED), so
> tls_sw_fallback() can pass through any skb without it: such a skb needs
> no offload encryption. complete_skb() now clears the bit that
> skb_copy_header() copied from the plaintext original, so the fallback's
> own output passes through any later validation pass untouched. This
> also does not rely on every path validating a skb at both the master
> and the slave.
> 
> Tested on two hosts back to back over a mlx5 port, the only slave of
> a bond0 (balance-xor, layer3+4), so each transmit is validated at the

As in another patch, a couple of selftests for "offload over
bond->NIC" would be good.

> master and again at the slave. Run from each host as sender, a TLS 1.2
> AES-GCM-128 TX socket kept open across an administrative bounce of the
> slave leaked every later record as plaintext before the fix; after it
> each record is encrypted exactly once and the kTLS peer reads the
> stream intact.

The wording and verbosity of this section ("administrative bounce", 3
lines to say "before it didn't work and now it does" which IMO
shouldn't even have to be said) needs to be cleaned up. The rest of
the commit message as well.

> Fixes: c55dcdd435aa ("net/tls: Fix use-after-free after the TLS device goes down and up")
> Cc: stable@vger.kernel.org # needs commit fc6d80eb5044 ("tcp: prevent collapsing skbs across boundary in rtx queue")
> Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
> ---
>  net/tls/tls_device_fallback.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/net/tls/tls_device_fallback.c b/net/tls/tls_device_fallback.c
> index 3b7d0ab2bcf1..24fba12febd1 100644
> --- a/net/tls/tls_device_fallback.c
> +++ b/net/tls/tls_device_fallback.c
> @@ -192,6 +192,14 @@ static void complete_skb(struct sk_buff *nskb, struct sk_buff *skb, int headln)
>  	skb->destructor = NULL;
>  	skb->sk = NULL;
>  
> +	/* nskb carries ciphertext now, but skb_copy_header() inherited the
> +	 * plaintext original's decrypted bit. Clear it so the bit keeps meaning
> +	 * "still-plaintext, needs an encryptor": a requeued nskb would otherwise
> +	 * be re-validated and encrypted a second time on any re-entry into
> +	 * tls_sw_fallback().
> +	 */

"nskb is encrypted now" is enough. the rest of this comment (why it
was set and what the bug would be) really doesn't belong here.

> +	nskb->decrypted = 0;
> +
>  	update_chksum(nskb, headln);
>  
>  	/* sock_efree means skb must gone through skb_orphan_partial() */
> @@ -371,6 +379,17 @@ static struct sk_buff *tls_sw_fallback(struct sock *sk, struct sk_buff *skb)
>  	s32 sync_size = 0;
>  	u64 rcd_sn;
>  
> +	/* Only skbs carrying the decrypted bit hold plaintext that awaits
> +	 * offload encryption; anything else needs no work here. That
> +	 * includes this function's own output (complete_skb() clears the
> +	 * bit skb_copy_header() inherited), which is seen again when a skb
> +	 * is validated a second time, e.g. a bond master handing it to a
> +	 * slave via bond_dev_queue_xmit(). fill_sg_in() would map it back
> +	 * onto its own record by tcp_seq, reusing rcd_sn and IV.
> +	 */

Sorry, I'm not reading all this. 0 or 1 line of comment here, nothing more.

> +	if (!skb_is_decrypted(skb))
> +		return skb;
> +
>  	/* worst case is:
>  	 * MAX_SKB_FRAGS in tls_record_info
>  	 * MAX_SKB_FRAGS + 1 in SKB head and frags.
> -- 
> 2.50.1
> 

-- 
Sabrina

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] tls: device: keep tls_sw_fallback() from encrypting its own output
  2026-10-21  9:04 ` Sabrina Dubroca
@ 2026-10-22 22:36   ` Rishikesh Jethwani
  0 siblings, 0 replies; 5+ messages in thread
From: Rishikesh Jethwani @ 2026-10-22 22:36 UTC (permalink / raw)
  To: Sabrina Dubroca
  Cc: netdev, john.fastabend, kuba, davem, edumazet, pabeni, horms,
	linux-kernel, stable

On Wed, Oct 21, 2026 at 2:04 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
>
> 2026-10-19, 15:26:51 -0600, Rishikesh Jethwani wrote:
> > A socket offloaded through a bond has every skb validated twice, once
> > at the master and again at the slave. tls_validate_xmit_skb() passes
> > the master-level call through for that reason (netif_is_bond_master()),
> > but tls_validate_xmit_skb_sw(), the validator tls_device_down()
> > installs, calls tls_sw_fallback() unconditionally, so it encrypts the
> > master-level ciphertext a second time with the same rcd_sn and IV. This
> > cancels the CTR keystream and exposes plaintext on the wire. For a
> > complete record encrypted twice in one skb, the second pass also
> > generates a valid GCM tag, so the peer accepts corrupted data without
> > an authentication failure.
> >
> > Guard on the content rather than the device. Offload plaintext always
> > carries the decrypted bit (tls_push_data() and
> > tls_device_write_space() send with MSG_SENDPAGE_DECRYPTED), so
> > tls_sw_fallback() can pass through any skb without it: such a skb needs
> > no offload encryption. complete_skb() now clears the bit that
> > skb_copy_header() copied from the plaintext original, so the fallback's
> > own output passes through any later validation pass untouched. This
> > also does not rely on every path validating a skb at both the master
> > and the slave.
> >
> > Tested on two hosts back to back over a mlx5 port, the only slave of
> > a bond0 (balance-xor, layer3+4), so each transmit is validated at the
>
> As in another patch, a couple of selftests for "offload over
> bond->NIC" would be good.

Will do. The hw offload test these would go into is still under
review:

https://lore.kernel.org/netdev/20261020181147.939575-2-rjethwani@purestorage.com/

Would you prefer I fold this fix and the bond tests into the next
revision of that series, or wait until it's applied and send v2 of
this patch with the tests on top?

> > master and again at the slave. Run from each host as sender, a TLS 1.2
> > AES-GCM-128 TX socket kept open across an administrative bounce of the
> > slave leaked every later record as plaintext before the fix; after it
> > each record is encrypted exactly once and the kTLS peer reads the
> > stream intact.
>
> The wording and verbosity of this section ("administrative bounce", 3
> lines to say "before it didn't work and now it does" which IMO
> shouldn't even have to be said) needs to be cleaned up. The rest of
> the commit message as well.
>
> > Fixes: c55dcdd435aa ("net/tls: Fix use-after-free after the TLS device goes down and up")
> > Cc: stable@vger.kernel.org # needs commit fc6d80eb5044 ("tcp: prevent collapsing skbs across boundary in rtx queue")
> > Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
> > ---
> >  net/tls/tls_device_fallback.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/net/tls/tls_device_fallback.c b/net/tls/tls_device_fallback.c
> > index 3b7d0ab2bcf1..24fba12febd1 100644
> > --- a/net/tls/tls_device_fallback.c
> > +++ b/net/tls/tls_device_fallback.c
> > @@ -192,6 +192,14 @@ static void complete_skb(struct sk_buff *nskb, struct sk_buff *skb, int headln)
> >       skb->destructor = NULL;
> >       skb->sk = NULL;
> >
> > +     /* nskb carries ciphertext now, but skb_copy_header() inherited the
> > +      * plaintext original's decrypted bit. Clear it so the bit keeps meaning
> > +      * "still-plaintext, needs an encryptor": a requeued nskb would otherwise
> > +      * be re-validated and encrypted a second time on any re-entry into
> > +      * tls_sw_fallback().
> > +      */
>
> "nskb is encrypted now" is enough. the rest of this comment (why it
> was set and what the bug would be) really doesn't belong here.
>
> > +     nskb->decrypted = 0;
> > +
> >       update_chksum(nskb, headln);
> >
> >       /* sock_efree means skb must gone through skb_orphan_partial() */
> > @@ -371,6 +379,17 @@ static struct sk_buff *tls_sw_fallback(struct sock *sk, struct sk_buff *skb)
> >       s32 sync_size = 0;
> >       u64 rcd_sn;
> >
> > +     /* Only skbs carrying the decrypted bit hold plaintext that awaits
> > +      * offload encryption; anything else needs no work here. That
> > +      * includes this function's own output (complete_skb() clears the
> > +      * bit skb_copy_header() inherited), which is seen again when a skb
> > +      * is validated a second time, e.g. a bond master handing it to a
> > +      * slave via bond_dev_queue_xmit(). fill_sg_in() would map it back
> > +      * onto its own record by tcp_seq, reusing rcd_sn and IV.
> > +      */
>
> Sorry, I'm not reading all this. 0 or 1 line of comment here, nothing more.
>
> > +     if (!skb_is_decrypted(skb))
> > +             return skb;
> > +
> >       /* worst case is:
> >        * MAX_SKB_FRAGS in tls_record_info
> >        * MAX_SKB_FRAGS + 1 in SKB head and frags.
> > --
> > 2.50.1
> >
>
> --
> Sabrina

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] tls: device: keep tls_sw_fallback() from encrypting its own output
  2026-10-19 21:26 [PATCH net] tls: device: keep tls_sw_fallback() from encrypting its own output Rishikesh Jethwani
  2026-10-21  9:04 ` Sabrina Dubroca
@ 2026-10-23 21:39 ` netdev-bot+sashiko
  2026-10-27 19:12   ` Rishikesh Jethwani
  1 sibling, 1 reply; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-10-23 21:39 UTC (permalink / raw)
  To: rjethwani
  Cc: netdev, john.fastabend, kuba, sd, davem, edumazet, pabeni, horms,
	linux-kernel, stable

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch makes complete_skb() in
  net/tls/tls_device_fallback.c run `nskb->decrypted = 0;` on every skb…

--

tls: device: keep tls_sw_fallback() from encrypting its own output

This patch makes tls_sw_fallback() pass through any skb that lacks the
decrypted bit. It also makes complete_skb() clear the decrypted bit on
the fallback's output. Together these stop a skb from being encrypted a
second time when it is validated at both a bond master and its slave.

> diff --git a/net/tls/tls_device_fallback.c b/net/tls/tls_device_fallback.c
> index 3b7d0ab2bcf1..24fba12febd1 100644
> --- a/net/tls/tls_device_fallback.c
> +++ b/net/tls/tls_device_fallback.c
> @@ -192,6 +192,14 @@ static void complete_skb(struct sk_buff *nskb, struct sk_buff *skb, int headln)
>  	skb->destructor = NULL;
>  	skb->sk = NULL;
>
> +	/* nskb carries ciphertext now, but skb_copy_header() inherited the
> +	 * plaintext original's decrypted bit. Clear it so the bit keeps meaning
> +	 * "still-plaintext, needs an encryptor": a requeued nskb would otherwise
> +	 * be re-validated and encrypted a second time on any re-entry into
> +	 * tls_sw_fallback().
> +	 */
> +	nskb->decrypted = 0;

[Severity: Medium]
Can clearing skb->decrypted here strip PSP encapsulation from fallback
output?

PSP also uses skb->decrypted on transmit, but it means "this skb belongs
to a PSP TX association". The bit is set at enqueue time:

include/net/psp/functions.h:psp_enqueue_set_decrypted() {
	...
	pas = psp_sk_assoc(sk);
	if (pas && pas->tx.spi)
		skb->decrypted = 1;
}

Drivers then use it to decide on PSP TX offload:

include/net/psp/functions.h:psp_skb_get_assoc_rcu() {
	if (!skb->decrypted || !skb->sk)
		return NULL;

	return psp_sk_get_assoc_rcu(skb->sk);
}

mlx5 reaches this through mlx5e_accel_tx_begin()->mlx5e_psp_is_offload(),
and netdevsim through nsim_do_psp(). mlx5e_accel_tx_begin() applies kTLS
and then PSP to the same skb.

Nothing seems to prevent one socket from having both features.
psp_sock_assoc_set_tx() installs psp_validate_xmit without checking for a
TLS ULP. tls_set_device_offload() overwrites sk->sk_validate_xmit_skb
unconditionally. Documentation/networking/psp.rst also describes starting
with TLS and then switching to PSP.

On such a socket the TLS SW fallback can engage in two ways.
tls_device_down() can install tls_validate_xmit_skb_sw() after a port
bounce, or tls_validate_xmit_skb() can fall back after a reroute. The
path is then:

tls_sw_fallback()
  tls_enc_skb()
    complete_skb()
      nskb->decrypted = 0;
mlx5e_accel_tx_begin()
  mlx5e_psp_is_offload()
    psp_skb_get_assoc_rcu() returns NULL

The result is that the segment goes out as plain TCP with no PSP header.

Before this patch, skb_copy_header() carried decrypted=1 over to nskb, so
PSP encapsulation still happened. Once the fallback is active,
retransmits go through it as well. The peer's __psp_sk_rx_policy_check()
would then drop the non-PSP data segments, so the connection could stall.
PSP protection would also be skipped silently on the wire, although the
payload stays TLS-encrypted.

Could this be fixed without clearing skb->decrypted? Some options:

- Use a separate marker for fallback output.
- Skip the bond-master pass in tls_validate_xmit_skb_sw() the same way
  tls_validate_xmit_skb() already does with netif_is_bond_master().
- Keep the bit when the socket has a PSP TX association.

> +
>  	update_chksum(nskb, headln);
>
>  	/* sock_efree means skb must gone through skb_orphan_partial() */

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019212651.733565-1-rjethwani%40purestorage.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] tls: device: keep tls_sw_fallback() from encrypting its own output
  2026-10-23 21:39 ` netdev-bot+sashiko
@ 2026-10-27 19:12   ` Rishikesh Jethwani
  0 siblings, 0 replies; 5+ messages in thread
From: Rishikesh Jethwani @ 2026-10-27 19:12 UTC (permalink / raw)
  To: netdev-bot+sashiko
  Cc: netdev, john.fastabend, kuba, sd, davem, edumazet, pabeni, horms,
	linux-kernel, stable

On Fri, Oct 23, 2026 at 2:39 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The patch makes complete_skb() in
>   net/tls/tls_device_fallback.c run `nskb->decrypted = 0;` on every skb…
>
> --
>
> tls: device: keep tls_sw_fallback() from encrypting its own output
>
> This patch makes tls_sw_fallback() pass through any skb that lacks the
> decrypted bit. It also makes complete_skb() clear the decrypted bit on
> the fallback's output. Together these stop a skb from being encrypted a
> second time when it is validated at both a bond master and its slave.
>
> > diff --git a/net/tls/tls_device_fallback.c b/net/tls/tls_device_fallback.c
> > index 3b7d0ab2bcf1..24fba12febd1 100644
> > --- a/net/tls/tls_device_fallback.c
> > +++ b/net/tls/tls_device_fallback.c
> > @@ -192,6 +192,14 @@ static void complete_skb(struct sk_buff *nskb, struct sk_buff *skb, int headln)
> >       skb->destructor = NULL;
> >       skb->sk = NULL;
> >
> > +     /* nskb carries ciphertext now, but skb_copy_header() inherited the
> > +      * plaintext original's decrypted bit. Clear it so the bit keeps meaning
> > +      * "still-plaintext, needs an encryptor": a requeued nskb would otherwise
> > +      * be re-validated and encrypted a second time on any re-entry into
> > +      * tls_sw_fallback().
> > +      */
> > +     nskb->decrypted = 0;
>
> [Severity: Medium]
> Can clearing skb->decrypted here strip PSP encapsulation from fallback
> output?
>
> PSP also uses skb->decrypted on transmit, but it means "this skb belongs
> to a PSP TX association". The bit is set at enqueue time:
>
> include/net/psp/functions.h:psp_enqueue_set_decrypted() {
>         ...
>         pas = psp_sk_assoc(sk);
>         if (pas && pas->tx.spi)
>                 skb->decrypted = 1;
> }
>
> Drivers then use it to decide on PSP TX offload:
>
> include/net/psp/functions.h:psp_skb_get_assoc_rcu() {
>         if (!skb->decrypted || !skb->sk)
>                 return NULL;
>
>         return psp_sk_get_assoc_rcu(skb->sk);
> }
>
> mlx5 reaches this through mlx5e_accel_tx_begin()->mlx5e_psp_is_offload(),
> and netdevsim through nsim_do_psp(). mlx5e_accel_tx_begin() applies kTLS
> and then PSP to the same skb.
>
> Nothing seems to prevent one socket from having both features.
> psp_sock_assoc_set_tx() installs psp_validate_xmit without checking for a
> TLS ULP. tls_set_device_offload() overwrites sk->sk_validate_xmit_skb
> unconditionally. Documentation/networking/psp.rst also describes starting
> with TLS and then switching to PSP.
>
> On such a socket the TLS SW fallback can engage in two ways.
> tls_device_down() can install tls_validate_xmit_skb_sw() after a port
> bounce, or tls_validate_xmit_skb() can fall back after a reroute. The
> path is then:
>
> tls_sw_fallback()
>   tls_enc_skb()
>     complete_skb()
>       nskb->decrypted = 0;
> mlx5e_accel_tx_begin()
>   mlx5e_psp_is_offload()
>     psp_skb_get_assoc_rcu() returns NULL
>
> The result is that the segment goes out as plain TCP with no PSP header.
>
> Before this patch, skb_copy_header() carried decrypted=1 over to nskb, so
> PSP encapsulation still happened. Once the fallback is active,
> retransmits go through it as well. The peer's __psp_sk_rx_policy_check()
> would then drop the non-PSP data segments, so the connection could stall.
> PSP protection would also be skipped silently on the wire, although the
> payload stays TLS-encrypted.
>
> Could this be fixed without clearing skb->decrypted? Some options:

A socket can't have both. Commit a41f24c612c3 ("net: psp: avoid conflicts
with skb->decrypted and sk_validate_xmit_skb()") made PSP and TCP ULPs
mutually exclusive for exactly this reason, and sk_has_decrypt_user() in
net/core/sock.c enforces it:

        bool sk_has_decrypt_user(const struct sock *sk)
        {
                return psp_sk_assoc(sk) ||
                       (sk_is_inet(sk) && inet_csk_has_ulp(sk)); /* for tls */
        }

It is checked in both directions:

 - __tcp_set_ulp() returns -EINVAL if the socket already has a PSP
   assoc, so TLS_TX can never be configured on a PSP socket.

 - psp_sock_assoc_set_rx() fails with "Socket has incompatible state"
   if a ULP is attached. psp_sock_assoc_set_tx() requires an existing
   RX assoc ("Socket has no Rx key"), so a kTLS socket can't get a PSP
   TX assoc either.

Neither the ULP nor sk->psp_assoc can be removed while the socket is
alive: tcp_cleanup_ulp() only runs from tcp_v4_destroy_sock(), and
psp_sk_assoc_free() only from inet_sock_destruct(). The TX check doesn't
look for a TLS ULP because the RX step already did.

The listener gap that commit mentions doesn't affect this either. PSP has
no TCP_LISTEN check, so a listener can hold an RX assoc, but sk_clone()
clears newsk->psp_assoc and nothing re-sets it during child creation. An
accepted socket therefore starts with no PSP assoc, and __tcp_set_ulp()
succeeds on it precisely because of that.

So any skb that reaches tls_sw_fallback() belongs to a socket with no
PSP assoc. Its decrypted bit only ever has the TLS meaning ("plaintext
for offload"), and clearing it on the encrypted output can't affect
PSP.

Your mechanism is otherwise correct: decrypted is inside the headers group,
so before this patch skb_copy_header() carried decrypted=1 over to nskb, and
clearing it would make psp_skb_get_assoc_rcu() return NULL and drop the PSP
encapsulation. It just isn't reachable.

I considered the bond-master skip instead, but it would make correctness
depend on tls_validate_xmit_skb_sw() knowing every upper device that
validates the skb before the real one. Bond is only the one that is
special-cased today. If an offloaded socket is rerouted through a VLAN
or macsec device, the skb is validated there and again at the lower
device, since both re-queue with dev_queue_xmit(). Clearing decrypted
makes the fallback idempotent by construction: its output is never
encrypted again, whatever path the skb takes.

As for Documentation/networking/psp.rst: that passage describes using TLS to
negotiate keys for a connection, not attaching both features to one socket.
a41f24c612c3 deliberately closed the combination ("PSP was not designed to
be used with ULPs") and left the doc alone. I can post a separate doc patch
if the wording is worth tightening.

I'll keep the patch as is.

> - Use a separate marker for fallback output.
> - Skip the bond-master pass in tls_validate_xmit_skb_sw() the same way
>   tls_validate_xmit_skb() already does with netif_is_bond_master().
> - Keep the bit when the socket has a PSP TX association.
>
> > +
> >       update_chksum(nskb, headln);
> >
> >       /* sock_efree means skb must gone through skb_orphan_partial() */
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019212651.733565-1-rjethwani%40purestorage.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-10-27 19:12 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-19 21:26 [PATCH net] tls: device: keep tls_sw_fallback() from encrypting its own output Rishikesh Jethwani
2026-10-21  9:04 ` Sabrina Dubroca
2026-10-22 22:36   ` Rishikesh Jethwani
2026-10-23 21:39 ` netdev-bot+sashiko
2026-10-27 19:12   ` Rishikesh Jethwani

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox