|SUCCESS| net/bnxt/tf_core: fix null deref on pool use allocation

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164867

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-03

sha: 80317ff6adfd (assumed from Fixes: tag)
Author: Denis Sergeev <denserg.edu at gmail.com>
Subject: [PATCH] net/bnxt/tf_core: fix null deref on pool use allocation

This patch fixes a potential NULL pointer dereference in the bnxt driver's
CPM (Central Pool Manager) component. The rte_zmalloc() call in
cpm_insert_pool_id() lacked a NULL return check, and errors from this
function were silently ignored in tfc_cpm_set_cmm_inst().

> diff --git a/drivers/net/bnxt/tf_core/v3/tfc_cpm.c b/drivers/net/bnxt/tf_core/v3/tfc_cpm.c
> index f58ec48db7..28e2a3d8dd 100644
> --- a/drivers/net/bnxt/tf_core/v3/tfc_cpm.c
> +++ b/drivers/net/bnxt/tf_core/v3/tfc_cpm.c
> @@ -92,6 +92,10 @@ static int cpm_insert_pool_id(struct tfc_cpm *cpm, uint16_t pool_id)
>  
>  	/* Alloc new entry */
>  	new_pool_use = rte_zmalloc("tf", sizeof(struct cpm_pool_use), 0);
> +	if (new_pool_use == NULL) {
> +		PMD_DRV_LOG_LINE(ERR, "Failed to allocate pool_use entry");
> +		return -ENOMEM;
> +	}

The coding style check here is good. However, the error message could be
more informative. Does it make sense to include the pool_id in the log
message to aid debugging? For example:

    PMD_DRV_LOG_LINE(ERR, "Failed to allocate pool_use entry for pool %u",
                     pool_id);

[ ... ]

> @@ -287,6 +291,7 @@ int tfc_cpm_get_pool_size(struct tfc_cpm *cpm, uint32_t *pool_sz_in_records)
>  int tfc_cpm_set_cmm_inst(struct tfc_cpm *cpm, uint16_t pool_id, struct tfc_cmm *cmm)
>  {
>  	struct cpm_pool_entry *pool;
> +	int rc;
>  
>  	if (cpm == NULL) {
>  		PMD_DRV_LOG_LINE(ERR, "CPM is NULL");
> @@ -313,7 +318,12 @@ int tfc_cpm_set_cmm_inst(struct tfc_cpm *cpm, uint16_t pool_id, struct tfc_cmm *
>  		pool->valid = false;
>  	} else {
>  		pool->valid = true;
> -		cpm_insert_pool_id(cpm, pool_id);
> +		rc = cpm_insert_pool_id(cpm, pool_id);
> +		if (rc) {
> +			pool->cmm = NULL;
> +			pool->valid = false;
> +			return rc;
> +		}
>  	}

The error handling looks reasonable. When cpm_insert_pool_id() fails, the
code correctly reverts pool->valid and pool->cmm. However, is there any
concern about the order here? The code sets pool->valid = true before
calling cpm_insert_pool_id(). If that function fails partway through
(perhaps it successfully acquires a lock before failing allocation), does
this cleanup properly restore all state? Does cpm_insert_pool_id() modify
any state in cpm before the allocation that needs unwinding?

Also, when this function returns an error after setting pool->cmm and
pool->valid, are there other cleanup requirements? Does the caller need to
know that the cmm instance was not successfully installed? The original
code ignored the return value entirely, so existing callers may not handle
this error properly.