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

[netdev-bot+sashiko]

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

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

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 rollback after a failed attach does not restore the RX
  aggregation-ring mode that existed before the attach.
- [Low] Detach now hides a failed reopen and returns success, and neither
  the commit message nor the code comment says so.

--

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

This patch changes bnxt_xdp_set() so the old program is released only
after bnxt_open_nic() succeeds. When an attach or a replace fails, the
previous program and its ring configuration are put back. The ring and
feature setup moves into a new helper, bnxt_xdp_apply_cfg(), and a detach
whose reopen fails is not undone.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> index 9e5009be8e989..430272fc0594a 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c

[ ... ]

> @@ -418,25 +438,22 @@ 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);
> +
> +	if (netif_running(dev)) {
> +		rc = bnxt_open_nic(bp, true, false);
> +		/* dev_xdp_detach_link() drops the ref even if we fail */
> +		if (rc && prog) {

[Severity: Low]
On a detach, prog is NULL, so if bnxt_open_nic() fails the error never
reaches the caller. The code falls through to bpf_prog_put(old) and
returns 0. By that point bnxt_open_nic() has already closed the device:

bnxt_open_nic() {
    ...
	if (rc) {
		netdev_err(bp->dev, "nic open fail (rc: %x)\n", rc);
		netif_close(bp->dev);
	}
    ...
}

Before this patch the detach path ended with return bnxt_open_nic(...),
so callers saw the error. Those callers include "ip link set dev X xdp
off", a bpf_link detach and bonding propagation. Now they all report
success while the port is down.

Returning 0 may be the better choice, since it keeps core and driver
state consistent. Could the commit message say explicitly that a failed
reopen during detach now returns success and leaves the device closed?
The patch is tagged for stable, so this change is visible to users there
too.

The in-code comment also looks imprecise. dev_xdp_detach_link() does not
drop a prog reference itself. It warns on a driver error and clears the
link:

dev_xdp_detach_link() {
    ...
	WARN_ON(dev_xdp_install(dev, mode, bpf_op, NULL, 0, NULL));
	dev_xdp_set_link(dev, mode, NULL);
    ...
}

This branch also handles the fd based dev_xdp_attach() path. On a driver
error that path returns early, and the core keeps cur_prog together with
its own reference:

dev_xdp_attach() {
    ...
		err = dev_xdp_install(dev, mode, bpf_op, extack, flags, new_prog);
		if (err)
			return err;
    ...
}

Could the comment be reworded to cover both paths and to say that rc is
dropped on purpose for a detach?

> +			WRITE_ONCE(bp->xdp_prog, old);
> +			bnxt_xdp_apply_cfg(bp, old_tx_xdp);
> +			return rc;

[Severity: Medium]
When old is NULL (a fresh attach), this rollback rebuilds the ring mode
by calling bnxt_xdp_apply_cfg() with bp->xdp_prog == NULL. That goes
through bnxt_set_rx_skb_mode(bp, false). Does that path actually undo
the page mode setup?

In page mode, __bnxt_set_rx_skb_mode() clears both agg ring flags
unconditionally:

	if (page_mode) {
		bp->flags &= ~(BNXT_FLAG_AGG_RINGS | BNXT_FLAG_NO_AGG_RINGS);
		bp->flags |= BNXT_FLAG_RX_PAGE_MODE;
		...
		if (dev->mtu > BNXT_MAX_PAGE_MODE_MTU) {
			bp->flags |= BNXT_FLAG_JUMBO;

The non page mode branch of bnxt_set_rx_skb_mode() only clears
BNXT_FLAG_NO_AGG_RINGS, and only when bnxt_get_max_rings() reports
rx > 1. It never sets the flag again.

Take a function where bnxt_get_dflt_rings() decided at probe that agg
rings cannot be afforded:

	bp->flags |= BNXT_FLAG_NO_AGG_RINGS;
	bp->dev->hw_features &= ~(NETIF_F_LRO | NETIF_F_GRO_HW);

If the MTU is above BNXT_MAX_PAGE_MODE_MTU and a program with
xdp_has_frags is attached, the sequence would be:

bnxt_xdp_set()
  bnxt_check_rings()        /* passes with pre-attach flags */
  bnxt_xdp_apply_cfg()
    __bnxt_set_rx_skb_mode(bp, true)  /* clears NO_AGG_RINGS, sets JUMBO */
  bnxt_open_nic()
    __bnxt_reserve_rings()  /* RX doubled, hwr.rx < 2 */

and __bnxt_reserve_rings() fails here:

	if (bp->flags & BNXT_FLAG_AGG_RINGS) {
		if (hwr.rx >= 2) {
			rx_rings = hwr.rx >> 1;
		} else {
			if (netif_running(bp->dev))
				return -ENOMEM;

During the rollback, bnxt_get_max_rings() still sees JUMBO, so it halves
max_rx to 0 and fails. NO_AGG_RINGS therefore stays cleared.
bnxt_set_ring_params() then finds rx_space > PAGE_SIZE without
NO_AGG_RINGS and sets JUMBO again.

bnxt_open_nic() has already closed the device. Won't every later "ip link
set up" fail with the same -ENOMEM in __bnxt_reserve_rings()? That would
leave the port unusable until the MTU is lowered or the driver is
reloaded.

The commit message says:

    Restore the previous program and its ring and feature configuration
    when an attach or a replace fails

Would it be more reliable to snapshot and restore the state instead of
recomputing it? That would cover the relevant bp->flags bits
(NO_AGG_RINGS, AGG_RINGS, JUMBO), the LRO and GRO_HW bits in
dev->hw_features, and bp->rx_ring_size, which bnxt_set_ring_params() can
clamp.

> +		}
>  	}

[ ... ]

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