Re: [RFC net-next 7/7] net/mlx5e: kTLS, Keep a TX connection on one SQ at a time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] net/mlx5e: unlocked txq_ix update in mlx5e_ktls_tx_select_txq()
- [Medium] net/mlx5e: self-trylock failure in mlx5e_ktls_tx_own_txq() on
  txq_ix re-read
- [Medium] net/mlx5e: missed mlx5e_ktls_tx_select_txq() when sticky
  queue is invalid
- [Medium] net/mlx5e: mlx5e_ktls_tx_own_txq() drops software-segmented
  GSO packets
- [Medium] net/mlx5e: kTLS in-flight wmem checks ignore tcp_tsq_workfn()
  +1 reference
--

commit d7dc36218f27e50a58cf06b01d4aeb04325f18d6
Author: Tariq Toukan <tariqt@nvidia.com>

net/mlx5e: kTLS, Keep a TX connection on one SQ at a time

This commit tracks the TX queue index on each kTLS TX offload context and
restricts moving the connection to another TX queue while packets of the
socket are still in flight, dropping packets that would otherwise post WQEs
concurrently to multiple SQs.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> index dfaee2c3624e0..0bac217bbfec8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
[ ... ]
> @@ -323,14 +322,19 @@ u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
>  	if (likely(!mlx5e_sk_sticky_txq(sk)))
>  		return __mlx5e_select_queue(dev, skb, selq);
>  
> -	txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
> -	if (txq_ix >= 0)
> -		return txq_ix;
> +	may_move = mlx5e_sk_txq_may_move(skb);
> +	if (!may_move) {
> +		txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
> +		if (txq_ix >= 0)
> +			return txq_ix;
> +	}
>  
>  	txq_ix = __mlx5e_select_queue(dev, skb, selq);
>  	/* Cache the final queue, as netdev_pick_tx() does with its own pick. */
>  	if (rcu_access_pointer(sk->sk_dst_cache))
>  		sk_tx_queue_set(sk, txq_ix);
> +	if (may_move)
> +		mlx5e_ktls_tx_select_txq(dev, skb, txq_ix);
>  
>  	return txq_ix;
>  }

[Severity: Medium]
Does skipping mlx5e_ktls_tx_select_txq() when !may_move cause multi-packet
bursts to be dropped after mlx5e_select_sticky_queue() rejects a stale queue?

When skb->ooo_okay is 0 (for example, because an earlier skb was still
completing when __tcp_transmit_skb() built this skb), may_move is false.  If
mlx5e_select_sticky_queue() returns -1 because the cached queue index is out
of range after reducing the channel count with ethtool -L,
mlx5e_select_queue() picks a new txq_ix and caches it via sk_tx_queue_set(),
but leaves priv_tx->txq_ix pointing at the old queue even if all prior skbs
have now completed (sk_wmem_alloc_get(sk) <= own).

Subsequent skbs in the same burst then take the newly cached queue in
mlx5e_select_sticky_queue() and queue up behind the first skb, raising
sk_wmem_alloc above skb->truesize.  When the first skb reaches
mlx5e_ktls_handle_tx_skb(), mlx5e_ktls_tx_own_txq() sees
sk_wmem_alloc_get(skb->sk) > skb->truesize and drops every skb in the burst
except the last one, triggering TCP retransmissions and a hardware kTLS OOO
resync.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> index 0bd4d961e7044..9603ef4b3eb3f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
[ ... ]
> @@ -846,6 +850,59 @@ void mlx5e_ktls_tx_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb)
>  	rcu_read_unlock();
>  }
>  
> +/* A connection must not post WQEs to another SQ while WQEs that it posted to
> + * its SQ may be pending, see TLSDEV_F_STICKY_TXQ. Let it move to the TX queue
> + * of @sq only if no other skb of its socket is in flight: an skb is freed only
> + * once the WQEs posted for it are done. The move is made under the lock of the
> + * old TX queue, under which the connection posts there.
> + */
> +static bool mlx5e_ktls_tx_own_txq(struct net_device *netdev,
> +				  struct mlx5e_ktls_offload_context_tx *priv_tx,
> +				  struct mlx5e_txqsq *sq, struct sk_buff *skb)
> +{
> +	int old = READ_ONCE(priv_tx->txq_ix);
> +	struct netdev_queue *old_txq;
> +	bool own;
> +
> +	if (old == MLX5E_KTLS_TX_NO_TXQ)
> +		return cmpxchg(&priv_tx->txq_ix, old, sq->txq_ix) == old;
> +
> +	old_txq = netdev_get_tx_queue(netdev, old);
> +	if (!__netif_tx_trylock(old_txq))
> +		return false;

[Severity: Medium]
Can re-reading priv_tx->txq_ix into old without checking old == sq->txq_ix
cause mlx5e_ktls_tx_own_txq() to trylock the current TX queue and drop the
packet?

mlx5e_ktls_handle_tx_skb() runs under HARD_TX_LOCK for sq->txq and calls
mlx5e_ktls_tx_own_txq() after observing READ_ONCE(priv_tx->txq_ix) !=
sq->txq_ix.  If another CPU updates priv_tx->txq_ix to sq->txq_ix (for
example via mlx5e_ktls_tx_select_txq()) before the second READ_ONCE() here,
old equals sq->txq_ix.  __netif_tx_trylock(old_txq) then tries to acquire
sq->txq->_xmit_lock, which is already held by this CPU, so it fails and drops
the skb with a false warning.

> +
> +	own = READ_ONCE(priv_tx->txq_ix) == old &&
> +	      sk_wmem_alloc_get(skb->sk) <= skb->truesize;

[Severity: Medium]
Does checking sk_wmem_alloc_get(skb->sk) <= skb->truesize drop the first N-1
segments when a GSO skb is software-segmented before mlx5e_xmit()?

When validate_xmit_skb() segments a GSO skb in software via
__skb_gso_segment() -> tcp_gso_segment(), all N resulting segments are
charged to sk->sk_wmem_alloc with destructor tcp_wfree before
dev_hard_start_xmit() loops over the segment list.  If the connection moves
to a new TX queue at xmit time (such as via tc skbedit queue_mapping),
sk_wmem_alloc_get(skb->sk) holds the sum of truesizes of all remaining
segments, so mlx5e_ktls_tx_own_txq() rejects and frees segments 1 through N-1
one by one until only segment N remains, transmitting segment N out of order
and forcing a kTLS resync.

> +	if (own)
> +		WRITE_ONCE(priv_tx->txq_ix, sq->txq_ix);
> +	__netif_tx_unlock(old_txq);
> +
> +	return own;
> +}

