Re: [PATCH v5 net 01/10] octeontx2-af: npc: cn20k: Propagate MCAM key-type errors on cn20k

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-04-29 at 07:57:13, Ratheesh Kannoth (rkannoth@marvell.com) wrote:
> npc_mcam_idx_2_key_type() can fail; callers used to ignore it and still
> used kw_type when enabling, configuring, copying, and reading MCAM entries.
> That could program or decode hardware with an undefined key type.
>
> Return -EINVAL when key-type lookup fails. Return -EINVAL from
> npc_cn20k_copy_mcam_entry() when src and dest key types differ instead of
> failing silently.
>
> Change npc_cn20k_{enable,config,copy,read}_mcam_entry() to return int on
> success or error. Thread those errors through the cn20k MCAM write and read
> mbox handlers, the cn20k baseline steer read path, NPC defrag move
> (disable/copy/enable with dev_err and -EFAULT), and the DMAC update path in
> rvu_npc_fs.c.
>
> Make npc_copy_mcam_entry() return int so the cn20k branch can return
> npc_cn20k_copy_mcam_entry() without a void/int mismatch, and fail
> NPC_MCAM_SHIFT_ENTRY when copy fails.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> index 7291fdb89b03..7170dcf26200 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
[ ... ]
> @@ -1375,11 +1395,13 @@ int rvu_mbox_handler_npc_cn20k_mcam_alloc_and_write_entry(struct rvu *rvu,
>  							  struct npc_mcam_alloc_and_write_entry_rsp *rsp)
>  {
>  	struct rvu_pfvf *pfvf = rvu_get_pfvf(rvu, req->hdr.pcifunc);
> +	struct npc_mcam_free_entry_req free_req = { 0 };
>  	struct npc_mcam_alloc_entry_req entry_req;
>This isn't a bug introduced here, but entry_req is allocated on the stack
>and is not zero-initialized. Its kw_type field does not appear to be set
>before calling rvu_mbox_handler_npc_mcam_alloc_entry().
>Could this uninitialized stack garbage lead to unpredictable bounds-check
>failures or random mcam allocation errors?

I see your concern regarding the uninitialized entry_req. The reason it was left as-is is that this patch
focuses specifically on the MCAM key-type error propagation logic. However, I agree that leaving stack garbage is suboptimal.
If you prefer not to bundle this with the current Smatch fix

> @@ -3607,9 +3640,30 @@ int npc_defrag_move_vdx_to_free(struct rvu *rvu,
>  				   NPC_AF_CN20K_MCAMEX_BANKX_STAT_EXT(midx,
>  								      bank));
>
> -		npc_cn20k_enable_mcam_entry(rvu, blkaddr, old_midx, false);
> -		npc_cn20k_copy_mcam_entry(rvu, blkaddr, old_midx, new_midx);
> -		npc_cn20k_enable_mcam_entry(rvu, blkaddr, new_midx, true);
> +		/* If bug happened during copy/enable mcam, then there is a bug in allocation
> +		 * algorithm itself. There is no point in rewinding and returning, as it
> +		 * will face further issue. Return error after printing error
> +		 */
> +		if (npc_cn20k_enable_mcam_entry(rvu, blkaddr, old_midx, false)) {
> +			dev_err(rvu->dev,
> +				"%s: Error happened while disabling old_mid=%u\n",
> +				__func__, old_midx);
> +			return -EFAULT;
> +		}
> +
> +		if (npc_cn20k_copy_mcam_entry(rvu, blkaddr, old_midx, new_midx)) {
> +			dev_err(rvu->dev,
> +				"%s: Error happened while copying old_midx=%u new_midx=%u\n",
> +				__func__, old_midx, new_midx);
> +			return -EFAULT;
> +		}
>When these early returns execute, the software vidx map has already been
>redirected to new_midx via a preceding call to npc_vidx_maps_modify().
>Since the early returns skip the remaining bookkeeping steps (updating
>mcam->entry2pfvf_map, freeing old_midx, and updating counts), does this leave
>the system in a corrupted state?
>Would the vidx now point to a new_midx that the user does not officially own
>(causing NPC_MCAM_PERM_DENIED on future accesses), while old_midx is left
>disabled and permanently leaked?

understand the concern regarding the vidx map falling out of sync. However, npc_defrag_move_vdx_to_free runs under a mutex, and
a failure during the copy/enable phase indicates the NPC block has entered a non-deterministic state.
In this specific 'should-never-happen' scenario, attempting a graceful software rewind could mask a critical fault or lead to further exceptions.
My intention with this Smatch fix was to ensure the error is at least caught and logged rather than silently ignored.
I can address a full transactional rollback mechanism in a separate hardening series for net-next.
But issue is graceful rollback can also fail as it is kind of another defrag process.

> @@ -2038,8 +2040,12 @@ void npc_mcam_enable_flows(struct rvu *rvu, u16 target)
>  				continue;
>  			}
>
> -			if (rule->vfvlan_cfg)
> -				npc_update_dmac_value(rvu, blkaddr, rule, pfvf);
> +			if (rule->vfvlan_cfg) {
> +				if (npc_update_dmac_value(rvu, blkaddr, rule, pfvf))
> +					dev_err(rvu->dev,
> +						"Update dmac failed for %u, target=%#x\n",
> +						rule->entry, target);
> +			}
>If npc_update_dmac_value() fails and returns an error like -EINVAL, execution
>falls through and logs the error, but continues processing the current rule.
>The code then computes a hardware offset via NPC_AF_MCAMEX_BANKX_ACTION()
>and unconditionally performs a direct hardware write using rule->entry.
>If rule->entry happens to be invalid or out-of-bounds, could this fall-through
>cause unrelated device registers to be corrupted, or trigger a data abort?
>Even though a later patch in this series hardens some helpers, is a continue
>statement missing here to protect the immediate rvu_write64() that follows?
>Additionally, this is not a regression in this patch, but I noticed that
>list_for_each_entry() is used to iterate over mcam->mcam_rules while
>holding mcam->lock.
>If npc_update_dmac_value() is called, it explicitly drops mcam->lock before
>invoking the mailbox handler.
>If a concurrent thread acquires the lock and deletes the current rule while
>it is dropped, will the list iteration resume with a use-after-free on the
>freed rule pointer?
>Should a safe list iteration macro be used instead, combined with reference
>counting to preserve the rule?
I acknowledge that dropping mcam->lock during list_for_each_entry is a significant architectural vulnerability.
However, fixing this properly requires moving to a reference-counted rule model or an RCU-based iteration to prevent regressions.
Since this series is a targeted bug fix for net focused on Smatch errors, I believe a full locking refactor is
too high-risk for this specific pull request. Will work on hardening patch to net-next.