Re: [PATCH net v2] bnxt_en: don't leave an XDP program installed when the open fails

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · 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).

- [High] The new error path in bnxt_xdp_set() also rolls back a detach.
- [Medium] The v2 rollback does not restore 'the ring and feature
  configuration that was in service before the change', as the commit…

--

bnxt_en: don't leave an XDP program installed when the open fails

With this patch, when bnxt_open_nic() fails, bnxt_xdp_set() puts the
previous XDP program back into bp->xdp_prog. The old program's reference
is now dropped only after the change succeeds. The ring and feature setup
moves into a new bnxt_xdp_apply_cfg() helper, and the error path runs it
again with the previous XDP TX ring count.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> index 9e5009be8e989..edfd751a601ab 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> @@ -381,11 +381,31 @@ int bnxt_xdp_xmit(struct net_device *dev, int num_frames,
>  	return nxmit;
>  }
>  
> +static void bnxt_xdp_apply_cfg(struct bnxt *bp, int tx_xdp)
> +{
> +	struct net_device *dev = bp->dev;
> +	int tc = bp->num_tc ? : 1;
> +
> +	if (bp->xdp_prog) {
> +		bnxt_set_rx_skb_mode(bp, true);
> +		xdp_features_set_redirect_target_locked(dev, true);
> +	} else {
> +		xdp_features_clear_redirect_target_locked(dev);
> +		bnxt_set_rx_skb_mode(bp, false);
> +	}
> +	bp->tx_nr_rings_xdp = tx_xdp;
> +	bp->tx_nr_rings = bp->tx_nr_rings_per_tc * tc + tx_xdp;
> +	bnxt_set_cp_rings(bp, true);
> +	bnxt_set_tpa_flags(bp);
> +	bnxt_set_ring_params(bp);
> +}

[ ... ]

