Re: [PATCH net] octeontx2-af: Validate NIX maximum LFs correctly

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
octeontx2-af: Validate NIX maximum LFs correctly

This patch changes the validation condition for setting the maximum
number of NIX LFs via devlink. It replaces the check for assigned MCAM
entries with a check for assigned NIX LFs to determine if resizing
is allowed.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c
> index 6494a9ee2f0dd..8ba4b5ba9d2c7 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c
[ ... ]
> @@ -1521,11 +1523,18 @@ static int rvu_af_dl_nix_maxlf_validate(struct devlink *devlink, u32 id,
>  	/* Do not allow user to modify maximum NIX LFs while mcam entries
>  	 * have already been assigned.
>  	 */
> -	mcam = &rvu->hw->mcam;
> -	if (mcam->bmap_fcnt < mcam->bmap_entries) {
> -		NL_SET_ERR_MSG_MOD(extack,
> -				   "mcam entries have already been assigned, can't resize");
> -		return -EPERM;

[Severity: High]
Does removing the check for MCAM entries allow the devlink parameter
set operation to silently destroy active MCAM tracking state?

The .set callback unconditionally calls npc_mcam_rsrcs_deinit() and
npc_mcam_rsrcs_init(), which frees and reallocates the mcam->bmap. Since
MCAM entries can be allocated independently of NIX LFs (e.g. during
rvu_switch_enable() or default switchdev/PTP rules), could this cause
resource leaks and use-after-free bugs when untracked rules are later
accessed?

> +	blkaddr = rvu_get_next_nix_blkaddr(rvu, blkaddr);
> +	while (blkaddr) {
> +		block = &rvu->hw->block[blkaddr];

[Severity: High]
This is a pre-existing issue, but does this locklessly read the resource
states?

It appears that the devlink validate and set callbacks execute without
holding the driver's resource locks (rvu->rsrc_lock or mcam->lock).
Could a concurrent mailbox request allocate an LF immediately after this
lockless validation check succeeds, leading to a race where the .set
callback frees structures that are actively being used?

> +
> +		free_lfs = rvu_rsrc_free_count(&block->lf);
> +		if (free_lfs != block->lf.max) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "mcam entries have already been assigned, can't resize");

[Severity: Medium]
Since the validation logic was updated to evaluate NIX LF allocations,
should this error message and the comment above be updated as well? The
message still states that "mcam entries have already been assigned", which
might confuse users since it's actually evaluating NIX LFs.

> +			return -EPERM;
> +		}
> +
> +		blkaddr = rvu_get_next_nix_blkaddr(rvu, blkaddr);
>  	}