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/20260812051215.456258-1-rkannoth@marvell.com?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 [off-list ref]

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

quoted

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

[ ... ]

quoted

@@ -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.

quoted

+		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,

[ ... ]

quoted

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

[ ... ]

quoted

@@ -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?

quoted

+			} 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)

[ ... ]