if_bnxt: add kTLS (kernel TLS) TX offload

[sumit.saxena_broadcom.com]

Summary

Add kTLS TX crypto offload, layered on MPC (crypto add/delete
commands submitted over its TCE channel) and TX completion coalescing
(shared tx_bd_opaque encoding).

Adds kTLS session lifecycle management, a key-context ID allocator,
HWRM key-context alloc/free with partition-mode support, and the TX
submit path: in-order packets go out on the normal iflib path with
CSUM_SND_TAG set, while out-of-order or retransmitted TLS records get
a presync command plus a replayed copy of the record built from the
driver's own replay ring. Also adds kTLS/MPC completion-time counter
sysctls and the max_ktls_entries tunable.

[gallatin]

Also, bnxt_ktls_pre_xmit() and bnxt_ktls_submit_mbuf() both write BDs and advance txr->prod directly, and neither

compares against the consumer.  Is that safe?

sys/dev/bnxt/bnxt_en/bnxt_hwrm.c

1291

If this can actually be called from here, it could result in sleeping while the hwrm lock is held. But I'm not sure this is even reachable

sys/dev/bnxt/bnxt_en/bnxt_ktls.c

381

This needs to match bnxt_crypto_del()'s condition to return early and not do async work. Eg, it needs to also include test_bit(BNXT_STATE_IN_FW_RESET, &softc->state). Or that check needs to be removed from bnxt_crypto_del(). Else you will submit a stack variable (tmp) for async work.

Then again, i'm not sure what the point of even calling bnxt_crypto_del() is in that case, since it does exactly nothing.

631

You should probably zero this.

679

Don't we need to free the partition allocated above in partition mode?

704

This should move down below the unlock to avoid sleeping while holding a mtx

1067

I think this should not be a pointer, but the actual struct

1433

Other error paths free the mbuf, but that is missing here. i think a defrag failure could leak an mbuf.

1650

I think we are missing the case where m_epg_npgs == 1 and the payload occupies [m_epg_1st_off, m_epg_last_len) inside a single page, so the length is

m_epg_last_len - m_epg_1st_off.

1860

This is reversed in bnxt_crypto_add() via bnxt_copy_tls_mp_data(). Does that need to be done here?

1920

I'd re-write as
if (ptag == NULL || ptag->sw == NULL || ptag->sw->type != IF_SND_TAG_TYPE_TLS)

Eg, the defensiveness here to guard against a null tag only causes you to not deref it here, and to then later deref it.

1977

don't you want a break here? Else a STATE_BAD will count as a STATE_FREED as well

sys/dev/bnxt/bnxt_en/if_bnxt.c

655

Ick. Why vzalloc? This is freebsd

Where is this freed?

[sumit.saxena_broadcom.com]

In D58606#1345673, @gallatin wrote:

Also, bnxt_ktls_pre_xmit() and bnxt_ktls_submit_mbuf() both write BDs and advance txr->prod directly, and neither

compares against the consumer.  Is that safe?

I think it should be safe. While adding Tx ktls support in iflib "43d7ee540efe iflib: support for transmit side nic KTLS offload" we had reserved padding BDs in the Tx ring for bnxt_ktls_prexmit() and bnxt_ktls_submit_mbuf(). bnxt driver sets padding tx BDs in this code:

#ifdef KTLS_IFLIB_SUPPORT

		scctx->isc_tx_pad = BNXT_MAX_NUM_SEGS + 5;

#endif

[sumit.saxena_broadcom.com]

sys/dev/bnxt/bnxt_en/bnxt_hwrm.c

1291

If start of the day, HW ktls offload in enabled in firmware, then the ktls is disabled in firmware followed by the firmware reset(not sure if this operation is allowed). In that case, interface re-init after FW reset may end up calling this code to free up ktls resources. We can hold ktls resources even if HW ktls is disabled on fly. Eventually driver unload can always free up the ktls resources. I think we can safely drop this "else" part.

