Subject: Re: [PATCH V3 net-next 4/6] net: hns3: support IP and tunnel VNI dissectors for tc flow

From: Jakub Kicinski <[email-masked]>
Date: Fri, 5 Jun 2026 17:44:55 -0700

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: hns3: support IP and tunnel VNI dissectors for tc flow

This patch adds support for FLOW_DISSECTOR_KEY_IP for ip_tos and 
FLOW_DISSECTOR_KEY_ENC_KEYID for outer_tun_vni in the HNS3 driver's TC flow 
dissection. It also explicitly marks user defined tuples as unused.

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index aa85abda29096..3559b471b3feb 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c

[ ... ]

> @@ -5760,6 +5763,17 @@ static bool hclge_fd_convert_tuple(u32 tuple_bit, u8 *key_x, u8 *key_y,
>  		*(__le32 *)key_x = cpu_to_le32(tmp_x_l);
>  		*(__le32 *)key_y = cpu_to_le32(tmp_y_l);
>  
> +		return true;
> +	case KEY_OPT_VNI:
> +		calc_x(tmp_x_l, *(u32 *)(&p[offset]), *(u32 *)(&p[moffset]));
> +		calc_y(tmp_y_l, *(u32 *)(&p[offset]), *(u32 *)(&p[moffset]));
> +		for (i = 0; i < HCLGE_VNI_LENGTH; i++) {
> +			key_x[i] = ((__force u32)cpu_to_le32(tmp_x_l) >>
> +				    (i * BITS_PER_BYTE)) & 0xFF;
> +			key_y[i] = ((__force u32)cpu_to_le32(tmp_y_l) >>
> +				    (i * BITS_PER_BYTE)) & 0xFF;

[Severity: Medium]
On big-endian architectures, cpu_to_le32() byte-swaps the integer. If we
apply an arithmetic right-shift to a byte-swapped integer here, won't it
extract the bytes in the wrong order and drop the least significant byte?

For example, a 24-bit VNI of 0x123456 becomes 0x56341200 after swapping.
Shifting it right extracts 0x00, 0x12, and 0x34, entirely missing the 0x56
byte. Should this code shift the host-endian integer directly before
applying the mask?

> +		}
> +
>  		return true;
>  	default:
>  		return false;

[ ... ]

> @@ -7340,6 +7354,53 @@ static void hclge_get_cls_key_port(const struct flow_rule *flow,

[ ... ]

> +static void hclge_get_cls_key_ip_tos(const struct flow_rule *flow,
> +				     struct hclge_fd_rule *rule)
> +{
> +	if (flow_rule_match_key(flow, FLOW_DISSECTOR_KEY_IP)) {
> +		struct flow_match_ip match;
> +
> +		flow_rule_match_ip(flow, &match);
> +
> +		rule->tuples.ip_tos = match.key->tos;
> +		rule->tuples_mask.ip_tos = match.mask->tos;
> +		if (!rule->tuples_mask.ip_tos)
> +			rule->unused_tuple |= BIT(INNER_IP_TOS);

[Severity: High]
Since FLOW_DISSECTOR_KEY_IP is now added to the allowed keys list in
hclge_check_cls_flower(), the networking stack could pass rules containing
TTL constraints (match.mask->ttl).

Does this code silently ignore match.mask->ttl?

If the user requests a specific TTL match, the driver appears to successfully
extract the tos field but completely ignores the ttl constraint. Could this
cause the hardware filter to match broader traffic than intended, essentially
bypassing the user's intent? Should it return an error if an unsupported
TTL match is requested?

> +	} else {
> +		rule->unused_tuple |= BIT(INNER_IP_TOS);
> +	}
> +}