Re: [PATCH net-next 3/7] bnge: Move bnapi and ring_grp allocation to probe/remove

[netdev-bot+sashiko]

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

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

- [High] bnge_init_nic()'s error path frees probe-owned memory,
  permanently NULLing bn->grp_info and causing a NULL-pointer write on…

--

bnge: Move bnapi and ring_grp allocation to probe/remove

This moves the bn->bnapi and bn->grp_info allocations from the open path
into bnge_netdev_alloc(), frees them in bnge_netdev_free(), and adds an
irq_re_init flag to bnge_init_ring_grps() so that open only resets the
per-group state.  bnge_free_core() now clears the bnapi queue
back-pointers instead of freeing bn->bnapi.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index 6c1cb3aab5a8c..dcf04d391570b 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> @@ -1176,20 +1176,26 @@ static void bnge_free_ring_grps(struct bnge_net *bn)
>  	bn->grp_info = NULL;
>  }
>  
> -static int bnge_init_ring_grps(struct bnge_net *bn)
> +static int bnge_init_ring_grps(struct bnge_net *bn, bool irq_re_init)
>  {
>  	struct bnge_dev *bd = bn->bd;
>  	int i;
>  
> -	bn->grp_info = kzalloc_objs(struct bnge_ring_grp_info, bd->nq_nr_rings);
> -	if (!bn->grp_info)
> -		return -ENOMEM;
> +	if (irq_re_init) {
> +		bn->grp_info = kzalloc_objs(struct bnge_ring_grp_info,
> +					    bd->nq_nr_rings);
> +		if (!bn->grp_info)
> +			return -ENOMEM;
> +	}
> +
>  	for (i = 0; i < bd->nq_nr_rings; i++) {
> -		bn->grp_info[i].fw_stats_ctx = INVALID_HW_RING_ID;
>  		bn->grp_info[i].fw_grp_id = INVALID_HW_RING_ID;

[Severity: High]
With irq_re_init == false, which is the only value the open path passes,
the allocation branch is skipped and this loop writes bn->grp_info[i]
unconditionally.  Can bn->grp_info be NULL when we get here?

See the error path left behind in bnge_init_nic() below.

>  		bn->grp_info[i].rx_fw_ring_id = INVALID_HW_RING_ID;
>  		bn->grp_info[i].agg_fw_ring_id = INVALID_HW_RING_ID;
>  		bn->grp_info[i].nq_fw_ring_id = INVALID_HW_RING_ID;
> +
> +		if (irq_re_init)
> +			bn->grp_info[i].fw_stats_ctx = INVALID_HW_RING_ID;
>  	}
>  
>  	return 0;
> @@ -1201,25 +1207,6 @@ static void bnge_free_bnapi_mem(struct bnge_net *bn)
>  	bn->bnapi = NULL;
>  }
>  
> -static void bnge_free_core(struct bnge_net *bn)
> -{
> -	bnge_free_vnic_attributes(bn);
> -	bnge_free_tx_rings(bn);
> -	bnge_free_rx_rings(bn);
> -	bnge_free_nq_tree(bn);
> -	bnge_free_nq_arrays(bn);
> -	bnge_free_ring_stats(bn);
> -	bnge_free_ring_grps(bn);
> -	bnge_free_vnics(bn);
> -	kfree(bn->tx_ring_map);
> -	bn->tx_ring_map = NULL;
> -	kfree(bn->tx_ring);
> -	bn->tx_ring = NULL;
> -	kfree(bn->rx_ring);
> -	bn->rx_ring = NULL;
> -	bnge_free_bnapi_mem(bn);
> -}
> -
>  static int bnge_alloc_bnapi_mem(struct bnge_net *bn)
>  {
>  	struct bnge_dev *bd = bn->bd;

[ ... ]

> @@ -2834,9 +2858,7 @@ static int bnge_init_nic(struct bnge_net *bn)
>  
>  	bnge_init_tx_rings(bn);
>  
> -	rc = bnge_init_ring_grps(bn);
> -	if (rc)
> -		goto err_free_rx_ring_pair_bufs;
> +	bnge_init_ring_grps(bn, false);
>  
>  	bnge_init_vnics(bn);
>  
> @@ -2847,7 +2869,6 @@ static int bnge_init_nic(struct bnge_net *bn)
>  
>  err_free_ring_grps:
>  	bnge_free_ring_grps(bn);
> -err_free_rx_ring_pair_bufs:
>  	bnge_free_rx_ring_pair_bufs(bn);
>  	return rc;
>  }

