Re: [v9,net-next,5/6] octeontx2-af: npc: cn20k: dynamically allocate and free default MCAM entries

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
octeontx2-af: npc: cn20k: dynamically allocate and free default MCAM entries

Improve MCAM utilization by tying default (broadcast, multicast,
promisc, ucast) entry lifetime to NIX LF usage. On NIX LF alloc,
allocate default MCAM entries if missing; on NIX LF free, release
them so they return to the pool.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> index ef5b081162eb..1f2128c61ade 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c

[ ... ]

> @@ -1566,8 +1567,8 @@ int rvu_mbox_handler_nix_lf_alloc(struct rvu *rvu,
>
>  	/* Alloc NIX RQ HW context memory and config the base */
>  	hwctx_size = 1UL << ((ctx_cfg >> 4) & 0xF);
> -	err = qmem_alloc(rvu->dev, &pfvf->rq_ctx, req->rq_cnt, hwctx_size);
> -	if (err)
> +	rc = qmem_alloc(rvu->dev, &pfvf->rq_ctx, req->rq_cnt, hwctx_size);
> +	if (rc)
>  		goto free_mem;
>
>  	pfvf->rq_bmap = kcalloc(req->rq_cnt, sizeof(long), GFP_KERNEL);
> @@ -1583,8 +1584,8 @@ int rvu_mbox_handler_nix_lf_alloc(struct rvu *rvu,
>
>  	/* Alloc NIX SQ HW context memory and config the base */
>  	hwctx_size = 1UL << (ctx_cfg & 0xF);
> -	err = qmem_alloc(rvu->dev, &pfvf->sq_ctx, req->sq_cnt, hwctx_size);
> -	if (err)
> +	rc = qmem_alloc(rvu->dev, &pfvf->sq_ctx, req->sq_cnt, hwctx_size);
> +	if (rc)
>  		goto free_mem;
>
>  	pfvf->sq_bmap = kcalloc(req->sq_cnt, sizeof(long), GFP_KERNEL);
> @@ -1599,8 +1600,8 @@ int rvu_mbox_handler_nix_lf_alloc(struct rvu *rvu,
>
>  	/* Alloc NIX CQ HW context memory and config the base */
>  	hwctx_size = 1UL << ((ctx_cfg >> 8) & 0xF);
> -	err = qmem_alloc(rvu->dev, &pfvf->cq_ctx, req->cq_cnt, hwctx_size);
> -	if (err)
> +	rc = qmem_alloc(rvu->dev, &pfvf->cq_ctx, req->cq_cnt, hwctx_size);
> +	if (rc)
>  		goto free_mem;
>
>  	pfvf->cq_bmap = kcalloc(req->cq_cnt, sizeof(long), GFP_KERNEL);

[ ... ]

> @@ -1699,9 +1706,12 @@ int rvu_mbox_handler_nix_lf_alloc(struct rvu *rvu,
>
>  	goto exit;
>
> +free_dft:
> +	if (is_cn20k(rvu->pdev))
> +		npc_cn20k_dft_rules_free(rvu, pcifunc);
> +
>  free_mem:
>  	nix_ctx_free(rvu, pfvf);
> -	rc = -ENOMEM;
>
>  exit:
>  	/* Set macaddr of this PF/VF */

Does the removal of `rc = -ENOMEM;` cause the function to return success
when kcalloc() fails?

Looking at the kcalloc calls for rq_bmap, sq_bmap, and cq_bmap:

drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c:rvu_mbox_handler_nix_lf_alloc() {
	...
	rc = qmem_alloc(rvu->dev, &pfvf->rq_ctx, req->rq_cnt, hwctx_size);
	if (rc)
		goto free_mem;

	pfvf->rq_bmap = kcalloc(req->rq_cnt, sizeof(long), GFP_KERNEL);
	if (!pfvf->rq_bmap)
		goto free_mem;
	...
}

These three kcalloc failure paths reach free_mem with rc == 0 because the
preceding qmem_alloc() succeeded and set rc = 0. Previously, the code set
rc = -ENOMEM at the free_mem label, ensuring that any path reaching it
would return an error.

With the removal of this assignment, don't these paths now fall through
to exit: and return rc == 0, falsely indicating success?

The caller would believe NIX LF allocation succeeded with a partially
torn-down pfvf structure (NULL bitmaps, freed contexts), potentially
leading to NULL pointer dereferences on subsequent NIX LF operations.

Should the kcalloc failure paths set rc = -ENOMEM before jumping to
free_mem?