Re: [PATCH net-next v1 05/15] net: add METADATA_PSP dst to carry a psp association on an skb

[netdev-bot+sashiko]

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

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

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

- [Medium] The commit message says the second psp_xmit_md() check at
  dequeue, in sk_validate_xmit_skb() (net/core/dev.c:4164-4172), handles…

--

net: add METADATA_PSP dst to carry a psp association on an skb

This adds a METADATA_PSP metadata_dst type, which holds a reference on a
psp association so that socket-less skbs (for example, ones marked by a tc
rule) can be PSP-encrypted by existing driver tx paths. __dev_queue_xmit()
now validates the metadata dst against the device and MTU and keeps the dst
for the driver. sk_validate_xmit_skb() checks it again at dequeue in case
the psp device was unregistered while the packet was queued.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 9665f3b2f5cb2..2cca5f0d302f8 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c

[ ... ]

> @@ -4156,6 +4161,15 @@ static struct sk_buff *sk_validate_xmit_skb(struct sk_buff *skb,
>  
>  	if (sk_validate) {
>  		skb = sk_validate(sk, dev, skb);
> +	} else if (unlikely(skb_psp_md(skb))) {
> +		int psp = psp_xmit_md(skb, dev);
> +
> +		if (psp <= 0) {
> +			kfree_skb_reason(skb, psp == -EMSGSIZE ?
> +					 SKB_DROP_REASON_PKT_TOO_BIG :
> +					 SKB_DROP_REASON_PSP_OUTPUT);
> +			skb = NULL;
> +		}

[Severity: Medium]

The commit message says this re-check handles the case where "the psp
device may have been unregistered while the packet was queued, and its
keys deleted". Does that still hold for skbs that get requeued?

This check runs once, from validate_xmit_skb(), before the tx lock is
taken. If the txq is stopped by then, sch_direct_xmit() requeues the skb
into q->gso_skb. dequeue_skb() then sends it out again without validating
it a second time:

net/sched/sch_generic.c:dequeue_skb() {
    ...
		/* skb in gso_skb were already validated */
		*validate = false;
		if (xfrm_offload(skb))
			*validate = true;
    ...
}

On mlx5, one possible sequence is:

CPU0
sch_direct_xmit()
  validate_xmit_skb()
    sk_validate_xmit_skb()
      psp_xmit_md() returns 1

CPU1
mlx5e_nic_disable()
  mlx5e_close()
    ...
      netif_tx_disable()

CPU0
  HARD_TX_LOCK() finds the txq stopped
  dev_requeue_skb() puts the skb on q->gso_skb

CPU1
  mlx5e_psp_unregister()
    psp_dev_unregister()
      psp_dev_tx_key_del(psd, pas)
        mlx5e_psp_assoc_del()
          mlx5_destroy_encryption_key(priv->mdev, nkey->id);

Later, mlx5e_nic_enable() registers a new psd, and netif_device_attach()
wakes the queues. The qdisc is not reset at any point. The stale skb then
comes off gso_skb with validate set to false and reaches mlx5e_xmit(), where
mlx5e_psp_set_state() still reads from the old association:

	psp_st->spi = pas->tx.spi;
	psp_st->ver = pas->version;
	psp_st->keyid = *(u32 *)pas->drv_data;

Can this hand the NIC the id of a DEK that has already been destroyed, and
that may have been reused for another association? psp_xmit_md() would
catch the psd mismatch, but it never runs again for this skb.

On mlx5, the plain check-then-xmit race is blocked because
netif_tx_disable() runs before psp_dev_unregister(). That leaves only the
requeue window.

The socket path through psp_validate_xmit() was already exposed in the same
way before this patch. The new METADATA_PSP path, however, depends on this
re-check to handle the unregister case.

>  	} else if (unlikely(skb_is_decrypted(skb))) {
>  		pr_warn_ratelimited("unencrypted skb with no associated socket - dropping\n");
>  		kfree_skb(skb);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111005533.6436-1-toddfreed%40meta.com