Netdev List
 help / color / mirror / Atom feed
* Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs
@ 2026-10-22 18:17 Rishikesh Jethwani
  2026-11-03 14:45 ` Tariq Toukan
  0 siblings, 1 reply; 9+ messages in thread
From: Rishikesh Jethwani @ 2026-10-22 18:17 UTC (permalink / raw)
  To: tariqt
  Cc: Saeed Mahameed, Mark Bloch, Gal Pressman, Jakub Kicinski,
	Boris Pismenny, Sabrina Dubroca, John Fastabend, netdev

Hi Tariq,

Could a DUMP/SEND WQE from an old owner on SQ A read or update TIS
state after SQ B has reinitialised that TIS for a new owner?

The existing TIS pool predates live rekey. My TLS 1.3 TX KeyUpdate
series (v17 [1]; v18 in preparation) calls tls_dev_del() and then
tls_dev_add() on a live socket, and the question arises through this
sequence:

  1. A spurious RTO retransmit of old-key data is posted on SQ A.
  2. While that data remains unacknowledged, setsockopt(TLS_TX) starts
     rekey and installs the rekey validator, which stops admitting new
     plaintext to hardware offload. An ACK covering all remaining
     old-key data marks rekey ready before the driver observes the
     retransmit's SQ completion. A subsequent sendmsg() enters rekey
     completion.
  3. Rekey completion calls synchronize_net(), which waits for CPU
     transmit paths, not SQ completions, so a WQE on a stalled or
     backlogged SQ A can still be pending. mlx5e_ktls_del_tx() then
     releases the DEK and immediately calls pool_push(). DEK
     reclamation uses batched SYNC_CRYPTO; returning the TIS does not
     wait for synchronization covering its old DEK.
  4. Before re-adding the offload, TLS allocates the new SW-fallback
     AEAD. In that interval another connection's mlx5e_ktls_add_tx()
     can pop the returned TIS and use it on SQ B. The same socket can
     also reach SQ B on re-add: with the retransmit queue empty, TCP
     sets ooo_okay even while the retransmitted skb still holds
     sk_wmem_alloc, so queue selection may move the flow.
     mlx5e_ktls_handle_tx_skb() posts the new owner's initial
     parameters with skip_static_post=false, fence_first_post=false:
     static parameters unfenced, progress parameters fenced, then a
     fence NOP.

An unobserved completion does not by itself mean the WQE still accesses
the TIS; that is the hardware ordering question. The same question
applies on SQ A: resync fences its first parameter update, initial
setup does not, and I do not know what either guarantees against
earlier WQEs that reference the TIS.

The normal close path has additional ordering. tls_dev_del() runs
from work queued by tls_device_sk_destruct(), which itself runs from
__sk_free(), i.e. only once sk_wmem_alloc reaches zero. For
transmitted mlx5 skbs, SQ completion calls napi_consume_skb(), which
invokes tcp_wfree() to release that accounting, and parameter and DUMP
WQEs preceding a posted SEND on the same SQ are retired by that SEND's
completion. For successful transmissions, this gives close-time
ordering without an explicit drain in del_tx. WQEs posted without a
subsequent SEND, and SQ error/reset paths, need separate lifetime
guarantees. Live rekey does not wait for socket destruction.

Was the recycling fence relying on this socket-lifetime ordering, or
does it independently protect against outstanding work on another SQ?
c4dfe704f53f ("net/mlx5e: kTLS, Recycle objects of device-offloaded
TLS TX connections") says the fence ensures context modification does
not interfere with existing traffic. Does that cover old SEND, DUMP
and parameter WQEs on another SQ, including work the device has not
yet executed?

If reuse must wait, would retaining the TIS and DEK until all
referencing work has completed or been definitively stopped suffice,
including during SQ error/reset?

I have not reproduced a failure; the reuse is visible in the driver,
but I have not established the hardware ordering guarantee. A PRM
reference, or a statement of which completion must precede reuse,
would settle it.

Thanks,
Rishikesh

[1] https://lore.kernel.org/netdev/20260917224355.2288021-1-rjethwani@purestorage.com/

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

* Re: Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs
  2026-10-22 18:17 Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs Rishikesh Jethwani
@ 2026-11-03 14:45 ` Tariq Toukan
  2026-11-04  5:01   ` Rishikesh Jethwani
  0 siblings, 1 reply; 9+ messages in thread
From: Tariq Toukan @ 2026-11-03 14:45 UTC (permalink / raw)
  To: Rishikesh Jethwani, tariqt
  Cc: Saeed Mahameed, Mark Bloch, Gal Pressman, Jakub Kicinski,
	Boris Pismenny, Sabrina Dubroca, John Fastabend, netdev



On 22/10/2026 21:17, Rishikesh Jethwani wrote:
> Hi Tariq,
> 

Hi Rishikesh,

Good analysis, and thanks for the detailed write-up.

> Could a DUMP/SEND WQE from an old owner on SQ A read or update TIS
> state after SQ B has reinitialised that TIS for a new owner?
> 

Short answer: Yes, in principle it could, once del runs on a live socket 
as in your series. Today, only parameter/DUMP WQEs of a dropped skb can 
be left behind (item 1 below): a pending SEND holds sk_wmem_alloc, so 
the socket is not destructed before it completes.

The TIS is a function-global object, not bound to an SQ: a WQE that 
references its TISN reads/updates its state whenever that WQE executes, 
from whichever SQ it was posted on, and the hardware provides no 
cross-SQ ordering for such accesses. Details below, and at the end a 
proposal for the way forward, on which I'd appreciate your feedback.

> The existing TIS pool predates live rekey. My TLS 1.3 TX KeyUpdate
> series (v17 [1]; v18 in preparation) calls tls_dev_del() and then
> tls_dev_add() on a live socket, and the question arises through this
> sequence:
> 
>    1. A spurious RTO retransmit of old-key data is posted on SQ A.
>    2. While that data remains unacknowledged, setsockopt(TLS_TX) starts
>       rekey and installs the rekey validator, which stops admitting new
>       plaintext to hardware offload. An ACK covering all remaining
>       old-key data marks rekey ready before the driver observes the
>       retransmit's SQ completion. A subsequent sendmsg() enters rekey
>       completion.
>    3. Rekey completion calls synchronize_net(), which waits for CPU
>       transmit paths, not SQ completions, so a WQE on a stalled or
>       backlogged SQ A can still be pending. mlx5e_ktls_del_tx() then
>       releases the DEK and immediately calls pool_push(). DEK
>       reclamation uses batched SYNC_CRYPTO; returning the TIS does not
>       wait for synchronization covering its old DEK.
>    4. Before re-adding the offload, TLS allocates the new SW-fallback
>       AEAD. In that interval another connection's mlx5e_ktls_add_tx()
>       can pop the returned TIS and use it on SQ B. The same socket can
>       also reach SQ B on re-add: with the retransmit queue empty, TCP
>       sets ooo_okay even while the retransmitted skb still holds
>       sk_wmem_alloc, so queue selection may move the flow.
>       mlx5e_ktls_handle_tx_skb() posts the new owner's initial
>       parameters with skip_static_post=false, fence_first_post=false:
>       static parameters unfenced, progress parameters fenced, then a
>       fence NOP.
> 

Your reading of the driver is accurate on all points. In particular,
mlx5e_ktls_del_tx() returns the TIS to the pool with no completion
tracking. The same goes for the DEK: the batched SYNC_CRYPTO guards
against stale cached keys when a DEK object is reused, but nothing
waits for pending WQEs before that, and without bulk DEK support the
DEK is destroyed right away. So the DEK needs the same lifetime
guarantee as the TIS.

The same-socket exposure is also broader than the ooo_okay disjunct:
the RTO and PLB rehash paths reset the dst, which clears the cached
TX queue, and txq_reselection_ms lets a busy flow re-pick its queue
periodically, both regardless of ooo_okay. So one connection's WQEs
can be in flight on two SQs today, even without rekey.

> An unobserved completion does not by itself mean the WQE still accesses
> the TIS; that is the hardware ordering question. The same question
> applies on SQ A: resync fences its first parameter update, initial
> setup does not, and I do not know what either guarantees against
> earlier WQEs that reference the TIS.
> 

The fence modes in the WQE ctrl segment (FM field; the parameter WQEs
use Initiator_Small_Fence) are all defined in terms of previous WQEs
on the same SQ: a fenced WQE waits until earlier WQEs on that SQ are
no longer in execution. Even the strongest mode (Strong_Ordering)
orders only against previous WQEs of the same SQ. No fence mode, and
no other WQE-level primitive, orders against outstanding work on
another SQ.

So within one SQ, the fenced first parameter WQE of a resync does
guarantee that earlier WQEs referencing the TIS are done executing
before the context is modified. The unfenced static parameters of the
initial setup are safe only because, under the current lifetime
contract, the previous owner's WQEs are known to be complete by the
time the TIS leaves the pool. Across SQs, nothing is ordered.

> The normal close path has additional ordering. tls_dev_del() runs
> from work queued by tls_device_sk_destruct(), which itself runs from
> __sk_free(), i.e. only once sk_wmem_alloc reaches zero. For
> transmitted mlx5 skbs, SQ completion calls napi_consume_skb(), which
> invokes tcp_wfree() to release that accounting, and parameter and DUMP
> WQEs preceding a posted SEND on the same SQ are retired by that SEND's
> completion. For successful transmissions, this gives close-time
> ordering without an explicit drain in del_tx. WQEs posted without a
> subsequent SEND, and SQ error/reset paths, need separate lifetime
> guarantees. Live rekey does not wait for socket destruction.
> 

Correct, including the completion detail: parameter and DUMP WQEs do
not request CQEs of their own, so they are retired only when a later
CQE-generating WQE on the same SQ completes. And you have a point
about WQEs posted without a subsequent SEND: the error paths of
mlx5e_ktls_handle_tx_skb(), and a DMA mapping failure of the SEND
itself in mlx5e_sq_xmit_wqe(), drop the skb after parameter/DUMP WQEs
were posted for it. Its wmem is then released while those WQEs are
still pending. That is a narrow, pre-existing hole in the
destruct-time argument, independent of rekey. SQ error/reset is
covered: mlx5e_free_txqsq_descs() runs only once the SQ was moved out
of RDY, when the hardware is done with it.

> Was the recycling fence relying on this socket-lifetime ordering, or
> does it independently protect against outstanding work on another SQ?
> c4dfe704f53f ("net/mlx5e: kTLS, Recycle objects of device-offloaded
> TLS TX connections") says the fence ensures context modification does
> not interfere with existing traffic. Does that cover old SEND, DUMP
> and parameter WQEs on another SQ, including work the device has not
> yet executed?
> 

It relies on the socket-lifetime ordering. The fence in c4dfe704f53f
addresses the same-SQ pipelining: the new owner's data must not be
transmitted before the context update has completed, and the update
must not interfere with WQEs in execution on that SQ. It does not, and
cannot, cover WQEs pending on another SQ; those were assumed complete
by the destruct-time contract you describe, or stopped by the channel
teardown. Live rekey via del+add breaks that assumption.

> If reuse must wait, would retaining the TIS and DEK until all
> referencing work has completed or been definitively stopped suffice,
> including during SQ error/reset?
> 

Yes, that is the right condition: every WQE referencing the TIS (or
the DEK) has either completed - a CQE of a later WQE on the same SQ
retires it, as the completions of an SQ are in order - or its SQ was
moved out of RDY and its descriptors were reclaimed in
mlx5e_free_txqsq_descs(), which covers the error/recovery and
teardown paths.

As for how to get there, here is what I have in mind, in three parts.
Feedback is welcome on all of them, and in particular on the third,
as it affects your series.

1. Pre-existing, independent of rekey: the stranded WQEs above.
    Rather than adding completion tracking for the TIS, I'd restore the
    socket-lifetime contract in the driver: an skb dropped after WQEs
    were posted for it is not freed right away, but held by a NOP that
    requests a completion, so that the skb and its wmem are released
    only once all WQEs posted ahead of it are retired (or the SQ is
    reset). mlx5e_ktls_del_tx() stays as is. I intend to send this as a
    fix to net.

2. Pre-existing, and a dependency of rekey: TX queue stability. I'd
    like to address the queue moves above in the stack, opt-in: a
    driver whose TX offload keeps per-connection state updated by queue
    execution declares it with a tlsdev_ops flag, and for sockets
    offloaded to it, the TX queue is re-selected only once
    sk_wmem_alloc has drained. That is, ooo_okay keeps only its wmem
    condition, and while wmem is held, a dst reset does not clear the
    cached queue and txq_reselection_ms does not expire it. With (1), a
    drained wmem means that no work referencing the connection state is
    pending on any SQ. txhash rethink and PLB still fire, and their
    network-level repathing (e.g. the IPv6 flowlabel) is unaffected;
    only the local queue switch waits until the driver drains. Other
    drivers, and non-offloaded sockets, keep today's behavior.

3. Rekey. Releasing the TIS of a live connection to the pool, as
    del+add does, requires that no old-key work is still pending, which
    the driver cannot cheaply tell. I see two directions:

    a. Keep the TIS, and replace the key in place through a new tlsdev
       op (say, tls_dev_rekey()). With (2), the new parameters are
       posted on the same SQ as all old-key WQEs, fenced, so they take
       effect only after them, and the pool is not involved. The driver
       keeps the replaced key until it is no longer in use. For that,
       the op would be called only once all data sent with the current
       key was acknowledged, which, as I understand it, your rekey
       completion already waits for, and which is needed anyway, as the
       device cannot retransmit old-key data after the switch.

    b. Keep del+add, and have the TLS core call tls_dev_del() only once
       sk_wmem_alloc has drained. With (1), that guarantees no pending
       work references the TIS or the DEK. The cost is a potentially
       long wait on a backlogged or paused queue, during which new data
       is held back or sent in SW.

    Tracking SEND completions per connection in the driver would also
    work, but means per-packet accounting for a rare event.

    I lean towards (a). Would it fit your series, or do you see reasons
    to keep del+add?


> I have not reproduced a failure; the reuse is visible in the driver,
> but I have not established the hardware ordering guarantee. A PRM
> reference, or a statement of which completion must precede reuse,
> would settle it.
> 

Thanks,
Tariq

> Thanks,
> Rishikesh
> 
> [1] https://lore.kernel.org/netdev/20260917224355.2288021-1-rjethwani@purestorage.com/


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

* Re: Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs
  2026-11-03 14:45 ` Tariq Toukan