[Severity: High]
Now that bn->grp_info belongs to probe/remove, should this
bnge_free_ring_grps() call still be here?

bnge_init_nic() reaches this label when bnge_init_chip() fails, which is
reachable through bnge_hwrm_stat_ctx_alloc(), bnge_hwrm_ring_alloc(),
bnge_hwrm_vnic_alloc() or bnge_setup_vnic() returning an error, and
bnge_free_ring_grps() does:

	kfree(bn->grp_info);
	bn->grp_info = NULL;

The unwind in bnge_open_core() (err_free_irq -> bnge_free_irq ->
bnge_del_napi -> bnge_free_core -> err_if_change) does not reallocate it,
and bnge_free_core() no longer touches grp_info at all after this patch,
so the netdev stays registered with bn->grp_info == NULL.

A second "ip link set <dev> up" then goes:

bnge_open_core()
  bnge_alloc_core()		/* does not touch grp_info */
  bnge_init_nic()
    bnge_init_ring_grps(bn, false)
      /* irq_re_init == false, allocation skipped */
      bn->grp_info[i].fw_grp_id = INVALID_HW_RING_ID;   /* NULL deref */

Does that write oops?  Even if it were guarded, both
bnge_hwrm_stat_ctx_alloc() and hwrm_ring_alloc_send_msg() in
bnge_hwrm_lib.c dereference bn->grp_info:

		grp_info = &bn->grp_info[ring->grp_idx];
		req->stat_ctx_id = cpu_to_le32(grp_info->fw_stats_ctx);

so the driver would also lose its mirror of the programmed firmware
stat_ctx_id / rx_ring_id / nq_ring_id associations.

Would dropping the bnge_free_ring_grps() call from this error path, so
only bnge_free_rx_ring_pair_bufs() remains, match the new ownership
model?

For reference, this is not fixed later in the series: at c51d67eb
bnge_init_nic() still has the err_free_ring_grps label calling
bnge_free_ring_grps(), bnge_init_ring_grps() still dereferences
bn->grp_info unguarded, and "bnge: Create NQ0 during probe and keep
active across open/close" adds another use of the same pointer in
bnge_free_nq0():

	bn->grp_info[0].nq_fw_ring_id = INVALID_HW_RING_ID;

[ ... ]

> @@ -3544,14 +3565,27 @@ int bnge_netdev_alloc(struct bnge_dev *bd, int max_irqs)
>  	spin_lock_init(&bn->stats_lock);
>  
>  	netdev->request_ops_lock = true;
> +
> +	rc = bnge_alloc_bnapi_mem(bn);
> +	if (rc)
> +		goto err_free_port_stats;
> +
> +	rc = bnge_init_ring_grps(bn, true);
> +	if (rc)
> +		goto err_free_bnapi_mem;
> +
>  	rc = register_netdev(netdev);
>  	if (rc) {
>  		dev_err(bd->dev, "Register netdev failed rc: %d\n", rc);
> -		goto err_free_port_stats;
> +		goto err_free_ring_grps;
>  	}
>  
>  	return 0;
>  
> +err_free_ring_grps:
> +	bnge_free_ring_grps(bn);
> +err_free_bnapi_mem:
> +	bnge_free_bnapi_mem(bn);
>  err_free_port_stats:
>  	bnge_free_port_stats(bn);
>  err_free_workq:

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901191728.643212-1-bhargava.marreddy%40broadcom.com