sys/dev/bnxt/bnxt_en/bnxt_ktls.c

381

This path definitely needs rework. The objective is-

1. For non-fatal reset: Send down delete crypto MPC down to NIC to clear connection at hardware level and if MPC is successfully completed, clear software state of the connection. As the bnxt_crypto_del() is async and we call pci_disable_busmaster() after this async call, we need to make it synchronous for this use case. If the bnxt_crypto_del() is successfully completed by NIC, then clear the software state of connection.

2. For Fatal FW reset: Just clear the software state and need not send down the crypto delete MPC to NIC.

631

Ack, will do this in next revision

679

Yes, we need some clean up / reset here to undo the work done by bnxt_partition_alloc.

704

Ack

1067

Ack, this code is ported from Linux. and I don't think partition mode is ever tested on FreeBSD, so it never get caught.

1433

Ack

1650

Ack, will handle it in next revision.

1860

I think bnxt_copy_tls_mp_data() is not required in bnxt_crypto_add(). bnxt_copy_tls_mp_data() assumes the initial record sequence number passed by kernel is in Network byte-order(big-endian) and converts it into little endian as NIC crypto engine wants the seq number in little-endian. However the kernel passes down the record sequence number in CPU native format to the driver during offload initiation ( kernel code which sets initial seqno- "tls->initial_offload_seqno = be64dec(en->rec_seq)").

Since initial TLS sequence number is mostly 0 (infact I have never seen initial sequence number other than 0), this issue did not get caught in the testing.

1920

Ack

1977

Ack.

sys/dev/bnxt/bnxt_en/if_bnxt.c

655

Ack, will fix it in next revision.

[sumit.saxena_broadcom.com]

Handled review bugs/comments

[sumit.saxena_broadcom.com]

AI scan (Pass 1):

• bnxt_ktls_init() enabled kTLS whenever the firmware's kTLS capability was set, without checking that MPC actually has a usable TCE ring (kTLS and MPC are gated on independent firmware capabilities, and bnxt_set_dflt_mpc_rings() can legitimately land on zero TCE rings even when the capability bit is set). The first TLS session would then hit bnxt_select_mpc_ring()'s % mpc_ring_count[TCE] with that count at 0. Require BNXT_MPC_CRYPTO_CAPABLE(softc) and a non-zero TCE ring count before enabling kTLS.
• bnxt_ktls_submit_mbuf() returned 0 (success) on the "no segments after len adjustment" path after already freeing the mbuf, unlike every other error path in the function. Its caller, bnxt_ktls_tx_replay(), treats 0 as "replay posted" and its own caller then skips rolling back the presync command already written to the ring, leaving it with no replay packet behind it. Return ENOSPC instead.
• bnxt_ktls_tx_replay() rewound m_data on r_mb->m_next, assuming that is always the TLS record mbuf. It isn't necessarily: the record is located elsewhere via bnxt_get_tls_record() walking the whole chain for the first M_EXTPG mbuf, and m_pullup() in bnxt_get_tcp_seq() can reshape the chain first. Use bnxt_get_tls_record() here too instead of assuming m_next.
• bnxt_free_ktls_rexmit_mbuf() dereferenced txr->replay_pkt[i] with no NULL check, even though bnxt_alloc_ktls_rexmit_mbuf() leaves replay_pkt NULL on its own allocation failure and callers only guard on if (ktls), not on a successful alloc for that specific ring; it also never NULLed replay_pkt after freeing it, so a second call (or the dangling pointer surviving past this call) would double free. Add both.
• bnxt_alloc_ktls_info() ignored bus_dma_tag_create() failure and left softc->ktls_info fully wired up with a NULL dma_tag anyway; bnxt_ktls_init()/bnxt_alloc_ktls_rexmit_mbuf() would then use it. Back out ktls_info entirely on that failure instead.
• bnxt_tls_snd_tag_alloc()'s failure path scrubbed nothing before returning kctx_tx to the zone, unlike the normal free path (bnxt_tls_tag_zfree()), even though bnxt_ktls_dev_add() may have already memcpy'd the AES-GCM session key into kctx_tx->crypto_info by the time some of those failures are hit (e.g. the unsupported snd_tag-type case). Scrub crypto_info there too.
• bnxt_pre_alloc_tx_crypto_keys() accumulated the *requested* batch_sz into its reported/looping key count, but bnxt_hwrm_key_ctx_alloc() can allocate fewer than requested (it clamps to max_key_ctxs_alloc, and overwrites the count entirely in partition_mode) - the true count is tracked separately in ktls->tck.total_alloc via bnxt_key_ctx_store(). Use that instead, both for the "did we get anything at all" check and the final reported total.
• bnxt_ktls_init() leaks the firmware key contexts bnxt_pre_alloc_tx_crypto_keys() already allocated if a later step (mpc_zone/taskqueue/zone creation) fails; each of those failure paths only cleaned up its own local resource. Call bnxt_clear_ktls() on those paths to at least drop the driver's own tracking of them. Note: this driver has no HWRM_FUNC_KEY_CTX_FREE call anywhere to release them on the firmware side (only the auto-generated struct definitions exist in hsi_struct_def.h) - that gap is pre-existing and is not fixed here.