@ 2026-11-04  5:01   ` Rishikesh Jethwani
  2026-11-05  9:25     ` Tariq Toukan
  0 siblings, 1 reply; 9+ messages in thread
From: Rishikesh Jethwani @ 2026-11-04  5:01 UTC (permalink / raw)
  To: Tariq Toukan
  Cc: Saeed Mahameed, Mark Bloch, Gal Pressman, Jakub Kicinski,
	Boris Pismenny, Sabrina Dubroca, John Fastabend, netdev

On Tue, Nov 3, 2026 at 6:45 AM Tariq Toukan <tariqt@nvidia.com> wrote:
>
>
>
> On 22/10/2026 21:17, Rishikesh Jethwani wrote:
> > Hi Tariq,
> >

Hi Tariq,

Thanks, this answers the cross-SQ ordering question. Short version:
I agree with (1), and (3a) fits the series; I prefer it. For (2), I
agree with the goal, but wmem-gated reselection alone does not keep a
connection's work on one SQ in mlx5e; the holes are listed below,
with two ways to close them, and I'd like your preference.

Two hardware questions here, and a third on initial setup under (1):

 - Does a completed DESTROY_SQ, or finding an SQ in ERR, guarantee
   that none of its WQEs can still access a TIS or DEK? And can
   DESTROY_SQ fail while the function is healthy?
   In mlx5e_close_txqsq(), the void mlx5e_destroy_sq() wrapper
   discards the destroy status, and mlx5e_free_txqsq_descs() then
   frees descriptors, advancing sq->cc without CQEs, so the "moved
   out of RDY" argument depends on the answer.
 - DEK release in mlx5_crypto_dek_destroy() has two paths. Without
   a DEK pool (dek_priv == NULL) the object is destroyed right away;
   with one, the DEK is pushed back to the pool, its id can be
   handed to another connection after the batched SYNC_CRYPTO, and
   the object itself is destroyed later from destroy_work. After
   all WQEs referencing a DEK have retired, is either path, including
   reuse of a pooled id by another connection, safe while an idle
   TIS still names that DEK, before replacement static parameters
   are posted? Or is today's release only safe because of the
   destruct-time ordering?

