Re: [RFC net-next 5/7] net: tls: keep the TX queue of a device-offloaded socket stable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net: netdev_pick_tx() skips sk_tx_queue_set() when
  sk_dst_cache is NULL
--

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

net: tls: keep the TX queue of a device-offloaded socket stable

This commit adds TLSDEV_F_STICKY_TXQ and SOCK_STICKY_TXQ so that sockets
offloaded to stateful TLS TX devices keep their cached TX queue mapping and
avoid setting skb->ooo_okay while packets are in flight.

Link: https://lore.kernel.org/netdev/20261022181751.1349984-1-rjethwani@purestorage.com/

> diff --git a/include/net/sock.h b/include/net/sock.h
> index 9bba1e793f1ee..ab4b5a5a74f5d 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
[ ... ]
> @@ -2067,6 +2072,29 @@ static inline void sk_tx_queue_clear(struct sock *sk)
>  	WRITE_ONCE(sk->sk_tx_queue_mapping, NO_QUEUE_MAPPING);
>  }
>  
> +#define SK_WMEM_ALLOC_BIAS 1
> +
> +/* A SOCK_STICKY_TXQ socket keeps its TX queue while it has packets in
> + * flight: a stateful TX offload must not see the flow on a new queue
> + * while its work may still be pending on the previous one.
> + */
> +static inline bool sk_tx_queue_pinned(const struct sock *sk)
> +{
> +	return sock_flag(sk, SOCK_STICKY_TXQ) &&
> +	       refcount_read(&sk->sk_wmem_alloc) - SK_WMEM_ALLOC_BIAS >=
> +	       SKB_TRUESIZE(1);
> +}
> +
> +/* Like sk_tx_queue_clear(), but keeps a pinned mapping, see
> + * sk_tx_queue_pinned(). Lifecycle resets (allocation, cloning) must use
> + * sk_tx_queue_clear() directly.
> + */
> +static inline void sk_tx_queue_maybe_clear(struct sock *sk)
> +{
> +	if (!sk_tx_queue_pinned(sk))
> +		sk_tx_queue_clear(sk);
> +}
> +
>  int sk_tx_queue_get(const struct sock *sk);
>  
>  static inline void __sk_rx_queue_set(struct sock *sk,
> @@ -2237,7 +2265,7 @@ __sk_dst_set(struct sock *sk, struct dst_entry *dst)
>  {
>  	struct dst_entry *old_dst;
>  
> -	sk_tx_queue_clear(sk);
> +	sk_tx_queue_maybe_clear(sk);
>  	WRITE_ONCE(sk->sk_dst_pending_confirm, 0);
>  	old_dst = rcu_dereference_protected(sk->sk_dst_cache,
>  					    lockdep_sock_is_held(sk));
> @@ -2250,7 +2278,7 @@ sk_dst_set(struct sock *sk, struct dst_entry *dst)
>  {
>  	struct dst_entry *old_dst;
>  
> -	sk_tx_queue_clear(sk);
> +	sk_tx_queue_maybe_clear(sk);
>  	WRITE_ONCE(sk->sk_dst_pending_confirm, 0);
>  	old_dst = unrcu_pointer(xchg(&sk->sk_dst_cache, RCU_INITIALIZER(dst)));
>  	dst_release(old_dst);

[Severity: Medium]
Can netdev_pick_tx() transmit a packet on a newly selected TX queue without
updating sk->sk_tx_queue_mapping when sk->sk_dst_cache is NULL?

net/core/dev.c:netdev_pick_tx() {
	...
	if (queue_index < 0 || skb->ooo_okay ||
	    queue_index >= dev->real_num_tx_queues) {
		int new_index = get_xps_queue(dev, sb_dev, skb);

		if (new_index < 0)
			new_index = skb_tx_hash(dev, sb_dev, skb);

		if (sk && sk_fullsock(sk) &&
		    rcu_access_pointer(sk->sk_dst_cache))
			sk_tx_queue_set(sk, new_index);

		queue_index = new_index;
	}
	...
}

In __tcp_transmit_skb(), when a SOCK_STICKY_TXQ socket with no packets in
flight sends an skb, it sets skb->ooo_okay = 1 and charges skb->truesize to
sk->sk_wmem_alloc before calling queue_xmit, which makes
sk_tx_queue_pinned(sk) true.  If sk->sk_dst_cache is cleared before that skb
reaches netdev_pick_tx() (for example by a concurrent sk_dst_reset() on a
redirect/PMTU update, or by an egress cgroup BPF program updating SO_MARK),
sk_tx_queue_maybe_clear() leaves the previous sk->sk_tx_queue_mapping intact
while setting sk->sk_dst_cache to NULL.

When that skb enters netdev_pick_tx(), skb->ooo_okay picks a new queue
new_index, but sk_tx_queue_set() is skipped because sk->sk_dst_cache is NULL.
A subsequent packet sent while the first is still in flight has
skb->ooo_okay = 0, repopulates sk->sk_dst_cache without clearing the old
sk->sk_tx_queue_mapping (since sk_tx_queue_pinned(sk) is true), and is sent
on the old queue while the first packet is still in flight on new_index,
corrupting per-connection device TLS state.

Allowing netdev_pick_tx() to also call sk_tx_queue_set() when
sock_flag(sk, SOCK_STICKY_TXQ) is set would keep sk->sk_tx_queue_mapping in
sync with the queue actually used.

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