Re: [PATCH net-next v3 07/13] net: mana: swap queue sets in mana_xdp_set

[netdev-bot+sashiko@kernel.org]

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

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

- [Low] The rewrite silently repairs a real, user-triggerable failure-
  path bug in the code it replaces, without documenting it or carrying a…

--

net: mana: swap queue sets in mana_xdp_set

The patch converts mana_xdp_set() from the detach/attach sequence to the
pre-allocate-and-swap scheme, so the RX queues are rebuilt for the new
buffer layout before anything live is touched. The XDP program becomes
part of struct mana_qset so it is installed and rolled back together with
the queues it was sized for.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_bpf.c b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> index e16ce2a071583..d2093625956bb 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_bpf.c

[ ... ]

> @@ -198,46 +205,46 @@ static int mana_xdp_set(struct net_device *ndev, struct bpf_prog *prog,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	/* One refcnt of the prog is hold by the caller already, so
> -	 * don't increase refcnt for this one.
> -	 */
> -	apc->bpf_prog = prog;
> -
>  	if (apc->port_is_up) {
> -		/* Re-create rxq's after xdp prog was loaded or unloaded.
> -		 * Ex: re create rxq's to switch from full pages to smaller
> -		 * size page fragments when xdp prog is unloaded and
> -		 * vice-versa.
> -		 */
> -
> -		/* Pre-allocate buffers to prevent failure in mana_attach */
> -		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
> -		if (err) {
> +		scratch = mana_qset_scratch_alloc(apc);
> +		if (!scratch) {
>  			NL_SET_ERR_MSG_MOD(extack,
> -					   "XDP: Insufficient memory for tx/rx re-config");
> -			return err;
> +					   "XDP: Insufficient memory for re-config");
> +			return -ENOMEM;
>  		}
>  
> -		err = mana_detach(ndev, false);
> +		err = mana_alloc_qset(apc, scratch, apc->num_queues,
> +				      apc->rx_queue_size, apc->tx_queue_size,
> +				      apc->priv_flags, apc->configured_mtu,
> +				      prog, &newq);
>  		if (err) {
> -			netdev_err(ndev,
> -				   "mana_detach failed at xdp set: %d\n", err);
>  			NL_SET_ERR_MSG_MOD(extack,
> -					   "XDP: Re-config failed at detach");
> -			goto err_dealloc_rxbuffs;
> +					   "XDP: Re-config failed at alloc");
> +			mana_qset_scratch_free(scratch);
> +			return err;
>  		}

[Severity: Low]
Does this hunk quietly fix a failure-path bug in the code it replaces?

In the removed version, apc->bpf_prog was assigned before any allocation:

	apc->bpf_prog = prog;

	if (apc->port_is_up) {
		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
		if (err) {
			NL_SET_ERR_MSG_MOD(extack,
					   "XDP: Insufficient memory for tx/rx re-config");
			return err;
		}

That early return skips the err_dealloc_rxbuffs label, which is the only
place that restored the old pointer:

	err_dealloc_rxbuffs:
		apc->bpf_prog = old_prog;

So on -ENOMEM from mana_pre_alloc_rxbufs() the attach is reported as failed
to userspace, the BPF core drops the caller's reference, and apc->bpf_prog
keeps pointing at the released program. A later mana_xdp_get() consumer such
as mana_chn_setxdp() from mana_alloc_queues() (next ifup, or after a reset)
would then bpf_prog_add() on that pointer.

The new code leaves the live configuration alone until mana_alloc_qset() and
mana_publish_qset() succeed, so the stale pointer can no longer happen.

The commit message only mentions the queue swap and the window where queues
ran without a program:

    The program becomes part of the queue-set configuration, so it is
    swapped atomically with the queues it was built for and restored by the
    rollback path. Program references follow the same swap, so attaching no
    longer leaves the queues briefly running without one.

Could it also note the corrected failure behaviour? The defect appears to
date back to 730ff06d3f5c ("net: mana: Use page pool fragments for RX
buffers instead of full pages to improve memory efficiency."). Since this
refactor is not backportable, would a separate minimal fix for net, or a
Fixes tag, help stable triage here?

>  
> -		err = mana_attach(ndev);
> +		err = mana_publish_qset(apc, &newq, &oldq);
>  		if (err) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901014442.2945689-1-longli%40microsoft.com