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

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)

[ ... ]