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

[Ratheesh Kannoth]

On 2026-07-20 at 22:48:58, Simon Horman (horms@xxxxxxxxxx) wrote:
> 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);

These comments are not valid. Let me modify the code omment so that AI reviewer
wont raise this again.

pw-bot: changes-requested