Issues found during internal testing:

• Under a test creating 50K HW kTLS sessions and randomly killing/ restarting connections, dmesg showed intermittent "kTLS crypto add failed"/"kTLS crypto delete failed" with rc=16 (EBUSY), and not all 50K connections were created on every run.
• bnxt_xmit_crypto_cmd() sent EBUSY straight up to the caller the moment bnxt_start_xmit_mpc() found the MPC TX ring momentarily full, with no retry. Under heavy concurrent session churn (many crypto add/delete commands submitted back-to-back across a small set of MPC TCE rings) the ring can fill for a few microseconds before completions drain it; treating that as a hard failure aborted the corresponding kTLS session setup/teardown outright, explaining the missing connections. Retry with a short DELAY instead: this check runs strictly before anything is posted to the ring, so the condition is unambiguously transient. Bounded at 200 retries * 10us, counted in the new mpc_ring_busy error counter.
• Separately, bnxt_ktls_dev_del() only called bnxt_free_one_kctx() when bnxt_crypto_del() succeeded, silently leaking the KID forever on any failure (this predates the retry above, but the retry makes the remaining failures rarer, not impossible). Every failure return from bnxt_xmit_crypto_cmd() (ENOMEM on the mpc_zone ctx alloc, or EBUSY from the ring-space check) happens before anything reaches the ring, so a failed delete never has an in-flight, outcome- unknown command - it's always safe to free the KID regardless of rc. Under sustained churn this leak measurably shrank the available KID pool over a test run, compounding the "not all sessions created" symptom as fewer KIDs remained for new sessions.

AI scan (Pass 2):

