Re: [PATCH 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
---
--- Patch [1]: [PATCH net-next] octeontx2-af: add NPC TEID matching for GTP-U and GTP-C flows ---
commit 98f9555a96987d26df070fd9f55b613312f67f14
Author: Suman Ghosh <[email protected]>
Subject: 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 support
ethtool Rx flow rules matching on GTP-U and GTP-C TEIDs. It wires these
fields through the AF MCAM path and exposes them in debugfs.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> @@ -1839,6 +1839,8 @@ struct flow_msg {
>  		u8 next_header;
>  	};
>  	__be16 vlan_itci;
> +	__be32 gtpu_teid;
> +	__be32 gtpc_teid;

[Severity: Critical]
Is it safe to insert these new fields in the middle of struct flow_msg? 

Since this structure is embedded in the npc_install_flow_req mailbox message,
wouldn't this shift the memory offsets of all subsequent fields (like mpls_lse
and the trailing fields in npc_install_flow_req)? This might break the PF/VF
mailbox ABI for older VFs communicating with a newer PF, potentially allowing
corrupted state.

>  #define OTX2_FLOWER_MASK_MPLS_LB		GENMASK(31, 12)
>  #define OTX2_FLOWER_MASK_MPLS_TC		GENMASK(11, 9)
>  #define OTX2_FLOWER_MASK_MPLS_BOS		BIT(8)

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/npc.h b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> --- a/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> @@ -214,6 +214,8 @@ enum key_fields {
>  	NPC_DPORT_UDP,
>  	NPC_SPORT_SCTP,
>  	NPC_DPORT_SCTP,
> +	NPC_GTPU_TEID,
> +	NPC_GTPC_TEID,

[Severity: Critical]
Could inserting these enum values in the middle break the PF/VF mailbox ABI
bitmask? 

The integer values of enum key_fields are used as bit indices for the
req->features bitmask sent over the mailbox. Inserting here shifts the integer
values of NPC_IPSEC_SPI and all subsequent fields. An older VF requesting an
IPSec rule using the old bit index might have its request misinterpreted by the
new PF as a GTP rule.

>  	NPC_IPSEC_SPI,
>  	NPC_MPLS1_LBTCBOS,
>  	NPC_MPLS1_TTL,

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> @@ -3437,6 +3437,14 @@ static void rvu_dbg_npc_mcam_show_flows(struct seq_file *s,
>  			seq_printf(s, "%d ", rule->packet.icmp_code);
>  			seq_printf(s, "mask 0x%x\n", rule->mask.icmp_code);
>  			break;
> +		case NPC_GTPU_TEID:
> +			seq_printf(s, "%d ", ntohl(rule->packet.gtpu_teid));
> +			seq_printf(s, "mask 0x%x\n", ntohl(rule->mask.gtpu_teid));

[Severity: Low]
Does the %d format specifier display the unsigned 32-bit TEID correctly here?
If a TEID has its most significant bit set, it appears it will be printed as a
negative number instead of an unsigned value. Using %u might be preferred.

> +			break;

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c

[ ... ]

> @@ -720,9 +724,10 @@ static void npc_set_features(struct rvu *rvu, int blkaddr, u8 intf)
>  		*features |= BIT_ULL(NPC_IPPROTO_ICMP6);
>  	}
>  
> -	/* for ESP, check if corresponding layer type is present in the key */
> +	/* for ESP/GTP-U/GTP-C check if corresponding layer type is present in the key */
>  	if (npc_check_field(rvu, blkaddr, NPC_LE, intf))
> -		*features |= BIT_ULL(NPC_IPPROTO_ESP);
> +		*features |= BIT_ULL(NPC_IPPROTO_ESP) | BIT_ULL(NPC_GTPU_TEID) |
> +			     BIT_ULL(NPC_GTPC_TEID);

[Severity: High]
Is it intentional to advertise BIT_ULL(NPC_GTPU_TEID) and
BIT_ULL(NPC_GTPC_TEID) without specifically validating them via
npc_check_field()?

If the hardware Key Extraction (KEX) profile doesn't actually extract the
TEID bytes, claiming support for these features could result in rules being
accepted by otx2_prepare_flow_request() but silently ignored when programming
them in npc_update_entry().

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c

[ ... ]

> @@ -1036,8 +1038,29 @@ 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(pkt->dport) == GTPU_PORT) {

[Severity: Medium]
Should we also verify that the port mask is exact (0xFFFF) before inferring
the GTP protocol?

If a user configures a broad UDP matching rule with a base port that aligns
with 2152 under its mask, this logic might misinterpret it as a GTP rule
and apply TEID features incorrectly.

> +				/* Check for GTP-U packets */
> +				skip_user_def = true;

[Severity: Medium]
Is the skip_user_def variable used anywhere after being set to true?

If a user requests a GTP flow rule with a TEID of 1 (which equals
OTX2_DEFAULT_ACTION), the fallback else if condition later in this function
checks if fsp->h_ext.data[1] == cpu_to_be32(OTX2_DEFAULT_ACTION). Because
skip_user_def is never evaluated, wouldn't this incorrectly override the
requested Rx queue action with NIX_RX_ACTION_DEFAULT?

> +				pkt->gtpu_teid = fsp->h_ext.data[1];
> +				pmask->gtpu_teid = fsp->m_ext.data[1];
> +				req->features |= BIT_ULL(NPC_GTPU_TEID);
> +			} else if (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);
> +			}
> +		}
> +

[ ... ]