Re: [PATCH net-next 4/7] bnge: Drop obsolete bn->bnapi NULL checks in open/close paths

[netdev-bot+sashiko@kernel.org]

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

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

- [High] NULL pointer dereference on a retried interface open after a
  failed open: bn->grp_info is now allocated once during probe…

--

bnge: Drop obsolete bn->bnapi NULL checks in open/close paths

This removes the "if (!bn->bnapi)" guards from bnge_free_ring_stats(),
bnge_clear_bnapi_queues(), bnge_disable_int() and bnge_hwrm_ring_free(),
on the grounds that bn->bnapi now lives from bnge_netdev_alloc() until
bnge_netdev_free().

One question below about the sibling allocation, bn->grp_info, which was
moved to probe lifetime by the same series but is still freed by the open
error path.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index dcf04d391570b..dc3d51f31e5ef 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> @@ -85,9 +85,6 @@ static void bnge_free_ring_stats(struct bnge_net *bn)
>  	struct bnge_dev *bd = bn->bd;
>  	int i;
>  
> -	if (!bn->bnapi)
> -		return;
> -
>  	for (i = 0; i < bd->nq_nr_rings; i++) {
>  		struct bnge_napi *bnapi = bn->bnapi[i];
>  		struct bnge_nq_ring_info *nqr = &bnapi->nq_ring;

[Severity: High]
The reasoning here applies equally to bn->grp_info, which the same series
moved to probe lifetime via bnge_init_ring_grps(bn, true) in
bnge_netdev_alloc(), with the matching free in bnge_netdev_free().  But the
open path still frees it on error.  Can a retried open dereference a NULL
bn->grp_info?

bnge_init_nic():

	bnge_init_ring_grps(bn, false);

	bnge_init_vnics(bn);

	rc = bnge_init_chip(bn);
	if (rc)
		goto err_free_ring_grps;
	return rc;

err_free_ring_grps:
	bnge_free_ring_grps(bn);
	bnge_free_rx_ring_pair_bufs(bn);
	return rc;

and bnge_free_ring_grps() drops the probe-time allocation entirely:

static void bnge_free_ring_grps(struct bnge_net *bn)
{
	kfree(bn->grp_info);
	bn->grp_info = NULL;
}

bnge_init_chip() can fail on any of its firmware calls
(bnge_hwrm_stat_ctx_alloc(), bnge_hwrm_ring_alloc(),
bnge_hwrm_vnic_alloc(), bnge_hwrm_set_vnic_filter(), ...), for example on a
busy or timing-out firmware.  bnge_open_core() then unwinds through
err_free_irq -> bnge_free_irq() -> bnge_del_napi() -> bnge_free_core() ->
bnge_hwrm_if_change(bd, false) and returns, with the netdev still
registered and bn->grp_info left NULL.

Is there anything that reallocates it before the next open?  The only
allocation site is bnge_init_ring_grps() under irq_re_init, and the open
path calls it with irq_re_init == false, so the loop writes through the
NULL pointer:

	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_grp_id = INVALID_HW_RING_ID;

bd->nq_nr_rings is at least 1, so a second "ip link set <dev> up" after a
failed open looks like it oopses on bn->grp_info[0].fw_grp_id.

Should the err_free_ring_grps label in bnge_init_nic() simply stop freeing
grp_info now that its lifetime is probe to remove?

This also appears to still be the case at the end of the series (commit
c51d67eb0c2d), where bnge_free_nq0() dereferences bn->grp_info[0] during
teardown and would add a second crash path after such a failed open.

[ ... ]

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