> @@ -418,25 +438,21 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
>  	if (netif_running(dev))
>  		bnxt_close_nic(bp, true, false);
>  
> +	old_tx_xdp = bp->tx_nr_rings_xdp;
>  	old = xchg(&bp->xdp_prog, prog);
> -	if (old)
> -		bpf_prog_put(old);
> -
> -	if (prog) {
> -		bnxt_set_rx_skb_mode(bp, true);
> -		xdp_features_set_redirect_target_locked(dev, true);
> -	} else {
> -		xdp_features_clear_redirect_target_locked(dev);
> -		bnxt_set_rx_skb_mode(bp, false);
> +	bnxt_xdp_apply_cfg(bp, tx_xdp);

[Severity: Medium]

The commit message says the error path will "restore the ring and feature
configuration that was in service before the change". Before the forward
bnxt_xdp_apply_cfg() and bnxt_open_nic() run, though, only bp->xdp_prog
and bp->tx_nr_rings_xdp are saved.

The rollback call below rebuilds the configuration from the state that the
forward path and the failed open left in bp. It does not restore saved
values.

Can the RX ring size get lost here? A non-frags program with MTU at or
below BNXT_MAX_PAGE_MODE_MTU sets BNXT_FLAG_NO_AGG_RINGS. In that state,
bnxt_set_ringparam() accepts rx_pending up to BNXT_MAX_RX_DESC_CNT and
nothing clamps it.

On detach, bnxt_set_rx_skb_mode(bp, false) clears NO_AGG_RINGS, and
bnxt_set_tpa_flags() turns TPA back on. bnxt_set_ring_params() then
lowers the size:

bnxt_set_ring_params() {
    ...
	if (agg_factor) {
		if (ring_size > BNXT_MAX_RX_DESC_CNT_JUM_ENA) {
			ring_size = BNXT_MAX_RX_DESC_CNT_JUM_ENA;
			...
			bp->rx_ring_size = ring_size;
		}
    ...
}

Now suppose bnxt_open_nic() fails. The rollback turns page mode back on,
but it recomputes the ring masks from the already reduced
bp->rx_ring_size. The user's RX ring size ends up smaller even though
ndo_bpf returned an error.

What happens to the ring and TC counts? By the time bnxt_alloc_mem()
fails, __bnxt_open_nic() has already run bnxt_reserve_rings() and
bnxt_adj_tx_rings():

- __bnxt_reserve_rings() writes the firmware-granted counts into
  bp->tx_nr_rings, bp->rx_nr_rings and bp->cp_nr_rings.
- bnxt_adj_tx_rings() rewrites bp->tx_nr_rings_per_tc.

The TC mismatch path in bnxt_reserve_rings() also does this before
returning:

	if (tcs && (bp->tx_nr_rings_per_tc * tcs !=
		    bp->tx_nr_rings - bp->tx_nr_rings_xdp)) {
		netdev_err(bp->dev, "tx ring reservation failure\n");
		netdev_reset_tc(bp->dev);
		bp->num_tc = 0;
		if (bp->tx_nr_rings_xdp)
			bp->tx_nr_rings_per_tc = bp->tx_nr_rings_xdp;
		else
			bp->tx_nr_rings_per_tc = bp->tx_nr_rings;
		return -ENOMEM;
	}

bnxt_xdp_apply_cfg(bp, old_tx_xdp) then computes bp->tx_nr_rings from
these overwritten tx_nr_rings_per_tc and num_tc values. The TC map is
not restored.

For example, take a failed detach where the reservation reduced
rx_nr_rings from r to r' < r. It would end with tx_nr_rings_xdp = r and
rx_nr_rings = r'. The next open's bnxt_adj_tx_rings() may recompute
tx_nr_rings_xdp, so this mismatch might not last. The rx_ring_size and
TC changes do stay.

Could rx_ring_size, tx_nr_rings_per_tc, rx_nr_rings and num_tc be saved
before the change and restored before recomputing? Alternatively, the
claim in the commit message could be narrowed.

> +
> +	if (netif_running(dev)) {
> +		rc = bnxt_open_nic(bp, true, false);
> +		if (rc) {
> +			WRITE_ONCE(bp->xdp_prog, old);
> +			bnxt_xdp_apply_cfg(bp, old_tx_xdp);
> +			return rc;
> +		}

[Severity: High]

Does this rollback also apply when prog == NULL, i.e. a detach? In that
case old is put back into bp->xdp_prog, page mode comes back on, and
bpf_prog_put(old) is skipped, so the driver keeps its reference.

Some ndo_bpf callers assume a detach cannot fail and carry on anyway.
dev_xdp_detach_link(), reached from bpf_xdp_link_release() and
bpf_xdp_link_detach(), does this:

	bpf_op = dev_xdp_bpf_op(dev, mode);
	WARN_ON(dev_xdp_install(dev, mode, bpf_op, NULL, 0, NULL));
	dev_xdp_set_link(dev, mode, NULL);
	return 0;

dev_xdp_set_link() clears both link and prog in dev->xdp_state. Bonding
behaves the same way in __bond_release_one():

		if (dev_xdp_propagate(slave_dev, &xdp))
			slave_warn(bond_dev, slave_dev, "failed to unload XDP program\n");

Before this patch, bnxt had already dropped old and cleared
bp->xdp_prog, so the core and the driver agreed that nothing was
attached. After this patch they disagree.

bnxt_open_nic() calls netif_close() on failure. On the next
"ip link set up", bnxt_init_one_rx_ring() finds the program:

	if (BNXT_RX_PAGE_MODE(bp) && bp->xdp_prog) {
		bpf_prog_add(bp->xdp_prog, 1);
		rxr->xdp_prog = bp->xdp_prog;
	}

Would the detached program then run on every RX ring again, while
"ip link" shows no XDP program attached?

A later netlink detach would not reach the driver either.
dev_xdp_attach() skips ndo_bpf when new_prog == cur_prog, and both are
NULL at that point. At unregister, dev_xdp_uninstall() skips modes where
the core's prog is NULL.

The only place bnxt drops bp->xdp_prog is bnxt_xdp_set(). Does that mean
the reference to the program and its maps leaks unless another program
is attached later?

The legacy netlink path does keep the old program on error, which matches
the new behaviour. The link release and bonding release callers above do
not.

>  	}
> -	bp->tx_nr_rings_xdp = tx_xdp;
> -	bp->tx_nr_rings = bp->tx_nr_rings_per_tc * tc + tx_xdp;
> -	bnxt_set_cp_rings(bp, true);
> -	bnxt_set_tpa_flags(bp);
> -	bnxt_set_ring_params(bp);
>  
> -	if (netif_running(dev))
> -		return bnxt_open_nic(bp, true, false);
> +	if (old)
> +		bpf_prog_put(old);
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930070901.1218980-1-vnagare%40redhat.com