Re: [PATCH net-next v2 1/6] bnge: Extract bnapi allocation and cleanup into helpers

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The commit message describes the three new NULL guards as
  preparatory hardening ("add NULL guards for rx_ring and tx_ring in…

--

bnge: Extract bnapi allocation and cleanup into helpers

This moves the bn->bnapi allocation and free code out of
bnge_alloc_core()/bnge_free_core() into bnge_alloc_bnapi_mem() and
bnge_free_bnapi_mem(), and adds NULL checks for bn->rx_ring and
bn->tx_ring in the ring cleanup helpers.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index a4288f0258f87..c1810a3098883 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> @@ -825,6 +825,9 @@ static void bnge_free_tpa_info(struct bnge_net *bn)
>  	struct bnge_dev *bd = bn->bd;
>  	int i, j;
>  
> +	if (!bn->rx_ring)
> +		return;
> +
>  	for (i = 0; i < bd->rx_nr_rings; i++) {
>  		struct bnge_rx_ring_info *rxr = &bn->rx_ring[i];
>  
> @@ -881,6 +884,9 @@ static void bnge_free_rx_rings(struct bnge_net *bn)
>  	struct bnge_dev *bd = bn->bd;
>  	int i;
>  
> +	if (!bn->rx_ring)
> +		return;
> +
>  	bnge_free_tpa_info(bn);
>  	for (i = 0; i < bd->rx_nr_rings; i++) {
>  		struct bnge_rx_ring_info *rxr = &bn->rx_ring[i];
> @@ -1024,6 +1030,9 @@ static void bnge_free_tx_rings(struct bnge_net *bn)
>  	struct bnge_dev *bd = bn->bd;
>  	int i;
>  
> +	if (!bn->tx_ring)
> +		return;
> +

[Severity: Medium]
Are these three new NULL checks really just preparation, or do they fix a
reachable NULL pointer dereference that exists today?

The commit message says only:

    Prepare the driver to move bnapi allocation out of the open/close path,
    and add NULL guards for rx_ring and tx_ring in core driver cleanup paths.

Before this patch, bnge_alloc_core() allocates bn->bnapi, then bn->rx_ring,
then bn->tx_ring, and every failure goes to err_free_core: which calls
bnge_free_core(). bnge_free_core() unconditionally calls
bnge_free_tx_rings() and bnge_free_rx_rings(), so if the bn->rx_ring
allocation fails both pointers are still NULL:

drivers/net/ethernet/broadcom/bnge/bnge_netdev.c:bnge_free_tx_rings() {
	for (i = 0; i < bd->tx_nr_rings; i++) {
		struct bnge_tx_ring_info *txr = &bn->tx_ring[i];
		...
		bnge_free_ring(bd, &ring->ring_mem);
}

and bnge_free_ring() dereferences that near-NULL address right away:

drivers/net/ethernet/broadcom/bnge/bnge_rmem.c:bnge_free_ring() {
	if (!rmem->pg_arr)
		goto skip_pages;

The rx side looks the same: bnge_free_rx_rings() -> bnge_free_tpa_info()
reads rxr->rx_tpa_idx_map, and bnge_free_rx_rings() itself calls
page_pool_destroy(rxr->page_pool), both off a NULL bn->rx_ring.

Since bnge_alloc_core() runs from the ndo_open path, an allocation failure
during "ip link set dev ethX up" would oops instead of returning -ENOMEM.
git blame points the affected loops at commit 0259379037cab9 ("bng_en: Add
initial support for RX and TX rings"). Would it make sense to split these
guards into their own patch with a Fixes: 0259379037cab9 tag, so it can be
picked up for stable? No commit in this series (10cfa109c8..63494c308b19)
carries such a tag or mentions the crash.

Note that bnge_free_rx_ring_pair_bufs() already had the same
if (!bn->rx_ring) return; check, which suggests the guard is functional
rather than cosmetic.

>  	for (i = 0; i < bd->tx_nr_rings; i++) {
>  		struct bnge_tx_ring_info *txr = &bn->tx_ring[i];
>  		struct bnge_ring_struct *ring;

[ ... ]

> @@ -1241,6 +1254,19 @@ static int bnge_alloc_core(struct bnge_net *bn)
>  		nqr->ring_struct.ring_mem.flags = BNGE_RMEM_RING_PTE_FLAG;
>  	}
>  
> +	return 0;
> +}
> +
> +static int bnge_alloc_core(struct bnge_net *bn)
> +{
> +	struct bnge_dev *bd = bn->bd;
> +	int i, j, rc;
> +
> +	rc = bnge_alloc_bnapi_mem(bn);
> +	if (rc)
> +		return rc;
> +
> +	rc = -ENOMEM;
>  	bn->rx_ring = kzalloc_objs(struct bnge_rx_ring_info, bd->rx_nr_rings);
>  	if (!bn->rx_ring)
>  		goto err_free_core;

This is the branch that reaches bnge_free_core() with both bn->rx_ring and
bn->tx_ring still NULL.

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