• bnxt_ktls_mpc_cmp() unconditionally dereferenced and mutated ctx->kctx_tx (state, mpc_retry, start/end timing) on every completion, with nothing preventing that tag from having already been returned to the UMA zone (and possibly reallocated to a new session) by the time the completion arrived - both bnxt_tls_snd_tag_alloc()'s failure path (which freed the tag immediately after bnxt_ktls_dev_del() posted an async delete) and the normal bnxt_tls_snd_tag_free()/bnxt_tls_work() path (whose taskqueue defer does not actually wait for any in-flight MPC completion) could race a real completion against reuse of that memory. Add an mpc_inflight refcount to bnxt_ktls_offload_ctx_tx, taken in bnxt_xmit_crypto_cmd() before a command is posted and dropped in bnxt_ktls_mpc_cmp() once its completion is processed; the tag is now only actually freed (via bnxt_tls_tag_zfree()) once the count reaches zero, from whichever of the three sites - the alloc-failure path, bnxt_tls_work(), or the completion handler itself - observes that last.
• bnxt_ktls_submit_mbuf()'s segment-counting loop dropped the final DMA segment outright when it was longer than the remaining replay length, instead of including it with a clamped length; the resulting replay packet was silently shorter than the TLS record it was supposed to reproduce. Clamp segs[i].ds_len in place instead of breaking before counting it.
• bnxt_pre_alloc_tx_crypto_keys() decremented alloc_keys by the requested batch_sz even when bnxt_hwrm_key_ctx_alloc() granted fewer contexts than requested (rc == 0 doesn't mean the full batch was allocated - see max_key_ctxs_alloc clamping and partition mode, already noted above). Decrement by what tck.total_alloc actually grew by instead, and stop with a logged message if a call makes no progress at all.
• bnxt_ktls_dev_del()'s device-not-open retry loop busy-waited via DELAY(1000 * 100) for up to 2 seconds total across BNXT_RETRY_MAX attempts; it runs from bnxt_tls_snd_tag_free() in ordinary thread context with no locks held, so use pause() instead.
• bnxt_detach() waited on uma_zone_get_cur(ktls->zone) reaching zero with no bound, unlike the pending-counter wait right after it; a kTLS session that the stack never tears down (stuck connection, application bug) would hang detach/unload forever. Bound it to 30 seconds, matching the existing pending wait, and log if the bound is hit.
• bnxt_attach_post() freed ktls_info on bnxt_alloc_ktls_rexmit_mbuf() failure but not the counters bnxt_alloc_ktls_counters() had already allocated for it, leaking every counter_u64_t for the life of the attach. Free counters first (bnxt_free_ktls_info() NULLs softc->ktls_info, which would make a later bnxt_free_ktls_counters() call a no-op).
• ktls_counters[]'s "seq_fwd_tx_replay_pkts" sysctl entry used BNXT_KTLS_TX_SEQ_FWD (aliasing seq_fwd_tx_pkts) instead of BNXT_KTLS_TX_SEQ_FWD_REPLAY, hiding the latter counter entirely.
• bnxt_create_ktls_sysctls()'s error-counter loop indexed err_counters[i] by loop position instead of err_counters->id like the normal-counter loop just above it; harmless while the table happens to be enum-ordered, but fragile to reordering.
• bnxt_key_ctx_store()'s tck/rck macros were unscoped object-like #defines expanding to a raw array-subscript expression, risking silently mangling any unrelated local/member named tck or rck anywhere bnxt_ktls.h is included. Replaced with BNXT_TCK()/ BNXT_RCK() function-like macros.
• SET_TX_OPAQUE_KTLS's do { ... } while(0) had a trailing semicolon in the macro definition itself, producing a stray empty statement at every call site. Removed.
• Dropped a duplicate #include <sys/malloc.h>, and reordered bnxt_free_ktls_rexmit_mbuf()'s void inline to the conventional inline void.
• Documented in place, without changing behavior, three items where the reviewer's suggested direction risked being worse than the current code: (1) bnxt_key_ctx_store()'s per-key bnxt_kid_info allocation in the non-contiguous case is real memory/scan-overhead amplification versus the contiguous case, but bounded by kctx->max_ctx and not a leak - fixing it needs a second storage mode in struct bnxt_kid_info touching both bnxt_alloc_one_kctx()/bnxt_free_one_kctx(); (2) bnxt_ktls_submit_mbuf()'s TSO segment size for multi-segment replay packets under 9000 bytes is a DMA-cluster-size artifact rather than a real MSS, but the principled fix (the original mbuf's pi->ipi_tso_segsz) can legitimately be 0 for a small, never-TSO'd original packet that still defragmented into multiple segments here; and (3) the bnxt_xmit_crypto_cmd() ring-busy retry's DELAY() is bounded to 2ms with tx_lock already released, unlike bnxt_ktls_dev_del()'s hundred-millisecond retry loop above, so it doesn't need a sleepable primitive.
• Added a comment on bnxt_ktls_tx_ooo()'s offset = (uintptr_t)m->m_data clarifying that m_data is not a pointer here: for M_EXTPG TLS record mbufs the stack repurposes it as a byte offset into the logical record (tcp_output.c's tcp_m_copym(), cxgbe's kTLS driver via mtod(m_tls, vm_offset_t)) - flagged by review as a suspected bug, verified correct against both call sites.

