Re: [PATCH nf] netfilter: nft_payload: fix vlan_tpid get from h_vlan_proto

[Pablo Neira Ayuso <pablo@netfilter.org>]

[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]

On Sat, Apr 03, 2021 at 10:59:59PM +0800, wenxu wrote:
> 
> 在 2021/4/3 21:33, wenxu 写道:
> > 在 2021/4/3 3:54, Pablo Neira Ayuso 写道:
> >> On Fri, Apr 02, 2021 at 03:13:52PM +0800, wenxu@ucloud.cn wrote:
> >>> From: wenxu <wenxu@ucloud.cn>
> >>>
> >>> vlan_tpid of flow_dissector_key_vlan should be set as h_vlan_proto
> >>> but not h_vlan_encapsulated_proto.
> >> Probably this patch instead?
> > I don't think so.  The vlan_tpid in flow_dissector_key_vlan should be the
> >
> > vlan proto (such as ETH_P_8021Q or ETH_P_8021AD) but not h_vlan_encapsulated_proto (for next header proto).
> >
> > But this is a problem that the vlan_h_proto is the same as offsetof(struct ethhdr, h_proto)
> 
> The design of flow_dissector_key_basic->n_porto should be set as next header proto(ipv4/6)
> 
> for vlan packet which is h_vlan_encapsulated_proto in the vlan header. (check from fl_set_key and skb_flow_dissect)
> 
> Maybe the patch should as following?
>
> diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
> index cb1c8c2..84c5ecc 100644
> --- a/net/netfilter/nft_payload.c
> +++ b/net/netfilter/nft_payload.c
> @@ -233,8 +233,8 @@ static int nft_payload_offload_ll(struct nft_offload_ctx *ctx,
>                 if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
>                         return -EOPNOTSUPP;
>  
> -               NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_VLAN, vlan,
> -                                 vlan_tpid, sizeof(__be16), reg);
> +               NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_BASIC, basic,
> +                                 n_proto, sizeof(__be16), reg);

Maybe.

Certainly, the patch that I'm attaching seems to be needed. Otherwise,
vlan id match does not work.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 1487 bytes --]

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index d157d1b9cad6..b7c1c91d7abd 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -1942,23 +1942,25 @@ static int mlx5e_flower_parse_meta(struct net_device *filter_dev,
 		return 0;
 
 	flow_rule_match_meta(rule, &match);
-	if (match.mask->ingress_ifindex != 0xFFFFFFFF) {
-		NL_SET_ERR_MSG_MOD(extack, "Unsupported ingress ifindex mask");
-		return -EOPNOTSUPP;
-	}
+	if (match.mask->ingress_ifindex) {
+		if (match.mask->ingress_ifindex != 0xFFFFFFFF) {
+			NL_SET_ERR_MSG_MOD(extack, "Unsupported ingress ifindex mask");
+			return -EOPNOTSUPP;
+		}
 
-	ingress_dev = __dev_get_by_index(dev_net(filter_dev),
-					 match.key->ingress_ifindex);
-	if (!ingress_dev) {
-		NL_SET_ERR_MSG_MOD(extack,
-				   "Can't find the ingress port to match on");
-		return -ENOENT;
-	}
+		ingress_dev = __dev_get_by_index(dev_net(filter_dev),
+						 match.key->ingress_ifindex);
+		if (!ingress_dev) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Can't find the ingress port to match on");
+			return -ENOENT;
+		}
 
-	if (ingress_dev != filter_dev) {
-		NL_SET_ERR_MSG_MOD(extack,
-				   "Can't match on the ingress filter port");
-		return -EOPNOTSUPP;
+		if (ingress_dev != filter_dev) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Can't match on the ingress filter port");
+			return -EOPNOTSUPP;
+		}
 	}
 
 	return 0;