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

[Simon Horman <horms@kernel.org>]

On Wed, Jun 03, 2026 at 05:59:46PM +0100, Simon Horman wrote:
> From: 'Simon Horman' <horms@kernel.org>
> 
> 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

Sorry, my tooling isn't helping me much today.
There was supposed to be some commentary in my previous email.
Let me add it.

> ---
> 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;
> 

The issue below looks like it should be looked at in the context of this
patch.

> [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];

While the one below seems like it can be assessed in the context
of possible follow-up.

> [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");
> 

The one below seems less important. But perhaps it would be good
to address if there is a v2 of this patch.

> [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);
> >  	}