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

[Subbaraya Sundeep <sbhatta@marvell.com>]

Hi Simon,

On 2026-06-03 at 22:37:59, Simon Horman (horms@kernel.org) wrote:
> 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?
>
Some background:
 Say NIX hardware supports 128 LFs then
 1. 128 MCAM entries are always reserved by AF to receive unicast traffic to each LF
 2. PFs/VFs request a LF attachment during their probe via mbox message to AF
 3. devlink command to set NIX LFs is added later upon requests from customers/users
    so that if their system uses only 2 PFs then they can use the rest 126 mcam
    entries for additional traffic steering
 4. The only execption is user has to load PF and VF drivers as modules after
    setting NIX LFs as first command via AF devlink.
> > 
> > > +	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?
> > 
As from above background this is not possible.
> > > +
> > > +		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.
> > 
Sure will modify the message

Thanks,
Sundeep

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