[gallatin]

My llm found a few things:

• [P1] Initialize kctx_tx->ktls before validation can fail — D58606.186334.patch:1461 Earlier error paths call bnxt_tls_tag_zfree(), which dereferences kctx_tx->ktls->zone. Fresh UMA objects have this field NULL, so an unsupported cipher/version or allocation failure can panic the kernel. Assign it immediately after uma_zalloc().
• [P1] Remove kTLS sysctls before freeing their backing storage — D58606.186334.patch:189 Sysctls retain pointers into ktls_info, but bnxt_ktls_init() or replay-buffer failure frees that structure while attach continues. Reading such a sysctl then dereferences freed memory in counter_u64_fetch().
• [P1] Rebuild or invalidate kTLS state across firmware reset — D58606.186334.patch:124 bnxt_clear_ktls() frees KID lists and partition_bmap, but leaves active READY tags, partition_mode, and the UMA zone intact. Recovery never reruns bnxt_ktls_init() or reprograms sessions: existing tags use erased firmware KIDs, while a new partition-mode allocation calls bit_ffc_at() with a NULL bitmap.
• [P1] Do not recycle a KID before delete completion — D58606.186334.patch:2328 A successful bnxt_crypto_del() only enqueues an asynchronous command, yet the KID is immediately made reusable. A subsequent ADD—particularly on another TCE ring—can race the old DELETE and have its new context erased. On submission failure, recycling is also unsafe because the old firmware context remains.
• [P1] Do not destroy kTLS resources with live send tags — D58606.186334.patch:298 After 30 seconds detach proceeds even when the zone still has allocated tags, then destroys the zone and frees ktls_info. A later send-tag release dereferences those destroyed objects. Detach must drain or explicitly revoke all tags before destruction.
• [P1] Preserve the connection MSS for replay packets — D58606.186334.patch:2880 Replacing the original MSS with 9000 or the largest DMA fragment can produce jumbo retransmissions on a 1500-byte path. Preserve/thread through the original ipi_tso_segsz.
• [P2] Free released tags after failed MPC completions — D58606.186334.patch:2617 A released tag is in FREE, but an unsuccessful completion changes it to BAD; the final decrement only frees tags still in FREE. Failed DELETEs therefore permanently leak UMA objects.

[sumit.saxena_broadcom.com]

Fix several use-after-free/leak/crash paths found by review: set
kctx_tx->ktls before any early return can free it, tear down the
ktls_stats/mpc_cmp_time_stats sysctls before the memory they point
into, reset partition_mode and add a reset generation so a firmware
reset can't leave a stale KID in use or a NULL partition bitmap
dereferenced, defer KID reuse until the delete MPC completion actually
confirms it instead of racing a new ADD against it, wait unbounded
(not 30s) for live send tags before detach destroys their zone, and
preserve the connection's real TSO MSS for replay packets instead of
a fixed 9000 or a DMA-fragment-size guess. Also trim several inline
comments down to the essential fact.

Fix further issues from an another review pass: retry a busy
sysctl_ctx_free() instead of freeing kTLS state out from under live
sysctl nodes, close a use-after-free between bnxt_tls_work() and the
MPC completion handler, drop a dead if_snd_tag_alloc_params local, fix
an mpc_cmp_time sysctl indexed by loop position instead of counter id,
and a few widths/dead-code/comment cleanups.