Re: [PATCH net-next] octeontx2-af: npc: Warn on NPC_IPSEC_SPI key overlap

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260713144247.545592-1-rkannoth@xxxxxxxxxxx?part=1
---
--- Patch [1]: [PATCH net-next] octeontx2-af: npc: Warn on NPC_IPSEC_SPI key overlap ---
commit f018da26dc974202fbe56b064f0bc0375c74613a
Author: Hariprasad Kelam <hkelam@xxxxxxxxxxx>

octeontx2-af: npc: Warn on NPC_IPSEC_SPI key overlap

When scanning the MKEX profile to determine supported NPC features, warn
if the SPI extraction field overlaps with other key fields. AH and ESP
may legitimately use the same key offset for SPI, so continue to
advertise NPC_IPSEC_SPI via npc_is_field_present() instead of treating
the overlap as a hard failure.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> index 91b5947dae060..b217c1c19810a 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> @@ -730,6 +730,9 @@ static void npc_set_features(struct rvu *rvu, int blkaddr, u8 intf)
>  			*features &= ~BIT_ULL(NPC_OUTER_VID);
>  
>  	/* Allow extracting SPI field from AH and ESP headers at same offset */
> +	if (npc_check_overlap(rvu, blkaddr, NPC_IPSEC_SPI, 0, intf))
> +		dev_warn(rvu->dev, "Overlap detected the field NPC_IPSEC_SPI\n");

[Severity: Medium]
The commit message notes that AH and ESP may legitimately use the same key
offset for SPI. However, will this new dev_warn() trigger anyway for those
valid configurations?

If an MKEX profile configures AH and ESP SPI extraction at the same
offset, npc_check_overlap() will still see them as mutually exclusive fields
with different lids (LID_LD and LID_LE). Does this result in spurious
warnings polluting the kernel log on valid hardware configurations?

[Severity: High]
This is a pre-existing issue, but does npc_check_overlap() have a data
race when called concurrently?

Looking at npc_check_overlap(), it assigns a local pointer to the shared
scratchpad array and then mutates it:

rvu_npc_fs.c:npc_check_overlap() {
	...
	dummy = &mcam->rx_key_fields[NPC_UNKNOWN];
	...
	memset(dummy, 0, sizeof(struct npc_key_field));
	npc_set_layer_mdata(rvu, mcam, NPC_UNKNOWN, cfg, lid, lt, intf);
	...
}

If multiple VFs simultaneously send mailbox commands that trigger overlap
checks across different CPUs, won't they overwrite this global dummy struct
and corrupt each other's flow validation?

> +	/* Set SPI flag only if AH/ESP and IPSEC_SPI are in the key */
>  	if (npc_is_field_present(rvu, NPC_IPSEC_SPI, intf) &&
>  	    (*features & (BIT_ULL(NPC_IPPROTO_ESP) | BIT_ULL(NPC_IPPROTO_AH))))
>  		*features |= BIT_ULL(NPC_IPSEC_SPI);