[Severity: Medium]
Do the sk_wmem_alloc_get(skb->sk) checks in mlx5e_ktls_tx_own_txq() and
mlx5e_sk_txq_may_move() fail for packets sent from the TCP Small Queues
tasklet?

When tcp_wfree() schedules the TSQ worker, it leaves a +1 reference in
sk->sk_wmem_alloc for sk_free() to release after tcp_tsq_handler() returns:

net/ipv4/tcp_output.c:tcp_tsq_workfn() {
    ...
			tcp_tsq_handler(sk);
		}
		bh_unlock_sock(sk);

		if (need_free)
			sk_free(sk);
    ...
}

Inside tcp_tsq_handler(), __tcp_transmit_skb() charges skb->truesize while
that +1 reference is still present, so sk_wmem_alloc_get(skb->sk) is
skb->truesize + 1 even when no other skb is in flight (which is why
__tcp_transmit_skb() and sk_tx_queue_pinned() test against SKB_TRUESIZE(1)
rather than 0).  As a result, both sk_wmem_alloc_get(skb->sk) <= own in
mlx5e_sk_txq_may_move() and sk_wmem_alloc_get(skb->sk) <= skb->truesize in
mlx5e_ktls_tx_own_txq() evaluate to false during TSQ transmissions, blocking
queue moves at selection time and dropping packets moved to another TX queue.

> +
> +/* The TX queue selection lets the socket of @skb move to @txq_ix only while
> + * no other skb of the socket is in flight, see mlx5e_select_queue(), so the
> + * connection has no WQE pending on any SQ: hand it the new TX queue now. Once
> + * @skb is queued there, the socket's next skbs queue up behind it and hold
> + * sk_wmem_alloc, and mlx5e_ktls_tx_own_txq() would refuse the move.
> + */
> +void mlx5e_ktls_tx_select_txq(struct net_device *netdev, struct sk_buff *skb,
> +			      u16 txq_ix)
> +{
> +	struct mlx5e_ktls_offload_context_tx *priv_tx;
> +	struct tls_context *tls_ctx;
> +
> +	if (!tls_is_skb_tx_device_offloaded(skb))
> +		return;
> +
> +	tls_ctx = tls_get_ctx(skb->sk);
> +	if (rcu_dereference_bh(tls_ctx->netdev) != netdev)
> +		return;
> +
> +	priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
> +	WRITE_ONCE(priv_tx->txq_ix, txq_ix);
> +}

[Severity: High]
Can this unlocked WRITE_ONCE(priv_tx->txq_ix, txq_ix) race with a packet
transmitting on the old TX queue and allow both SQs to post WQEs for the same
kTLS connection concurrently?

In mlx5e_select_queue(), may_move = mlx5e_sk_txq_may_move(skb) is checked
before __mlx5e_select_queue() and sk_tx_queue_set() without holding any TX
queue lock, and mlx5e_ktls_tx_select_txq() then unconditionally overwrites
priv_tx->txq_ix without locking old_txq or re-checking sk_wmem_alloc_get():

CPU0                                    CPU1
mlx5e_select_queue(skb1)
  may_move = mlx5e_sk_txq_may_move(skb1)
    /* true: wmem <= truesize */
                                        mlx5e_select_queue(skb2)
                                          /* !may_move: picks sticky old_q */
                                        mlx5e_xmit(skb2) [on old_sq]
                                          HARD_TX_LOCK(old_txq)
                                          mlx5e_ktls_handle_tx_skb(skb2)
                                            priv_tx->txq_ix == old_sq->txq_ix
                                            /* starts posting WQEs on old_sq */
  txq_ix = __mlx5e_select_queue(...)
  sk_tx_queue_set(sk, new_q)
  mlx5e_ktls_tx_select_txq(skb1, new_q)
    WRITE_ONCE(priv_tx->txq_ix, new_q)
mlx5e_xmit(skb1) [on new_sq]
  HARD_TX_LOCK(new_txq)
  mlx5e_ktls_handle_tx_skb(skb1)
    priv_tx->txq_ix == new_sq->txq_ix
    /* skips mlx5e_ktls_tx_own_txq(),
     * posts WQEs on new_sq concurrently! */

This can happen when skb1 is delayed in an upper device's qdisc (such as a
bonding or VLAN device where the socket lock is no longer held), or when skb1
is a non-decrypted packet (e.g. a pure ACK or pre-offload packet) whose
sk_wmem_alloc was released by skb_orphan_partial() in an upper qdisc, making
mlx5e_sk_txq_may_move() see own == 0 and sk_wmem_alloc == 0 before CPU1 sends
encrypted data on old_sq.  Both SQs then update the connection's TIS state
and priv_tx fields (expected_seq, ctx_post_pending, rec_seq) without
serialization, corrupting TLS records on the wire.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109114734.2087878-1-tariqt@nvidia.com?part=7