Re: [PATCH v2 net-next] octeontx2-af: add NPC TEID matching for GTP-U and GTP-C flows

On 2026-08-14 at 14:26:54, Simon Horman ([email protected]) 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.
> Full review at: https://sashiko.dev/#/patchset/[email protected]?part=1
>
> Simon says:
>
> Thanks for the updates in v2.
> There is another round of feedback from Sashiko.
> I would appreciate it if you could take a look.
>
> ---
> --- Patch [1]: [PATCH v2 net-next] octeontx2-af: add NPC TEID matching for GTP-U and GTP-C flows ---
> commit 58f64feb1709d91cb7f52c12d7e09c8779588a04
> Author: Suman Ghosh <[email protected]>
>
> octeontx2-af: add NPC TEID matching for GTP-U and GTP-C flows
>
> This patch adds NPC_GTPU_TEID and NPC_GTPC_TEID key fields to the octeontx2
> driver. It wires them through the AF MCAM path to allow ethtool Rx flow rules
> to match on the TEID of GTP-U and GTP-C packets.
>
> Link: https://sashiko.dev/#/patchset/20260810050436.1822566-1-rkannoth%40marvell.com
>
> > 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 d422bdd5e8f80..773097cd85343 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
>
> [ ... ]
>
> > @@ -1805,6 +1821,26 @@ int rvu_mbox_handler_npc_install_flow(struct rvu *rvu,
> >  		req->features |= BIT_ULL(NPC_LXMB);
> >  	}
> >
> > +	if ((req->features & BIT_ULL(NPC_GTPU_TEID)) &&
> > +	    !npc_check_field(rvu, blkaddr, NPC_GTPU_TEID, req->intf)) {
> > +		dev_warn(rvu->dev,
> > +			 "%s: mkex profile does not extract GTP-U TEID\n",
> > +			 __func__);
>
> [Severity: Medium]
> Are these MKEX profile capability checks effectively dead code in the standard
> execution path?
>
> Looking earlier in rvu_mbox_handler_npc_install_flow(), there is a check:
>
> 	if (npc_check_field(rvu, blkaddr, NPC_DMAC, req->intf))
> 		goto process_flow;
>
> Does this mean the new GTP TEID checks are completely bypassed if the profile
> supports DMAC?
>
> Also, if a profile without DMAC support is used, does this expose an
> un-ratelimited warning log spam vector if a VF repeatedly requests GTP rules?
> The preceding DMAC warnings check for is_pffunc_af(req->hdr.pcifunc) to limit
> who can trigger them, but that seems to be missing here.
>
> > +		rvu_npc_free_entry_for_flow_install(rvu, req->hdr.pcifunc,
> > +						    allocated, req->entry);
> > +		return NPC_FLOW_NOT_SUPPORTED;
> > +	}
> > +
> > +	if ((req->features & BIT_ULL(NPC_GTPC_TEID)) &&
> > +	    !npc_check_field(rvu, blkaddr, NPC_GTPC_TEID, req->intf)) {
> > +		dev_warn(rvu->dev,
> > +			 "%s: mkex profile does not extract GTP-C TEID\n",
> > +			 __func__);
> > +		rvu_npc_free_entry_for_flow_install(rvu, req->hdr.pcifunc,
> > +						    allocated, req->entry);
> > +		return NPC_FLOW_NOT_SUPPORTED;
> > +	}
> > +
> >  process_flow:
> >  	if (from_vf && req->default_rule) {
> >  		rvu_npc_free_entry_for_flow_install(rvu, req->hdr.pcifunc,
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c
> > index 99d78fc5a2c4a..7f69e386638ce 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c
>
> [ ... ]
>
> > @@ -1036,8 +1038,31 @@ static int otx2_prepare_flow_request(struct ethtool_rx_flow_spec *fsp,
> >  		return -EOPNOTSUPP;
> >  	}
> >  	if (fsp->flow_type & FLOW_EXT) {
> > +		int skip_user_def = false;
> >  		u16 vlan_etype;
> >
> > +		switch (flow_type) {
> > +		case UDP_V4_FLOW:
> > +		case UDP_V6_FLOW:
> > +		case TCP_V4_FLOW:
> > +		case TCP_V6_FLOW:
> > +			if (ntohs(pmask->dport) == 0xFFFF &&
> > +			    ntohs(pkt->dport) == GTPU_PORT) {
> > +				/* Check for GTP-U packets */
> > +				skip_user_def = true;
> > +				pkt->gtpu_teid = fsp->h_ext.data[1];
> > +				pmask->gtpu_teid = fsp->m_ext.data[1];
> > +				req->features |= BIT_ULL(NPC_GTPU_TEID);
>
> [Severity: High]
> Does this unconditional conversion hijack legitimate non-GTP rules and TCP
> flows?
>
> Since this logic runs for TCP_V4_FLOW and TCP_V6_FLOW, will it incorrectly
> force GTP matching on TCP flows, breaking the rule completely?
>
> Also, if a user specifies a non-GTP UDP rule with FLOW_EXT (e.g. providing a
> VLAN or MAC address) but doesn't provide a TEID mask in m_ext.data[1], does
> this still set skip_user_def to true and require hardware to identify a GTP
> layer?
>
> > +			} else if (ntohs(pmask->dport) == 0xFFFF &&
> > +				   ntohs(pkt->dport) == GTPC_PORT) {
> > +				/* Check for GTP-C packets */
> > +				skip_user_def = true;
> > +				pkt->gtpc_teid = fsp->h_ext.data[1];
> > +				pmask->gtpc_teid = fsp->m_ext.data[1];
> > +				req->features |= BIT_ULL(NPC_GTPC_TEID);
> > +			}
> > +		}
> > +
> >  		if (fsp->m_ext.vlan_etype) {
> >  			/* Partial masks not supported */
> >  			if (be16_to_cpu(fsp->m_ext.vlan_etype) != 0xFFFF)
>
> [ ... ]

Will address in v3.

pw-bot: changes-requested