> Hi Rishikesh,
>
> Good analysis, and thanks for the detailed write-up.
>
> > Could a DUMP/SEND WQE from an old owner on SQ A read or update TIS
> > state after SQ B has reinitialised that TIS for a new owner?
> >
>
> Short answer: Yes, in principle it could, once del runs on a live socket
> as in your series. Today, only parameter/DUMP WQEs of a dropped skb can
> be left behind (item 1 below): a pending SEND holds sk_wmem_alloc, so
> the socket is not destructed before it completes.
>
> The TIS is a function-global object, not bound to an SQ: a WQE that
> references its TISN reads/updates its state whenever that WQE executes,
> from whichever SQ it was posted on, and the hardware provides no
> cross-SQ ordering for such accesses. Details below, and at the end a
> proposal for the way forward, on which I'd appreciate your feedback.
>
> > The existing TIS pool predates live rekey. My TLS 1.3 TX KeyUpdate
> > series (v17 [1]; v18 in preparation) calls tls_dev_del() and then
> > tls_dev_add() on a live socket, and the question arises through this
> > sequence:
> >
> >    1. A spurious RTO retransmit of old-key data is posted on SQ A.
> >    2. While that data remains unacknowledged, setsockopt(TLS_TX) starts
> >       rekey and installs the rekey validator, which stops admitting new
> >       plaintext to hardware offload. An ACK covering all remaining
> >       old-key data marks rekey ready before the driver observes the
> >       retransmit's SQ completion. A subsequent sendmsg() enters rekey
> >       completion.
> >    3. Rekey completion calls synchronize_net(), which waits for CPU
> >       transmit paths, not SQ completions, so a WQE on a stalled or
> >       backlogged SQ A can still be pending. mlx5e_ktls_del_tx() then
> >       releases the DEK and immediately calls pool_push(). DEK
> >       reclamation uses batched SYNC_CRYPTO; returning the TIS does not
> >       wait for synchronization covering its old DEK.
> >    4. Before re-adding the offload, TLS allocates the new SW-fallback
> >       AEAD. In that interval another connection's mlx5e_ktls_add_tx()
> >       can pop the returned TIS and use it on SQ B. The same socket can
> >       also reach SQ B on re-add: with the retransmit queue empty, TCP
> >       sets ooo_okay even while the retransmitted skb still holds
> >       sk_wmem_alloc, so queue selection may move the flow.
> >       mlx5e_ktls_handle_tx_skb() posts the new owner's initial
> >       parameters with skip_static_post=false, fence_first_post=false:
> >       static parameters unfenced, progress parameters fenced, then a
> >       fence NOP.
> >
>
> Your reading of the driver is accurate on all points. In particular,
> mlx5e_ktls_del_tx() returns the TIS to the pool with no completion
> tracking. The same goes for the DEK: the batched SYNC_CRYPTO guards
> against stale cached keys when a DEK object is reused, but nothing
> waits for pending WQEs before that, and without bulk DEK support the
> DEK is destroyed right away. So the DEK needs the same lifetime
> guarantee as the TIS.
>
> The same-socket exposure is also broader than the ooo_okay disjunct:
> the RTO and PLB rehash paths reset the dst, which clears the cached
> TX queue, and txq_reselection_ms lets a busy flow re-pick its queue
> periodically, both regardless of ooo_okay. So one connection's WQEs
> can be in flight on two SQs today, even without rekey.
>
> > An unobserved completion does not by itself mean the WQE still accesses
> > the TIS; that is the hardware ordering question. The same question
> > applies on SQ A: resync fences its first parameter update, initial
> > setup does not, and I do not know what either guarantees against
> > earlier WQEs that reference the TIS.
> >
>
> The fence modes in the WQE ctrl segment (FM field; the parameter WQEs
> use Initiator_Small_Fence) are all defined in terms of previous WQEs
> on the same SQ: a fenced WQE waits until earlier WQEs on that SQ are
> no longer in execution. Even the strongest mode (Strong_Ordering)
> orders only against previous WQEs of the same SQ. No fence mode, and
> no other WQE-level primitive, orders against outstanding work on
> another SQ.
>
> So within one SQ, the fenced first parameter WQE of a resync does
> guarantee that earlier WQEs referencing the TIS are done executing
> before the context is modified. The unfenced static parameters of the
> initial setup are safe only because, under the current lifetime
> contract, the previous owner's WQEs are known to be complete by the
> time the TIS leaves the pool. Across SQs, nothing is ordered.
>
> > The normal close path has additional ordering. tls_dev_del() runs
> > from work queued by tls_device_sk_destruct(), which itself runs from
> > __sk_free(), i.e. only once sk_wmem_alloc reaches zero. For
> > transmitted mlx5 skbs, SQ completion calls napi_consume_skb(), which
> > invokes tcp_wfree() to release that accounting, and parameter and DUMP
> > WQEs preceding a posted SEND on the same SQ are retired by that SEND's
> > completion. For successful transmissions, this gives close-time
> > ordering without an explicit drain in del_tx. WQEs posted without a
> > subsequent SEND, and SQ error/reset paths, need separate lifetime
> > guarantees. Live rekey does not wait for socket destruction.
> >
>
> Correct, including the completion detail: parameter and DUMP WQEs do
> not request CQEs of their own, so they are retired only when a later
> CQE-generating WQE on the same SQ completes. And you have a point
> about WQEs posted without a subsequent SEND: the error paths of
> mlx5e_ktls_handle_tx_skb(), and a DMA mapping failure of the SEND
> itself in mlx5e_sq_xmit_wqe(), drop the skb after parameter/DUMP WQEs
> were posted for it. Its wmem is then released while those WQEs are
> still pending. That is a narrow, pre-existing hole in the
> destruct-time argument, independent of rekey. SQ error/reset is
> covered: mlx5e_free_txqsq_descs() runs only once the SQ was moved out
> of RDY, when the hardware is done with it.
>
> > Was the recycling fence relying on this socket-lifetime ordering, or
> > does it independently protect against outstanding work on another SQ?
> > c4dfe704f53f ("net/mlx5e: kTLS, Recycle objects of device-offloaded
> > TLS TX connections") says the fence ensures context modification does
> > not interfere with existing traffic. Does that cover old SEND, DUMP
> > and parameter WQEs on another SQ, including work the device has not
> > yet executed?
> >
>
> It relies on the socket-lifetime ordering. The fence in c4dfe704f53f
> addresses the same-SQ pipelining: the new owner's data must not be
> transmitted before the context update has completed, and the update
> must not interfere with WQEs in execution on that SQ. It does not, and
> cannot, cover WQEs pending on another SQ; those were assumed complete
> by the destruct-time contract you describe, or stopped by the channel
> teardown. Live rekey via del+add breaks that assumption.
>
> > If reuse must wait, would retaining the TIS and DEK until all
> > referencing work has completed or been definitively stopped suffice,
> > including during SQ error/reset?
> >
>
> Yes, that is the right condition: every WQE referencing the TIS (or
> the DEK) has either completed - a CQE of a later WQE on the same SQ
> retires it, as the completions of an SQ are in order - or its SQ was
> moved out of RDY and its descriptors were reclaimed in
> mlx5e_free_txqsq_descs(), which covers the error/recovery and
> teardown paths.
>
> As for how to get there, here is what I have in mind, in three parts.
> Feedback is welcome on all of them, and in particular on the third,
> as it affects your series.
>
> 1. Pre-existing, independent of rekey: the stranded WQEs above.
>     Rather than adding completion tracking for the TIS, I'd restore the
>     socket-lifetime contract in the driver: an skb dropped after WQEs
>     were posted for it is not freed right away, but held by a NOP that
>     requests a completion, so that the skb and its wmem are released
>     only once all WQEs posted ahead of it are retired (or the SQ is
>     reset). mlx5e_ktls_del_tx() stays as is. I intend to send this as a
>     fix to net.

Agreed. Three spots for it: on SEND DMA failure, mlx5e_sq_xmit_wqe() and
mlx5e_sq_xmit_mpwqe() already post a NOP through mlx5e_tx_flush() but
free the skb first, so the skb should be held instead; that NOP asks for
a completion because mlx5e_notify_hw() sets MLX5_WQE_CTRL_CQ_UPDATE, not
mlx5e_post_nop(). mlx5e_ktls_handle_tx_skb()'s err_out needs the same
hold-NOP and a doorbell, since it can leave parameter/DUMP WQEs posted
without one. The MPWQE case is reachable too: when TX MPWQE is enabled,
mlx5e_sq_xmit_mpwqe() takes any linear, untagged, non-GSO skb with
inline mode NONE, which includes a pass-through skb that posted pending
parameters and took SKIP_NO_DATA, since it leaves with no TISN.

One correction to "SQ error/reset is covered": error-CQE recovery never
runs mlx5e_free_txqsq_descs(). mlx5e_tx_reporter_err_cqe_recover()
waits in mlx5e_wait_for_sq_flush() for cc to reach pc, then zeroes both
counters, so on success every WQE has been retired by an error CQE and
the held skb is released normally. If that wait times out, the SQ stays
in ERR with its queue stopped but NAPI still polling, so unretired skbs
stay held until a later completion or close. Timeout recovery is
different: mlx5e_safe_reopen_channels() closes the old SQs and reclaims
their descriptors in mlx5e_free_txqsq_descs(). In all of them the
hold-NOP from (1) eventually gets its skb released; whether the device
has stopped touching the TIS by then is the first question above.

A related recovery gap exists today without rekey: if a connection's
initial static parameters are flushed without executing, recovery does
not re-arm ctx_post_pending. A retransmit of the same record can also
skip static parameters, so software does not guarantee that the new
context gets installed. What does the hardware do with a SEND naming a
TIS whose static parameters never executed, recycled or freshly created?
The rekey contract below handles the corresponding replacement case;
initial setup may need the same treatment.

When do you expect to post (1)? I'll base the series on it and can
test on ConnectX-6 Dx before you post.

> 2. Pre-existing, and a dependency of rekey: TX queue stability. I'd
>     like to address the queue moves above in the stack, opt-in: a
>     driver whose TX offload keeps per-connection state updated by queue
>     execution declares it with a tlsdev_ops flag, and for sockets
>     offloaded to it, the TX queue is re-selected only once
>     sk_wmem_alloc has drained. That is, ooo_okay keeps only its wmem
>     condition, and while wmem is held, a dst reset does not clear the
>     cached queue and txq_reselection_ms does not expire it. With (1), a
>     drained wmem means that no work referencing the connection state is
>     pending on any SQ. txhash rethink and PLB still fire, and their
>     network-level repathing (e.g. the IPv6 flowlabel) is unaffected;
>     only the local queue switch waits until the driver drains. Other
>     drivers, and non-offloaded sockets, keep today's behavior.

Gating ooo_okay, the dst-reset queue clear and txq_reselection_ms
on wmem covers every path through sk_tx_queue_mapping. In mlx5e the
SQ can still change while that mapping stays put, or bypass it:

 - Channel replacement with tis_tir_td_order destroys the old
   channels only after the new ones are active (3b88a535a8e1).
   mlx5e_deactivate_txqsq() posts a final NOP, but nothing waits for
   its CQE, and mlx5e_poll_tx_cq() returns early once ENABLED is
   cleared. The deactivated SQ can still hold old-key WQEs (e.g.
   under PFC pause) while the same txq index already names the new
   SQ, so (3a)'s "same SQ as all old-key WQEs" holds only once the
   old SQ is destroyed, which is the DESTROY_SQ question above.
 - With num_tcs > 1, mlx5e_select_queue() adds a priority offset
   after netdev_pick_tx() (DSCP under MLX5_QPTS_TRUST_DSCP, else VLAN
   prio; HTB by skb->priority), so IP_TOS/IPV6_TCLASS or SO_PRIORITY
   changes move the SQ.
 - Egress skbedit queue_mapping bypasses ndo_select_queue(), and
   SKBEDIT_F_TXQ_SKBHASH can move a flow after a txhash rethink.
 - A queue-count change: netdev_pick_tx() ignores a cached index
   >= real_num_tx_queues whatever wmem says.

Separately, "a drained wmem means that no work referencing the
connection state is pending on any SQ" has a pre-existing hole:
netem with delay, jitter or rate calls skb_orphan_partial(), which
keeps skb->sk but swaps tcp_wfree for sock_efree, releasing the wmem
charge while the skb is parked. can_skb_orphan_partial() refuses
decrypted skbs and anything not owned through sock_wfree/tcp_wfree,
so two kinds of tcp_wfree skb get through:

 - plain TCP data queued before setsockopt(TLS_TX); at dequeue it
   ends before the start marker, and mlx5e_ktls_handle_tx_skb()
   posts the pending parameter WQEs naming the new TIS and takes
   the MLX5E_KTLS_SYNC_SKIP_NO_DATA path;
 - new-key ciphertext the SW context sent during rekey, an ordinary
   tcp_wfree skb not marked decrypted; dequeued after rekey
   completion, it passes through tls_validate_xmit_skb() on the
   device or on a bond master, and ends before the completion's
   marker.

The socket reference held by skb_set_owner_sk_safe() still delays
destruct, so your destruct-time argument in (1) is unaffected; only
the wmem gate in (2) can open early. Both orphan paths release the
charge, but (2) reads a drained charge as no outstanding work; the
partial orphan keeps the reference and drops the charge, so the two
come apart.

Both kinds of skb above can be handled in the driver alone: mlx5e can
skip the context WQEs for an skb with skb->destructor != tcp_wfree (the
test tcp_gso_segment() already uses), which covers the pre-install
plaintext, whose orphan decision netem_enqueue() made before the socket
was offloaded, and the parked rekey ciphertext alike. Both already go
out as they are; the pending parameters just wait for the next skb. No
record data awaiting HW encryption reaches that path: a decrypted skb
is never partially orphaned, and a fully orphaned decrypted one is
dropped in validate_xmit_skb() before any driver sees it. Refusing
partial orphan in can_skb_orphan_partial() would also close the rekey
case, but needs a core-to-TLS hook and still misses the pre-install
one.

So (2) as proposed is a good stability hint, but not proof. I see
two ways to get correctness:

 i. Close the holes individually, without a new per-packet lock: the
    driver check above, keeping the priority offset and skbedit on
    the pinned queue for offloaded sockets, and, while kTLS TX
    connections are live, closing the old channels before the
    switch; mlx5e can tell that today only by subtracting tx_tls_del
    from tx_tls_ctx, so it needs a proper count. That last part
    gives back the downtime 3b88a535a8e1 saved, and still relies on
    your answer to the first question; (i) only moves that
    dependency earlier. It also spreads the invariant across the
    stack and the driver, and a future queue-selection path can
    silently break it.

ii. A driver-side owner check where WQEs are posted: priv_tx
    records its owner SQ as (txq index, generation) and the last WQE
    counter it posted there; a post on another SQ is admitted only
    once the owner's completions cover that counter or its
    generation is retired (by a completed DESTROY_SQ or recovery,
    if the first question above confirms that suffices), and is
    otherwise SW-encrypted with tls_encrypt_skb(), as chcr, nfp and
    funeth do. Completion progress stays per SQ, so this is not
    per-connection completion accounting, but it does need a
    per-connection lock on the kTLS xmit path, which is uncontended
    unless the flow moves. I'd measure it before posting.

I lean towards (ii) for robustness, with (2) as the hint that keeps
it off the slow path, but I'd rather hear your view on the hot-path
cost first.

> 3. Rekey. Releasing the TIS of a live connection to the pool, as
>     del+add does, requires that no old-key work is still pending, which
>     the driver cannot cheaply tell. I see two directions:
>
>     a. Keep the TIS, and replace the key in place through a new tlsdev
>        op (say, tls_dev_rekey()). With (2), the new parameters are
>        posted on the same SQ as all old-key WQEs, fenced, so they take
>        effect only after them, and the pool is not involved. The driver
>        keeps the replaced key until it is no longer in use. For that,
>        the op would be called only once all data sent with the current
>        key was acknowledged, which, as I understand it, your rekey
>        completion already waits for, and which is needed anyway, as the
>        device cannot retransmit old-key data after the switch.
>
>     b. Keep del+add, and have the TLS core call tls_dev_del() only once
>        sk_wmem_alloc has drained. With (1), that guarantees no pending
>        work references the TIS or the DEK. The cost is a potentially
>        long wait on a backlogged or paused queue, during which new data
>        is held back or sent in SW.
>
>     Tracking SEND completions per connection in the driver would also
>     work, but means per-packet accounting for a rare event.
>
>     I lean towards (a). Would it fit your series, or do you see reasons
>     to keep del+add?

(3a) fits, and I prefer it. sendmsg keeps sending SW ciphertext during
rekey, which holds wmem, so (3b) can wait indefinitely unless the
sender stalls. With this series, mlx5e is the only in-tree driver
accepting TLS 1.3 crypto_info, so the core can require the new op for
TX rekey and leave other devices in SW.

Proposed contract:

  int (*tls_dev_rekey)(struct net_device *netdev, struct sock *sk,
                       enum tls_offload_ctx_dir direction,
                       struct tls_crypto_info *crypto_info,
                       u32 start_offload_tcp_sn);

 - direction is kept for symmetry with tls_dev_add()/tls_dev_del();
   only TX uses it for now, RX stays on del+add.
 - Called from tls_device_complete_rekey() in process context, under
   tx_lock, the socket lock and device_offload_lock (read), where
   tls_dev_del() is called today. All old-key records are ACKed and
   synchronize_net() has drained in-progress transmissions; skbs
   still in a qdisc are revalidated at dequeue by the rekey
   validator. crypto_info carries the new key and the live record
   sequence; start_offload_tcp_sn is write_seq.
 - The core builds the new fallback AEAD before the call (today it
   is built between del and add) and commits it, with the record
   marker, before restoring the HW validator. Until then the driver
   sees no new offload plaintext.
 - The driver keeps the TIS, allocates the new DEK and does all
   sleeping firmware work first, then publishes key, expected
   sequence and pending parameters. The re-post covers both static
   and progress parameters, with the fence on the first WQE (unlike
   today's initial post, where the static params go out unfenced).
 - The old DEK is freed from process context once a successful
   CQE covers the fenced replacement, subject to your answer to the
   second question above.
 - Retirement of the SQ only proves the old work stopped, not that
   the replacement took effect. If the SQ errors or closes before
   a successful CQE covers the replacement, the driver forces a
   resync on the next offloaded skb regardless of expected_seq:
   fenced static and progress params for that skb's record, with
   the skip-static shortcut bypassed, and a DUMP of any record
   prefix before the SEND. A plain repost is not enough, since
   expected_seq has already advanced and the next skb can match
   it while starting mid-record.
 - On failure, the old context and key stay intact, nothing is
   posted, and traffic stays in SW until teardown or a later KeyUpdate.

If this direction works for you, I'll write mlx5e_ktls_rekey_tx()
and whichever (2) option you prefer on top of your fix, and test
them on the same hardware.

Thanks,
Rishikesh

>
> > I have not reproduced a failure; the reuse is visible in the driver,
> > but I have not established the hardware ordering guarantee. A PRM
> > reference, or a statement of which completion must precede reuse,
> > would settle it.
> >
>
> Thanks,
> Tariq
>
> > Thanks,
> > Rishikesh
> >
> > [1] https://lore.kernel.org/netdev/20260917224355.2288021-1-rjethwani@purestorage.com/
>

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

* Re: Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs
  2026-11-04  5:01   ` Rishikesh Jethwani
@ 2026-11-05  9:25     ` Tariq Toukan
  2026-11-06 19:58       ` Rishikesh Jethwani
  0 siblings, 1 reply; 9+ messages in thread
From: Tariq Toukan @ 2026-11-05  9:25 UTC (permalink / raw)
  To: Rishikesh Jethwani
  Cc: Saeed Mahameed, Mark Bloch, Gal Pressman, Jakub Kicinski,
	Boris Pismenny, Sabrina Dubroca, John Fastabend, netdev


On 04/11/2026 7:01, Rishikesh Jethwani wrote:
 > Thanks, this answers the cross-SQ ordering question. Short version:
 > I agree with (1), and (3a) fits the series; I prefer it. For (2), I
 > agree with the goal, but wmem-gated reselection alone does not keep a
 > connection's work on one SQ in mlx5e; the holes are listed below,
 > with two ways to close them, and I'd like your preference.

Hi Rishikesh,

Thanks, very thorough again. Short version from my side: I'd go with
(i), closing the holes, rather than (ii), and with (3a) along the lines
of your contract. As the queue moves corrupt records today, regardless
of rekey, I'd send (2) and the mlx5e side of (i) to net as fixes; I
could reproduce the corruption on ConnectX-6 Dx. Details inline.
Feedback is welcome, especially where it touches your series.

 > Two hardware questions here, and a third on initial setup under (1):
 >
 >  - Does a completed DESTROY_SQ, or finding an SQ in ERR, guarantee
 >    that none of its WQEs can still access a TIS or DEK? And can
 >    DESTROY_SQ fail while the function is healthy?
[...]
 >  - DEK release in mlx5_crypto_dek_destroy() has two paths. Without
[...]
 >    After all WQEs referencing a DEK have retired, is either path,
 >    including reuse of a pooled id by another connection, safe while
 >    an idle TIS still names that DEK, before replacement static
 >    parameters are posted? Or is today's release only safe because
 >    of the destruct-time ordering?

Good questions. I'll take them to our HW/FW folks, and get back to you
once I have answers I can back up. Meanwhile, what I propose below
relies only on what the close path already relies on today: a CQE
retires all earlier WQEs of its SQ, and once an SQ is destroyed, none
of its WQEs executes.

 >> 1. Pre-existing, independent of rekey: the stranded WQEs above.
[...]
 > Agreed. Three spots for it: on SEND DMA failure, mlx5e_sq_xmit_wqe() and
 > mlx5e_sq_xmit_mpwqe() already post a NOP through mlx5e_tx_flush() but
 > free the skb first, so the skb should be held instead; that NOP asks for
 > a completion because mlx5e_notify_hw() sets MLX5_WQE_CTRL_CQ_UPDATE, not
 > mlx5e_post_nop(). mlx5e_ktls_handle_tx_skb()'s err_out needs the same
 > hold-NOP and a doorbell, since it can leave parameter/DUMP WQEs posted
 > without one. The MPWQE case is reachable too: when TX MPWQE is enabled,
 > mlx5e_sq_xmit_mpwqe() takes any linear, untagged, non-GSO skb with
 > inline mode NONE, which includes a pass-through skb that posted pending
 > parameters and took SKIP_NO_DATA, since it leaves with no TISN.

Right, the patch covers the same three spots: mlx5e_tx_flush() takes
the skb and attaches it to its NOP, which requests a completion through
mlx5e_notify_hw(), and both SEND error paths hand it the skbs of kTLS
sockets, MPWQE included.

 > One correction to "SQ error/reset is covered": error-CQE recovery never
 > runs mlx5e_free_txqsq_descs(). mlx5e_tx_reporter_err_cqe_recover()
 > waits in mlx5e_wait_for_sq_flush() for cc to reach pc, then zeroes both
 > counters, so on success every WQE has been retired by an error CQE and
 > the held skb is released normally.
[...]

Correct, thanks.

 > A related recovery gap exists today without rekey: if a connection's
 > initial static parameters are flushed without executing, recovery does
 > not re-arm ctx_post_pending. A retransmit of the same record can also
 > skip static parameters, so software does not guarantee that the new
 > context gets installed. What does the hardware do with a SEND naming a
 > TIS whose static parameters never executed, recycled or freshly created?
 > The rekey contract below handles the corresponding replacement case;
 > initial setup may need the same treatment.

Agreed, and it is wider than the static params: any lost WQE leaves the
TIS state off from what the driver tracks. WQEs are lost when an skb is
dropped after its kTLS WQEs were posted (or after expected_seq advanced
past it), when an SQ error flushes them, and when an SQ is closed with
WQEs pending. The driver holds the skb in each of these, so I'd mark
its connection right there, and have the next skb of a marked
connection resync fully: static params regardless of rec_seq, progress
params, and a DUMP of the record prefix. That covers initial setup, and
the replacement case of your contract, with no rekey-specific code. I'd
send it to net together with (1).

 > When do you expect to post (1)? I'll base the series on it and can
 > test on ConnectX-6 Dx before you post.

After internal review and testing; I can share the patches as RFCs in
the meantime. (1) goes to net together with the resync fix above and
the fixes for (2) below: the stack part, the mlx5e queue pin, the
channel replacement overlap, and the pass-through skbs. I tested them
on ConnectX-6 Dx, and testing on your side is very welcome, especially
with your series on top.

 >> 2. Pre-existing, and a dependency of rekey: TX queue stability. I'd
[...]
 > Gating ooo_okay, the dst-reset queue clear and txq_reselection_ms
 > on wmem covers every path through sk_tx_queue_mapping. In mlx5e the
 > SQ can still change while that mapping stays put, or bypass it:
 >
 >  - Channel replacement with tis_tir_td_order destroys the old
 >    channels only after the new ones are active (3b88a535a8e1).
 >    mlx5e_deactivate_txqsq() posts a final NOP, but nothing waits for
 >    its CQE, and mlx5e_poll_tx_cq() returns early once ENABLED is
 >    cleared. The deactivated SQ can still hold old-key WQEs (e.g.
 >    under PFC pause) while the same txq index already names the new
 >    SQ, so (3a)'s "same SQ as all old-key WQEs" holds only once the
 >    old SQ is destroyed, which is the DESTROY_SQ question above.

Right, and that overlap is a bug of 3b88a535a8e1 on its own. I'd fix it
by draining the old SQs before activating the new channels: their NAPI
is already disabled, so the switch reaps their completions directly,
and the old channels are still closed after the activation, which keeps
the downtime saving. If they don't drain in time (e.g. under a long
pause), it warns and falls back to closing them first. No kTLS-specific
count is needed.

 >  - With num_tcs > 1, mlx5e_select_queue() adds a priority offset
 >    after netdev_pick_tx() (DSCP under MLX5_QPTS_TRUST_DSCP, else VLAN
 >    prio; HTB by skb->priority), so IP_TOS/IPV6_TCLASS or SO_PRIORITY
 >    changes move the SQ.

mlx5e_select_queue() can keep the final queue of such sockets: cache it
in sk_tx_queue_mapping, and reuse it as long as ooo_okay is clear. A
priority change then takes effect once the connection has nothing in
flight, which for a busy bulk flow can take a while; that's the price
of keeping the order.

 >  - Egress skbedit queue_mapping bypasses ndo_select_queue(), and
 >    SKBEDIT_F_TXQ_SKBHASH can move a flow after a txhash rethink.

I'd document it as not supported with this kind of offload, and have
the driver count it (see below).

 >  - A queue-count change: netdev_pick_tx() ignores a cached index
 >    >= real_num_tx_queues whatever wmem says.

In mlx5e that comes only with a channel replacement, which the drain
covers, or with an HTB leaf deletion, which closes the leaf SQ before
its queue is removed.

 > Separately, "a drained wmem means that no work referencing the
 > connection state is pending on any SQ" has a pre-existing hole:
 > netem with delay, jitter or rate calls skb_orphan_partial(), which
 > keeps skb->sk but swaps tcp_wfree for sock_efree, releasing the wmem
 > charge while the skb is parked.
[...]
 > Both kinds of skb above can be handled in the driver alone: mlx5e can
 > skip the context WQEs for an skb with skb->destructor != tcp_wfree (the
 > test tcp_gso_segment() already uses), which covers the pre-install
 > plaintext, whose orphan decision netem_enqueue() made before the socket
 > was offloaded, and the parked rekey ciphertext alike.
[...]

Agreed that it belongs in the driver. Rather than keying on the
destructor, I wrote it so that nothing is posted for an skb that
doesn't use the context: the pending params are posted only once the
skb is known to need them (in order, or resynced, where they fold into
the full resync), and stay pending on SKIP_NO_DATA. A pass-through skb
then touches nothing, wherever it was parked. Does that cover both of
your cases?

 > So (2) as proposed is a good stability hint, but not proof. I see
 > two ways to get correctness:
 >
 >  i. Close the holes individually, without a new per-packet lock: the
[...]
 > ii. A driver-side owner check where WQEs are posted: priv_tx
[...]
 > I lean towards (ii) for robustness, with (2) as the hint that keeps
 > it off the slow path, but I'd rather hear your view on the hot-path
 > cost first.

I'd prefer (i). With the drain, it no longer needs to close channels
first or to count connections, and the rest is small and local. (ii)
adds a lock and a SW encryption fallback to the mlx5e xmit path, for a
case that shouldn't happen once the holes are closed; a fallback that
hardly ever runs is also hard to keep correct.

And as these moves corrupt records today, without rekey, I'd send (2)
and the mlx5e queue pin to net as fixes. On ConnectX-6 Dx with many
channels, kTLS TX records already fail decryption at the receiver with
the default txq_reselection_ms, and forcing moves breaks most
connections; with the net fixes, the same runs are clean.

As for a future path silently breaking the invariant, the driver can
catch that cheaply: it remembers the SQ that a connection last posted
on, and when the next skb arrives on another SQ while ooo_okay is clear
and the old SQ is still live, it bumps a counter and warns once. That
is a pointer compare per skb, and it turns a silent corruption into a
visible one.

 > (3a) fits, and I prefer it. sendmsg keeps sending SW ciphertext during
 > rekey, which holds wmem, so (3b) can wait indefinitely unless the
 > sender stalls. With this series, mlx5e is the only in-tree driver
 > accepting TLS 1.3 crypto_info, so the core can require the new op for
 > TX rekey and leave other devices in SW.
 >
 > Proposed contract:
 >
 >   int (*tls_dev_rekey)(struct net_device *netdev, struct sock *sk,
 >                        enum tls_offload_ctx_dir direction,
 >                        struct tls_crypto_info *crypto_info,
 >                        u32 start_offload_tcp_sn);

Looks good to me.

 >  - The old DEK is freed from process context once a successful
 >    CQE covers the fenced replacement, subject to your answer to the
 >    second question above.

Agreed, and it's simpler than I first thought: the driver doesn't need
to track completions per connection. When it posts the fenced
replacement, it can hand the old DEK to that WQE's descriptor, the way
DUMP WQEs hold their page refs today, and release it once the SQ
retires that WQE. Completions being in order, that covers every old-key
WQE posted before it on the SQ. A flush or an SQ close retires it too;
the TIS may then still name the old DEK until the resync installs the
new one, which is your second question again, and the same as an idle
TIS in the pool today. The release runs from NAPI, so the destroy
itself would be deferred to a work item.

 >  - Retirement of the SQ only proves the old work stopped, not that
 >    the replacement took effect. If the SQ errors or closes before
 >    a successful CQE covers the replacement, the driver forces a
 >    resync on the next offloaded skb regardless of expected_seq:
[...]

That's the resync fix above, which isn't specific to rekey.

One question on the core side: once the HW validator is restored, can
a decrypted old-key skb still be queued, e.g. a spurious retransmit
parked in a qdisc, and reach the driver? It would end before the new
marker, so mlx5e would take SKIP_NO_DATA with a decrypted skb, which
today hits WARN_ON_ONCE() and drops it.

 > If this direction works for you, I'll write mlx5e_ktls_rekey_tx()
 > and whichever (2) option you prefer on top of your fix, and test
 > them on the same hardware.

Sounds good. (2) is covered by the net fixes, so please go ahead with
the op and mlx5e_ktls_rekey_tx() on top of them, and I'll review. The
RFCs mentioned above should let you start before the fixes land.

Thanks,
Tariq

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

* Re: Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs
  2026-11-05  9:25     ` Tariq Toukan
@ 2026-11-06 19:58       ` Rishikesh Jethwani
  2026-11-08 19:45         ` Tariq Toukan
  0 siblings, 1 reply; 9+ messages in thread
From: Rishikesh Jethwani @ 2026-11-06 19:58 UTC (permalink / raw)
  To: Tariq Toukan
  Cc: Saeed Mahameed, Mark Bloch, Gal Pressman, Jakub Kicinski,
	Boris Pismenny, Sabrina Dubroca, John Fastabend, netdev

On Thu, Nov 5, 2026 at 1:26 AM Tariq Toukan <tariqt@nvidia.com> wrote:
>
>
> On 04/11/2026 7:01, Rishikesh Jethwani wrote:
>  > Thanks, this answers the cross-SQ ordering question. Short version:
>  > I agree with (1), and (3a) fits the series; I prefer it. For (2), I
>  > agree with the goal, but wmem-gated reselection alone does not keep a
>  > connection's work on one SQ in mlx5e; the holes are listed below,
>  > with two ways to close them, and I'd like your preference.
>
> Hi Rishikesh,
Hi Tariq,

Thanks. Agreed on (3a), (i) and the net fixes, with one condition on
(i) below. Answers to your two questions are below, plus what rekey
needs from the fixes before I build on them.

> Thanks, very thorough again. Short version from my side: I'd go with
> (i), closing the holes, rather than (ii), and with (3a) along the lines
> of your contract. As the queue moves corrupt records today, regardless
> of rekey, I'd send (2) and the mlx5e side of (i) to net as fixes; I
> could reproduce the corruption on ConnectX-6 Dx. Details inline.
> Feedback is welcome, especially where it touches your series.
>
>  > Two hardware questions here, and a third on initial setup under (1):
>  >
>  >  - Does a completed DESTROY_SQ, or finding an SQ in ERR, guarantee
>  >    that none of its WQEs can still access a TIS or DEK? And can
>  >    DESTROY_SQ fail while the function is healthy?
> [...]
>  >  - DEK release in mlx5_crypto_dek_destroy() has two paths. Without
> [...]
>  >    After all WQEs referencing a DEK have retired, is either path,
>  >    including reuse of a pooled id by another connection, safe while
>  >    an idle TIS still names that DEK, before replacement static
>  >    parameters are posted? Or is today's release only safe because
>  >    of the destruct-time ordering?
>
> Good questions. I'll take them to our HW/FW folks, and get back to you
> once I have answers I can back up. Meanwhile, what I propose below
> relies only on what the close path already relies on today: a CQE
> retires all earlier WQEs of its SQ, and once an SQ is destroyed, none
> of its WQEs executes.

Thanks. Until then, the two hardware questions remain open, including
whether DESTROY_SQ can fail; the destroy wrapper discards the status.
One correction to my last mail: recovery clears RECOVERING even on a
flush timeout, so the queue can wake once stop_room frees.

>  >> 1. Pre-existing, independent of rekey: the stranded WQEs above.
> [...]
>  > Agreed. Three spots for it: on SEND DMA failure, mlx5e_sq_xmit_wqe() and
>  > mlx5e_sq_xmit_mpwqe() already post a NOP through mlx5e_tx_flush() but
>  > free the skb first, so the skb should be held instead; that NOP asks for
>  > a completion because mlx5e_notify_hw() sets MLX5_WQE_CTRL_CQ_UPDATE, not
>  > mlx5e_post_nop(). mlx5e_ktls_handle_tx_skb()'s err_out needs the same
>  > hold-NOP and a doorbell, since it can leave parameter/DUMP WQEs posted
>  > without one. The MPWQE case is reachable too: when TX MPWQE is enabled,
>  > mlx5e_sq_xmit_mpwqe() takes any linear, untagged, non-GSO skb with
>  > inline mode NONE, which includes a pass-through skb that posted pending
>  > parameters and took SKIP_NO_DATA, since it leaves with no TISN.
>
> Right, the patch covers the same three spots: mlx5e_tx_flush() takes
> the skb and attaches it to its NOP, which requests a completion through
> mlx5e_notify_hw(), and both SEND error paths hand it the skbs of kTLS
> sockets, MPWQE included.
>
>  > One correction to "SQ error/reset is covered": error-CQE recovery never
>  > runs mlx5e_free_txqsq_descs(). mlx5e_tx_reporter_err_cqe_recover()
>  > waits in mlx5e_wait_for_sq_flush() for cc to reach pc, then zeroes both
>  > counters, so on success every WQE has been retired by an error CQE and
>  > the held skb is released normally.
> [...]
>
> Correct, thanks.
>
>  > A related recovery gap exists today without rekey: if a connection's
>  > initial static parameters are flushed without executing, recovery does
>  > not re-arm ctx_post_pending. A retransmit of the same record can also
>  > skip static parameters, so software does not guarantee that the new
>  > context gets installed. What does the hardware do with a SEND naming a
>  > TIS whose static parameters never executed, recycled or freshly created?
>  > The rekey contract below handles the corresponding replacement case;
>  > initial setup may need the same treatment.
>
> Agreed, and it is wider than the static params: any lost WQE leaves the
> TIS state off from what the driver tracks. WQEs are lost when an skb is
> dropped after its kTLS WQEs were posted (or after expected_seq advanced
> past it), when an SQ error flushes them, and when an SQ is closed with
> WQEs pending. The driver holds the skb in each of these, so I'd mark
> its connection right there, and have the next skb of a marked
> connection resync fully: static params regardless of rec_seq, progress
> params, and a DUMP of the record prefix. That covers initial setup, and
> the replacement case of your contract, with no rekey-specific code. I'd
> send it to net together with (1).

Agreed. What rekey needs from that resync fix: a lost initialization
or replacement always triggers a full resync on the next skb that
needs the TLS context, and a connection marked again while the driver
is consuming an earlier mark stays marked (test-and-clear, not
clear-after).

>  > When do you expect to post (1)? I'll base the series on it and can
>  > test on ConnectX-6 Dx before you post.
>
> After internal review and testing; I can share the patches as RFCs in
> the meantime. (1) goes to net together with the resync fix above and
> the fixes for (2) below: the stack part, the mlx5e queue pin, the
> channel replacement overlap, and the pass-through skbs. I tested them
> on ConnectX-6 Dx, and testing on your side is very welcome, especially
> with your series on top.
>
>  >> 2. Pre-existing, and a dependency of rekey: TX queue stability. I'd
> [...]
>  > Gating ooo_okay, the dst-reset queue clear and txq_reselection_ms
>  > on wmem covers every path through sk_tx_queue_mapping. In mlx5e the
>  > SQ can still change while that mapping stays put, or bypass it:
>  >
>  >  - Channel replacement with tis_tir_td_order destroys the old
>  >    channels only after the new ones are active (3b88a535a8e1).
>  >    mlx5e_deactivate_txqsq() posts a final NOP, but nothing waits for
>  >    its CQE, and mlx5e_poll_tx_cq() returns early once ENABLED is
>  >    cleared. The deactivated SQ can still hold old-key WQEs (e.g.
>  >    under PFC pause) while the same txq index already names the new
>  >    SQ, so (3a)'s "same SQ as all old-key WQEs" holds only once the
>  >    old SQ is destroyed, which is the DESTROY_SQ question above.
>
> Right, and that overlap is a bug of 3b88a535a8e1 on its own. I'd fix it
> by draining the old SQs before activating the new channels: their NAPI
> is already disabled, so the switch reaps their completions directly,
> and the old channels are still closed after the activation, which keeps
> the downtime saving. If they don't drain in time (e.g. under a long
> pause), it warns and falls back to closing them first. No kTLS-specific
> count is needed.
>
>  >  - With num_tcs > 1, mlx5e_select_queue() adds a priority offset
>  >    after netdev_pick_tx() (DSCP under MLX5_QPTS_TRUST_DSCP, else VLAN
>  >    prio; HTB by skb->priority), so IP_TOS/IPV6_TCLASS or SO_PRIORITY
>  >    changes move the SQ.
>
> mlx5e_select_queue() can keep the final queue of such sockets: cache it
> in sk_tx_queue_mapping, and reuse it as long as ooo_okay is clear. A
> priority change then takes effect once the connection has nothing in
> flight, which for a busy bulk flow can take a while; that's the price
> of keeping the order.
>
>  >  - Egress skbedit queue_mapping bypasses ndo_select_queue(), and
>  >    SKBEDIT_F_TXQ_SKBHASH can move a flow after a txhash rethink.
>
> I'd document it as not supported with this kind of offload, and have
> the driver count it (see below).
>
>  >  - A queue-count change: netdev_pick_tx() ignores a cached index
>  >    >= real_num_tx_queues whatever wmem says.
>
> In mlx5e that comes only with a channel replacement, which the drain
> covers, or with an HTB leaf deletion, which closes the leaf SQ before
> its queue is removed.
>
>  > Separately, "a drained wmem means that no work referencing the
>  > connection state is pending on any SQ" has a pre-existing hole:
>  > netem with delay, jitter or rate calls skb_orphan_partial(), which
>  > keeps skb->sk but swaps tcp_wfree for sock_efree, releasing the wmem
>  > charge while the skb is parked.
> [...]
>  > Both kinds of skb above can be handled in the driver alone: mlx5e can
>  > skip the context WQEs for an skb with skb->destructor != tcp_wfree (the
>  > test tcp_gso_segment() already uses), which covers the pre-install
>  > plaintext, whose orphan decision netem_enqueue() made before the socket
>  > was offloaded, and the parked rekey ciphertext alike.
> [...]
>
> Agreed that it belongs in the driver. Rather than keying on the
> destructor, I wrote it so that nothing is posted for an skb that
> doesn't use the context: the pending params are posted only once the
> skb is known to need them (in order, or resynced, where they fold into
> the full resync), and stay pending on SKIP_NO_DATA. A pass-through skb
> then touches nothing, wherever it was parked. Does that cover both of
> your cases?

Within the sequence window, yes. Outside it, a partially orphaned
pass-through skb can be held past half the sequence space and alias
expected_seq or a live record. An explicit bypass right after the
datalen check in mlx5e_ktls_handle_tx_skb() closes that without a
sequence comparison:

        if (!skb_is_decrypted(skb))
                return true;

All kTLS HW data is sent with MSG_SENDPAGE_DECRYPTED, and decrypted
skbs can't be partially orphaned, so every skb that reaches the
context then holds wmem.

>  > So (2) as proposed is a good stability hint, but not proof. I see
>  > two ways to get correctness:
>  >
>  >  i. Close the holes individually, without a new per-packet lock: the
> [...]
>  > ii. A driver-side owner check where WQEs are posted: priv_tx
> [...]
>  > I lean towards (ii) for robustness, with (2) as the hint that keeps
>  > it off the slow path, but I'd rather hear your view on the hot-path
>  > cost first.
>
> I'd prefer (i). With the drain, it no longer needs to close channels
> first or to count connections, and the rest is small and local. (ii)
> adds a lock and a SW encryption fallback to the mlx5e xmit path, for a
> case that shouldn't happen once the holes are closed; a fallback that
> hardly ever runs is also hard to keep correct.
>
> And as these moves corrupt records today, without rekey, I'd send (2)
> and the mlx5e queue pin to net as fixes. On ConnectX-6 Dx with many
> channels, kTLS TX records already fail decryption at the receiver with
> the default txq_reselection_ms, and forcing moves breaks most
> connections; with the net fixes, the same runs are clean.
>
> As for a future path silently breaking the invariant, the driver can
> catch that cheaply: it remembers the SQ that a connection last posted
> on, and when the next skb arrives on another SQ while ooo_okay is clear
> and the old SQ is still live, it bumps a counter and warns once. That
> is a pointer compare per skb, and it turns a silent corruption into a
> visible one.

I'm happy to use (i), but warning/counting alone does not cover the
forced-queue case: a detected conflict has to prevent the conflicting
hardware access, since old TIS accesses must have retired before
another SQ can modify the context. Dropping before any TLS WQE or
context-state change would preserve safety without SW crypto. TCP
can recover unacknowledged data through retransmission; conflicting
retransmits keep being dropped until the old accesses have retired.
I'll leave the mechanism to your patches. Points they need to cover:

1. Egress skbedit never calls ndo_select_queue(), so a counter and a
   warning still let conflicting WQEs reach two SQs.

2. sk_tx_queue_mapping is shared by every device on the route: if
   the route passes through another device for a while, its
   netdev_pick_tx() can overwrite the cached queue, and back on mlx5e
   the flow moves while the old SQ still has work. ooo_okay alone
   isn't proof either: TCP also sets it when the rtx queue is empty,
   and on a bond the stamp can be stale by the time the lower device
   selects. bond_select_queue() ignores sk_tx_queue_mapping, and the
   slave's netdev_pick_tx() runs only after the bond's qdisc dequeues,
   so an ooo_okay skb that was overtaken (e.g. by a later skb in a
   higher prio band after an SO_PRIORITY change) can re-pick while
   that later skb is still outstanding on the old SQ. For this bond
   example, re-check wmem at selection: allow the move only if
   sk_wmem_alloc_get(sk) <= skb->truesize while the skb still holds
   its own charge, and only at zero wmem for an uncharged one.
   Across transmit paths, the migration check and queue publication
   also need serialization.

3. The drain has to cover the QoS/HTB SQs too, and reap deactivated
   SQs although mlx5e_poll_tx_cq() returns early once ENABLED is
   clear; a missing or replaced txq2sq entry isn't evidence of
   retirement. On a drain timeout, the fallback must close the old
   SQs first (which keeps the guarantee, modulo DESTROY_SQ), never
   activate with them alive.

On the price you mention for priority changes, agreed. Please
document in Documentation/networking/tls-offload.rst that pinning
delays priority/DSCP/HTB-driven TX queue changes until migration is
safe, potentially indefinitely under sustained traffic.

>  > (3a) fits, and I prefer it. sendmsg keeps sending SW ciphertext during
>  > rekey, which holds wmem, so (3b) can wait indefinitely unless the
>  > sender stalls. With this series, mlx5e is the only in-tree driver
>  > accepting TLS 1.3 crypto_info, so the core can require the new op for
>  > TX rekey and leave other devices in SW.
>  >
>  > Proposed contract:
>  >
>  >   int (*tls_dev_rekey)(struct net_device *netdev, struct sock *sk,
>  >                        enum tls_offload_ctx_dir direction,
>  >                        struct tls_crypto_info *crypto_info,
>  >                        u32 start_offload_tcp_sn);
>
> Looks good to me.
>
>  >  - The old DEK is freed from process context once a successful
>  >    CQE covers the fenced replacement, subject to your answer to the
>  >    second question above.
>
> Agreed, and it's simpler than I first thought: the driver doesn't need
> to track completions per connection. When it posts the fenced
> replacement, it can hand the old DEK to that WQE's descriptor, the way
> DUMP WQEs hold their page refs today, and release it once the SQ
> retires that WQE. Completions being in order, that covers every old-key
> WQE posted before it on the SQ. A flush or an SQ close retires it too;
> the TIS may then still name the old DEK until the resync installs the
> new one, which is your second question again, and the same as an idle
> TIS in the pool today. The release runs from NAPI, so the destroy
> itself would be deferred to a work item.

Nice, that's simpler. On the core side, the op's contract: the
callback prepares the new key and pending parameters but posts no TLS
WQEs for this connection. After it returns success, the core commits
the new fallback AEAD and start marker, then restores the HW
validator, as v18's del+add path already does. Replacement parameters
are posted when the next skb needing the TLS context reaches the
driver. On failure, the callback releases its new allocations and
preserves the existing context for outstanding work and eventual
tls_dev_del(); the core commits nothing and TX stays in software.

Callbacks are serialized per connection, but another may arrive
before the previous replacement is posted. On success, the driver
must clean up any superseded, never-posted key.

On the driver side, the DEK lifetime requirement: the old DEK stays
alive until a completion proves old-key work has retired; a successful
CQE covering the fenced replacement does that. Whether a flush, an SQ
close, or deletion back to the TIS pool may release a DEK the TIS
still names depends on your hardware answers; if not, retention has to
extend through the pool, which is pre-existing and not rekey specific.

>  >  - Retirement of the SQ only proves the old work stopped, not that
>  >    the replacement took effect. If the SQ errors or closes before
>  >    a successful CQE covers the replacement, the driver forces a
>  >    resync on the next offloaded skb regardless of expected_seq:
> [...]
>
> That's the resync fix above, which isn't specific to rekey.
>
> One question on the core side: once the HW validator is restored, can
> a decrypted old-key skb still be queued, e.g. a spurious retransmit
> parked in a qdisc, and reach the driver? It would end before the new
> marker, so mlx5e would take SKIP_NO_DATA with a decrypted skb, which
> today hits WARN_ON_ONCE() and drops it.

It can be queued, but within the sequence window the core drops it
first, in two v18 patches that aren't upstream yet. "tls: device: add
TX KeyUpdate support" sets TLS_TX_REKEY_FLOOR before restoring
tls_validate_xmit_skb(), which then drops any payload skb whose
end_seq is not after snd_una; completion runs only once every old-key
record is ACKed. "net: sched: re-validate decrypted skbs on requeue"
makes an skb parked on the requeue list go through the current
validator again.

Outside that window, the floor does not reliably identify old-key
clones. Once snd_una advances at least 2^31 bytes past a parked
clone's end_seq, the comparison can reverse; that progress can begin
before rekey completion. Later traffic must overtake the parked
clone, for example if a classful qdisc lets a newly selected priority
class run ahead. Record lookup also uses 32-bit TCP sequence numbers
today without rekey, so a sufficiently delayed clone can alias a live
record there too. The floor catches old-key clones within its
comparison window, but does not remove that ambiguity. I'll look at
closing this in the core separately.

>  > If this direction works for you, I'll write mlx5e_ktls_rekey_tx()
>  > and whichever (2) option you prefer on top of your fix, and test
>  > them on the same hardware.
>
> Sounds good. (2) is covered by the net fixes, so please go ahead with
> the op and mlx5e_ktls_rekey_tx() on top of them, and I'll review. The
> RFCs mentioned above should let you start before the fixes land.

Please Cc me on the RFCs, and let me know which fixes the rekey
patches should depend on. I'll implement the core op, its contract,
the v18 integration and mlx5e_ktls_rekey_tx() on top of them, and post
the patches together as an RFC for your review. I'll keep the
flush/close DEK release and pool-lifetime questions explicit until
your HW/FW answers settle them. Devices without the callback keep TX
in software after a KeyUpdate.

> Thanks,
> Tariq

Thanks,
Rishikesh

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

* Re: Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs
  2026-11-06 19:58       ` Rishikesh Jethwani
@ 2026-11-08 19:45         ` Tariq Toukan
  2026-11-09  9:21           ` Tariq Toukan
  2026-11-09 23:21           ` Rishikesh Jethwani
  0 siblings, 2 replies; 9+ messages in thread
From: Tariq Toukan @ 2026-11-08 19:45 UTC (permalink / raw)
  To: Rishikesh Jethwani
  Cc: Saeed Mahameed, Mark Bloch, Gal Pressman, Jakub Kicinski,
	Boris Pismenny, Sabrina Dubroca, John Fastabend, netdev


On 06/11/2026 21:58, Rishikesh Jethwani wrote:
 > Thanks. Agreed on (3a), (i) and the net fixes, with one condition on
 > (i) below.

Hi Rishikesh,

Thanks. I took your points into the patches, details inline. I'm
re-running the ConnectX-6 Dx tests with the changes, and will share the
RFCs once they pass.

 > Thanks. Until then, the two hardware questions remain open, including
 > whether DESTROY_SQ can fail; the destroy wrapper discards the status.

DESTROY_SQ doesn't fail while the function is healthy. If it does,
something went badly wrong in the firmware.

 > One correction to my last mail: recovery clears RECOVERING even on a
 > flush timeout, so the queue can wake once stop_room frees.

Thanks. WQEs posted to that SQ then complete with error CQEs, which mark
their connections for a full resync, as in the first error.

 > Agreed. What rekey needs from that resync fix: a lost initialization
 > or replacement always triggers a full resync on the next skb that
 > needs the TLS context, and a connection marked again while the driver
 > is consuming an earlier mark stays marked (test-and-clear, not
 > clear-after).

Both hold now. The mark stays until an skb that uses the context
consumes it, and it is taken with a test-and-clear before the resync is
posted, so a loss reported meanwhile marks the connection again.

 > An explicit bypass right after the
 > datalen check in mlx5e_ktls_handle_tx_skb() closes that without a
 > sequence comparison:
 >
 >         if (!skb_is_decrypted(skb))
 >                 return true;

Agreed, it's in the pass-through patch, with credit.

 > I'm happy to use (i), but warning/counting alone does not cover the
 > forced-queue case: a detected conflict has to prevent the conflicting
 > hardware access, since old TIS accesses must have retired before
 > another SQ can modify the context. Dropping before any TLS WQE or
 > context-state change would preserve safety without SW crypto.

Agreed, the counter is now a drop. Each connection records the TX queue
that it posts on. An skb on another TX queue moves the connection only
if no other skb of its socket is in flight: an skb is freed only once
the WQEs posted for it completed, or were destroyed with their SQ, so
nothing of the connection is then pending on any SQ. Otherwise the skb
is dropped before anything is posted, and counted, and TCP retransmits
it.

The move is made under the old TX queue's lock, taken with a trylock,
under which the connection posts there. That serializes the check and
the publication with that path, and leaves the connection's datapath
with a compare of the queue index.

This covers your points 1 and 2 where the WQEs are posted, wherever the
queue came from. For 2, the mlx5e pin now also re-checks wmem at
selection as you suggest, so a stale ooo_okay doesn't lead to drops in
the first place. Does that match what you had in mind?

 > 3. The drain has to cover the QoS/HTB SQs too, and reap deactivated
 >    SQs although mlx5e_poll_tx_cq() returns early once ENABLED is
 >    clear; a missing or replaced txq2sq entry isn't evidence of
 >    retirement. On a drain timeout, the fallback must close the old
 >    SQs first (which keeps the guarantee, modulo DESTROY_SQ), never
 >    activate with them alive.

That's how the drain works: it covers the QoS SQs, reaps the deactivated
SQs directly, and on a timeout closes the old channels before activating
the new ones. The drop above doesn't rely on txq2sq either: an SQ that
is deactivated alone, e.g. on an HTB leaf delete, keeps its skbs until
it is destroyed, so their socket keeps its wmem until then.

 > On the price you mention for priority changes, agreed. Please
 > document in Documentation/networking/tls-offload.rst that pinning
 > delays priority/DSCP/HTB-driven TX queue changes until migration is
 > safe, potentially indefinitely under sustained traffic.

Done.

 > Nice, that's simpler. On the core side, the op's contract: the
 > callback prepares the new key and pending parameters but posts no TLS
 > WQEs for this connection.

[...]

 > Callbacks are serialized per connection, but another may arrive
 > before the previous replacement is posted. On success, the driver
 > must clean up any superseded, never-posted key.

Agreed on the contract.

 > Record lookup also uses 32-bit TCP sequence numbers
 > today without rekey, so a sufficiently delayed clone can alias a live
 > record there too. The floor catches old-key clones within its
 > comparison window, but does not remove that ambiguity. I'll look at
 > closing this in the core separately.

Thanks. On the driver side, the bypass above keeps plaintext skbs out of
the record lookup, so it's down to the decrypted clones.

 > Please Cc me on the RFCs, and let me know which fixes the rekey
 > patches should depend on.

Will do. The rekey patches should go on top of these net fixes:

   net/mlx5e: kTLS, Hold a dropped skb until its posted WQEs complete
   net/mlx5e: kTLS, Resync TX fully after its WQEs were lost
   net/mlx5e: kTLS, Post TX context params only for skbs that use them
   net/mlx5e: Drain old TX SQs before activating new channels
   net: tls: keep the TX queue of a device-offloaded socket stable
   net/mlx5e: kTLS, Keep the TX queue of an offloaded socket stable

The drop goes to net-next separately, as "net/mlx5e: kTLS, Keep a TX
connection on one SQ at a time"; the rekey patches don't depend on it.

Thanks,
Tariq

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

* Re: Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs
  2026-11-08 19:45         ` Tariq Toukan
@ 2026-11-09  9:21           ` Tariq Toukan
  2026-11-09 23:21           ` Rishikesh Jethwani
  1 sibling, 0 replies; 9+ messages in thread
From: Tariq Toukan @ 2026-11-09  9:21 UTC (permalink / raw)
  To: Rishikesh Jethwani
  Cc: Saeed Mahameed, Mark Bloch, Gal Pressman, Jakub Kicinski,
	Boris Pismenny, Sabrina Dubroca, John Fastabend, netdev


On 08/11/2026 21:45, Tariq Toukan wrote:
 > I'm re-running the ConnectX-6 Dx tests with the changes, and will
 > share the RFCs once they pass.

They pass, with one change to the drop below.

 > An skb on another TX queue moves the connection only if no other skb
 > of its socket is in flight

This also dropped on legitimate stack moves: the next skbs, queued
behind the first one on the new TX queue, hold sk_wmem_alloc when it
reaches the driver. So a move that the TX queue selection allows, only
while nothing else of the socket is in flight, now hands over the new
TX queue right there, and the check at xmit is left for skbs that
bypass the selection, like tc skbedit. No drops without forced moves
now.

I'll share my patches shortly.

Thanks,
Tariq

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

* Re: Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs
  2026-11-08 19:45         ` Tariq Toukan
  2026-11-09  9:21           ` Tariq Toukan
@ 2026-11-09 23:21           ` Rishikesh Jethwani
  2026-11-10 14:45             ` Tariq Toukan
  1 sibling, 1 reply; 9+ messages in thread
From: Rishikesh Jethwani @ 2026-11-09 23:21 UTC (permalink / raw)
  To: Tariq Toukan
  Cc: Saeed Mahameed, Mark Bloch, Gal Pressman, Jakub Kicinski,
	Boris Pismenny, Sabrina Dubroca, John Fastabend, netdev

On Sun, Nov 8, 2026 at 11:45 AM Tariq Toukan <tariqt@nvidia.com> wrote:
>
>
> On 06/11/2026 21:58, Rishikesh Jethwani wrote:
>  > Thanks. Agreed on (3a), (i) and the net fixes, with one condition on
>  > (i) below.
>
> Hi Rishikesh,

Hi Tariq,

Thanks. I've now reviewed the seven-patch RFC [1]. The held-skb,
full-resync and pass-through changes cover the points I raised. I've
replied on patches 7 and 4 with two remaining holes in the one-SQ
guarantee: the selection-time ownership transfer, and the PTP SQs in
the drain.

> Thanks. I took your points into the patches, details inline. I'm
> re-running the ConnectX-6 Dx tests with the changes, and will share the
> RFCs once they pass.
>
>  > Thanks. Until then, the two hardware questions remain open, including
>  > whether DESTROY_SQ can fail; the destroy wrapper discards the status.
>
> DESTROY_SQ doesn't fail while the function is healthy. If it does,
> something went badly wrong in the firmware.

Thanks for clarifying the healthy-function case. I'll continue to use
CQEs or successful SQ destruction as the retirement evidence.

The DEK question is still open: once all old-key WQEs have retired, may
the old DEK be destroyed or returned for reuse while an idle TIS still
names it? This needs to cover both standalone DEKs and pooled IDs reused
by another connection. A flushed replacement need not have installed
the new key, and deletion can return the still-bound TIS to its pool.
The resync fix restores the context on its next use, but does not by
itself establish that releasing the old DEK before then is safe.

Please keep that HW/FW answer explicit. Until it is settled, I cannot
treat a flush/close alone as sufficient to release a still-named DEK;
the conservative alternative is to retain the possible bindings until
a successful replacement or TIS destruction, including through pooling.

>  > One correction to my last mail: recovery clears RECOVERING even on a
>  > flush timeout, so the queue can wake once stop_room frees.
>
> Thanks. WQEs posted to that SQ then complete with error CQEs, which mark
> their connections for a full resync, as in the first error.
>
>  > Agreed. What rekey needs from that resync fix: a lost initialization
>  > or replacement always triggers a full resync on the next skb that
>  > needs the TLS context, and a connection marked again while the driver
>  > is consuming an earlier mark stays marked (test-and-clear, not
>  > clear-after).
>
> Both hold now. The mark stays until an skb that uses the context
> consumes it, and it is taken with a test-and-clear before the resync is
> posted, so a loss reported meanwhile marks the connection again.
>
>  > An explicit bypass right after the
>  > datalen check in mlx5e_ktls_handle_tx_skb() closes that without a
>  > sequence comparison:
>  >
>  >         if (!skb_is_decrypted(skb))
>  >                 return true;
>
> Agreed, it's in the pass-through patch, with credit.
>
>  > I'm happy to use (i), but warning/counting alone does not cover the
>  > forced-queue case: a detected conflict has to prevent the conflicting
>  > hardware access, since old TIS accesses must have retired before
>  > another SQ can modify the context. Dropping before any TLS WQE or
>  > context-state change would preserve safety without SW crypto.
>
> Agreed, the counter is now a drop. Each connection records the TX queue
> that it posts on. An skb on another TX queue moves the connection only
> if no other skb of its socket is in flight: an skb is freed only once
> the WQEs posted for it completed, or were destroyed with their SQ, so
> nothing of the connection is then pending on any SQ. Otherwise the skb
> is dropped before anything is posted, and counted, and TCP retransmits
> it.
>
> The move is made under the old TX queue's lock, taken with a trylock,
> under which the connection posts there. That serializes the check and
> the publication with that path, and leaves the connection's datapath
> with a compare of the queue index.
>
> This covers your points 1 and 2 where the WQEs are posted, wherever the
> queue came from. For 2, the mlx5e pin now also re-checks wmem at
> selection as you suggest, so a stale ooo_okay doesn't lead to drops in
> the first place. Does that match what you had in mind?
>
>  > 3. The drain has to cover the QoS/HTB SQs too, and reap deactivated
>  >    SQs although mlx5e_poll_tx_cq() returns early once ENABLED is
>  >    clear; a missing or replaced txq2sq entry isn't evidence of
>  >    retirement. On a drain timeout, the fallback must close the old
>  >    SQs first (which keeps the guarantee, modulo DESTROY_SQ), never
>  >    activate with them alive.
>
> That's how the drain works: it covers the QoS SQs, reaps the deactivated
> SQs directly, and on a timeout closes the old channels before activating
> the new ones. The drop above doesn't rely on txq2sq either: an SQ that
> is deactivated alone, e.g. on an HTB leaf delete, keeps its skbs until
> it is destroyed, so their socket keeps its wmem until then.
>
>  > On the price you mention for priority changes, agreed. Please
>  > document in Documentation/networking/tls-offload.rst that pinning
>  > delays priority/DSCP/HTB-driven TX queue changes until migration is
>  > safe, potentially indefinitely under sustained traffic.
>
> Done.
>
>  > Nice, that's simpler. On the core side, the op's contract: the
>  > callback prepares the new key and pending parameters but posts no TLS
>  > WQEs for this connection.
>
> [...]
>
>  > Callbacks are serialized per connection, but another may arrive
>  > before the previous replacement is posted. On success, the driver
>  > must clean up any superseded, never-posted key.
>
> Agreed on the contract.

Agreed. I'll replace v18's live TX del+add with the new callback and
implement mlx5e_ktls_rekey_tx() on the revised fixes. That includes
cleanup of superseded, never-posted keys, and explicit ownership of
retiring DEKs through WQE completion and deferred destruction. Devices
without the callback will stay in software after a KeyUpdate.

The delayed decrypted-clone case remains a separate core follow-up.
The pass-through bypass closes the unmarked-skb case, but the existing
32-bit record lookup can still alias a later record, even without
rekey. Rekey adds the cross-key case; the floor only covers delays
within its comparison window. The fix I'm considering needs TCP
support and a per-skb check in the driver's in-order TX path too; I'll
Cc you when I post it.

>  > Record lookup also uses 32-bit TCP sequence numbers
>  > today without rekey, so a sufficiently delayed clone can alias a live
>  > record there too. The floor catches old-key clones within its
>  > comparison window, but does not remove that ambiguity. I'll look at
>  > closing this in the core separately.
>
> Thanks. On the driver side, the bypass above keeps plaintext skbs out of
> the record lookup, so it's down to the decrypted clones.
>
>  > Please Cc me on the RFCs, and let me know which fixes the rekey
>  > patches should depend on.
>
> Will do. The rekey patches should go on top of these net fixes:
>
>    net/mlx5e: kTLS, Hold a dropped skb until its posted WQEs complete
>    net/mlx5e: kTLS, Resync TX fully after its WQEs were lost
>    net/mlx5e: kTLS, Post TX context params only for skbs that use them
>    net/mlx5e: Drain old TX SQs before activating new channels
>    net: tls: keep the TX queue of a device-offloaded socket stable
>    net/mlx5e: kTLS, Keep the TX queue of an offloaded socket stable
>
> The drop goes to net-next separately, as "net/mlx5e: kTLS, Keep a TX
> connection on one SQ at a time"; the rekey patches don't depend on it.

I do need patch 7, with the selection-time fix from my reply to it, as
a rekey dependency. Patches 1-6 alone do not cover egress skbedit or a
cached queue overwritten by another device on the route. A fenced
replacement on SQ B cannot order itself against old-key WQEs still on
SQ A. Keeping patch 7 in net-next is fine; I'll base rekey on it as
well as the net fixes. The RFC cover letter's "builds on this series"
matches that dependency.

> Thanks,
> Tariq

Thanks,
Rishikesh

[1] https://lore.kernel.org/netdev/20261109114734.2087878-1-tariqt@nvidia.com/

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

* Re: Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs
  2026-11-09 23:21           ` Rishikesh Jethwani
@ 2026-11-10 14:45             ` Tariq Toukan
  0 siblings, 0 replies; 9+ messages in thread
From: Tariq Toukan @ 2026-11-10 14:45 UTC (permalink / raw)
  To: Rishikesh Jethwani, Tariq Toukan
  Cc: Saeed Mahameed, Mark Bloch, Gal Pressman, Jakub Kicinski,
	Boris Pismenny, Sabrina Dubroca, John Fastabend, netdev


On 10/11/2026 1:21, Rishikesh Jethwani wrote:
 > Thanks. I've now reviewed the seven-patch RFC [1]. The held-skb,
 > full-resync and pass-through changes cover the points I raised. I've
 > replied on patches 7 and 4 with two remaining holes in the one-SQ
 > guarantee: the selection-time ownership transfer, and the PTP SQs in
 > the drain.

Thanks for the review. Agreed on both, replied there.

 > The DEK question is still open: once all old-key WQEs have retired, may
 > the old DEK be destroyed or returned for reuse while an idle TIS still
 > names it? This needs to cover both standalone DEKs and pooled IDs reused
 > by another connection. A flushed replacement need not have installed
 > the new key, and deletion can return the still-bound TIS to its pool.
 > The resync fix restores the context on its next use, but does not by
 > itself establish that releasing the old DEK before then is safe.
 >
 > Please keep that HW/FW answer explicit. Until it is settled, I cannot
 > treat a flush/close alone as sufficient to release a still-named DEK;
 > the conservative alternative is to retain the possible bindings until
 > a successful replacement or TIS destruction, including through pooling.

Double checking. Will update.

 > Agreed. I'll replace v18's live TX del+add with the new callback and
 > implement mlx5e_ktls_rekey_tx() on the revised fixes.

Sounds good.

 > The delayed decrypted-clone case remains a separate core follow-up.

[...]

 > I'll Cc you when I post it.

Thanks, please do.

 > I do need patch 7, with the selection-time fix from my reply to it, as
 > a rekey dependency. Patches 1-6 alone do not cover egress skbedit or a
 > cached queue overwritten by another device on the route. A fenced
 > replacement on SQ B cannot order itself against old-key WQEs still on
 > SQ A. Keeping patch 7 in net-next is fine; I'll base rekey on it as
 > well as the net fixes.

Right, I missed those cases. Rekey depends on patch 7 too, and v2 of
the RFC will carry the selection-time fix.

Thanks,
Tariq

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

end of thread, other threads:[~2026-11-10 14:45 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-22 18:17 Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs Rishikesh Jethwani
2026-11-03 14:45 ` Tariq Toukan
2026-11-04  5:01   ` Rishikesh Jethwani
2026-11-05  9:25     ` Tariq Toukan
2026-11-06 19:58       ` Rishikesh Jethwani
2026-11-08 19:45         ` Tariq Toukan
2026-11-09  9:21           ` Tariq Toukan
2026-11-09 23:21           ` Rishikesh Jethwani
2026-11-10 14:45             ` Tariq Toukan

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