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

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

[Vaibhav Nagare]

bnxt_xdp_set() stores the new program and drops the reference on the old
one before reopening the NIC.  If bnxt_open_nic() then fails, ndo_bpf()
returns an error with the new program still in bp->xdp_prog.  The caller
treats the error as "nothing was installed" and drops its own reference,
so bp->xdp_prog is left pointing at a freed program and the next XDP
update dereferences it:

  BUG: unable to handle page fault for address: ff78ecc80ddd1038
  RIP: 0010:__bpf_prog_put+0x5/0x80
  Call Trace:
   bnxt_xdp_set+0xad/0x1b0 [bnxt_en]
   dev_xdp_propagate+0x36/0xa0
   bond_xdp_set+0xeb/0x2d0 [bonding]
   dev_xdp_install+0x1b1/0x350
   bpf_xdp_link_update+0xc5/0x1b0
   link_update+0x104/0x1e0
   __sys_bpf+0x662/0xcf0

Seen on a 6.12 based kernel after bnxt_alloc_mem() failed an order-4
allocation on a fragmented host:

  bnxt_en 0000:a0:00.1 ens4f1np1: nic open fail (rc: fffffff4)
  bond1: (slave ens4f1np1): Error -12 calling ndo_bpf

Bonding is not required to hit this; a plain XDP attach on a bnxt
interface takes the same path.

Restore the previous program and its ring and feature configuration
when an attach or a replace fails, and release the old program only
once the change has been committed.  The configuration matters because
bnxt_init_one_rx_ring() only assigns rxr->xdp_prog in page mode, and
__bnxt_set_rx_skb_mode() derives dev->max_mtu from the installed
program.

A detach is not undone: dev_xdp_detach_link() releases the core's
reference whether or not the driver returns an error, so putting the
program back would leak it and leave the driver running a program the
core has already detached.

Fixes: c6d30e8391b8 ("bnxt_en: Add basic XDP support.")
Cc: stable@vger.kernel.org
Signed-off-by: Vaibhav Nagare <vnagare@redhat.com>
---
  v3:
   - don't undo a detach when the reopen fails.  dev_xdp_detach_link()
     releases the core's reference whether or not the driver returns an
     error, so restoring the program leaked it and left the driver running
     a program the core had already detached (Sashiko AI review)
   - v2: https://lore.kernel.org/netdev/20260930070901.1218980-1-vnagare@redhat.com/
   - v1: https://lore.kernel.org/netdev/20260928131458.1012180-1-vnagare@redhat.com/

 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 49 +++++++++++++------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index 9e5009be8e98..430272fc0594 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);
+}
+
 static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
 {
 	struct net_device *dev = bp->dev;
 	int tx_xdp = 0, rc, tc;
 	struct bpf_prog *old;
+	int old_tx_xdp;
 
 	netdev_assert_locked(dev);
 
@@ -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) {
+			WRITE_ONCE(bp->xdp_prog, old);
+			bnxt_xdp_apply_cfg(bp, old_tx_xdp);
+			return rc;
+		}
 	}
-	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;
 }
-- 
2.55.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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