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

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

[wenxu]

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.

Fixes: a82055af5959 ("netfilter: nft_payload: add VLAN offload support")
Fixes: 89d8fd44abfb ("netfilter: nft_payload: add C-VLAN offload support")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 net/netfilter/nft_payload.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
index cb1c8c2..4b582eb 100644
--- a/net/netfilter/nft_payload.c
+++ b/net/netfilter/nft_payload.c
@@ -229,7 +229,7 @@ static int nft_payload_offload_ll(struct nft_offload_ctx *ctx,
 		NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_VLAN, vlan,
 				  vlan_tci, sizeof(__be16), reg);
 		break;
-	case offsetof(struct vlan_ethhdr, h_vlan_encapsulated_proto):
+	case offsetof(struct vlan_ethhdr, h_vlan_proto):
 		if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
 			return -EOPNOTSUPP;
 
@@ -244,7 +244,7 @@ static int nft_payload_offload_ll(struct nft_offload_ctx *ctx,
 		NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_CVLAN, vlan,
 				  vlan_tci, sizeof(__be16), reg);
 		break;
-	case offsetof(struct vlan_ethhdr, h_vlan_encapsulated_proto) +
+	case offsetof(struct vlan_ethhdr, h_vlan_proto) +
 							sizeof(struct vlan_hdr):
 		if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
 			return -EOPNOTSUPP;
-- 
1.8.3.1

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

[kernel test robot]

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

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on nf/master]

url:    https://github.com/0day-ci/linux/commits/wenxu-ucloud-cn/netfilter-nft_payload-fix-vlan_tpid-get-from-h_vlan_proto/20210402-151443
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git master
config: i386-randconfig-r022-20210402 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/eca110c24cc53cf938a9fc7fcddcc8ebc5a35e68
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review wenxu-ucloud-cn/netfilter-nft_payload-fix-vlan_tpid-get-from-h_vlan_proto/20210402-151443
        git checkout eca110c24cc53cf938a9fc7fcddcc8ebc5a35e68
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/netfilter/nft_payload.c: In function 'nft_payload_offload_ll':
>> net/netfilter/nft_payload.c:232:2: error: duplicate case value
     232 |  case offsetof(struct vlan_ethhdr, h_vlan_proto):
         |  ^~~~
   net/netfilter/nft_payload.c:217:2: note: previously used here
     217 |  case offsetof(struct ethhdr, h_proto):
         |  ^~~~


vim +232 net/netfilter/nft_payload.c

   195	
   196	static int nft_payload_offload_ll(struct nft_offload_ctx *ctx,
   197					  struct nft_flow_rule *flow,
   198					  const struct nft_payload *priv)
   199	{
   200		struct nft_offload_reg *reg = &ctx->regs[priv->dreg];
   201	
   202		switch (priv->offset) {
   203		case offsetof(struct ethhdr, h_source):
   204			if (!nft_payload_offload_mask(reg, priv->len, ETH_ALEN))
   205				return -EOPNOTSUPP;
   206	
   207			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_ETH_ADDRS, eth_addrs,
   208					  src, ETH_ALEN, reg);
   209			break;
   210		case offsetof(struct ethhdr, h_dest):
   211			if (!nft_payload_offload_mask(reg, priv->len, ETH_ALEN))
   212				return -EOPNOTSUPP;
   213	
   214			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_ETH_ADDRS, eth_addrs,
   215					  dst, ETH_ALEN, reg);
   216			break;
   217		case offsetof(struct ethhdr, h_proto):
   218			if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
   219				return -EOPNOTSUPP;
   220	
   221			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_BASIC, basic,
   222					  n_proto, sizeof(__be16), reg);
   223			nft_offload_set_dependency(ctx, NFT_OFFLOAD_DEP_NETWORK);
   224			break;
   225		case offsetof(struct vlan_ethhdr, h_vlan_TCI):
   226			if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
   227				return -EOPNOTSUPP;
   228	
   229			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_VLAN, vlan,
   230					  vlan_tci, sizeof(__be16), reg);
   231			break;
 > 232		case offsetof(struct vlan_ethhdr, h_vlan_proto):
   233			if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
   234				return -EOPNOTSUPP;
   235	
   236			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_VLAN, vlan,
   237					  vlan_tpid, sizeof(__be16), reg);
   238			nft_offload_set_dependency(ctx, NFT_OFFLOAD_DEP_NETWORK);
   239			break;
   240		case offsetof(struct vlan_ethhdr, h_vlan_TCI) + sizeof(struct vlan_hdr):
   241			if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
   242				return -EOPNOTSUPP;
   243	
   244			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_CVLAN, vlan,
   245					  vlan_tci, sizeof(__be16), reg);
   246			break;
   247		case offsetof(struct vlan_ethhdr, h_vlan_proto) +
   248								sizeof(struct vlan_hdr):
   249			if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
   250				return -EOPNOTSUPP;
   251	
   252			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_CVLAN, vlan,
   253					  vlan_tpid, sizeof(__be16), reg);
   254			break;
   255		default:
   256			return -EOPNOTSUPP;
   257		}
   258	
   259		return 0;
   260	}
   261	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36729 bytes --]

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

[kernel test robot]

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

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on nf/master]

url:    https://github.com/0day-ci/linux/commits/wenxu-ucloud-cn/netfilter-nft_payload-fix-vlan_tpid-get-from-h_vlan_proto/20210402-151443
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git master
config: i386-randconfig-r022-20210402 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/eca110c24cc53cf938a9fc7fcddcc8ebc5a35e68
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review wenxu-ucloud-cn/netfilter-nft_payload-fix-vlan_tpid-get-from-h_vlan_proto/20210402-151443
        git checkout eca110c24cc53cf938a9fc7fcddcc8ebc5a35e68
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/netfilter/nft_payload.c: In function 'nft_payload_offload_ll':
>> net/netfilter/nft_payload.c:232:2: error: duplicate case value
     232 |  case offsetof(struct vlan_ethhdr, h_vlan_proto):
         |  ^~~~
   net/netfilter/nft_payload.c:217:2: note: previously used here
     217 |  case offsetof(struct ethhdr, h_proto):
         |  ^~~~


vim +232 net/netfilter/nft_payload.c

   195	
   196	static int nft_payload_offload_ll(struct nft_offload_ctx *ctx,
   197					  struct nft_flow_rule *flow,
   198					  const struct nft_payload *priv)
   199	{
   200		struct nft_offload_reg *reg = &ctx->regs[priv->dreg];
   201	
   202		switch (priv->offset) {
   203		case offsetof(struct ethhdr, h_source):
   204			if (!nft_payload_offload_mask(reg, priv->len, ETH_ALEN))
   205				return -EOPNOTSUPP;
   206	
   207			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_ETH_ADDRS, eth_addrs,
   208					  src, ETH_ALEN, reg);
   209			break;
   210		case offsetof(struct ethhdr, h_dest):
   211			if (!nft_payload_offload_mask(reg, priv->len, ETH_ALEN))
   212				return -EOPNOTSUPP;
   213	
   214			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_ETH_ADDRS, eth_addrs,
   215					  dst, ETH_ALEN, reg);
   216			break;
   217		case offsetof(struct ethhdr, h_proto):
   218			if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
   219				return -EOPNOTSUPP;
   220	
   221			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_BASIC, basic,
   222					  n_proto, sizeof(__be16), reg);
   223			nft_offload_set_dependency(ctx, NFT_OFFLOAD_DEP_NETWORK);
   224			break;
   225		case offsetof(struct vlan_ethhdr, h_vlan_TCI):
   226			if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
   227				return -EOPNOTSUPP;
   228	
   229			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_VLAN, vlan,
   230					  vlan_tci, sizeof(__be16), reg);
   231			break;
 > 232		case offsetof(struct vlan_ethhdr, h_vlan_proto):
   233			if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
   234				return -EOPNOTSUPP;
   235	
   236			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_VLAN, vlan,
   237					  vlan_tpid, sizeof(__be16), reg);
   238			nft_offload_set_dependency(ctx, NFT_OFFLOAD_DEP_NETWORK);
   239			break;
   240		case offsetof(struct vlan_ethhdr, h_vlan_TCI) + sizeof(struct vlan_hdr):
   241			if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
   242				return -EOPNOTSUPP;
   243	
   244			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_CVLAN, vlan,
   245					  vlan_tci, sizeof(__be16), reg);
   246			break;
   247		case offsetof(struct vlan_ethhdr, h_vlan_proto) +
   248								sizeof(struct vlan_hdr):
   249			if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
   250				return -EOPNOTSUPP;
   251	
   252			NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_CVLAN, vlan,
   253					  vlan_tpid, sizeof(__be16), reg);
   254			break;
   255		default:
   256			return -EOPNOTSUPP;
   257		}
   258	
   259		return 0;
   260	}
   261	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 36729 bytes --]

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

[Pablo Neira Ayuso]

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

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?


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

diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
index cb1c8c231880..dc45199c2489 100644
--- a/net/netfilter/nft_payload.c
+++ b/net/netfilter/nft_payload.c
@@ -237,15 +237,14 @@ static int nft_payload_offload_ll(struct nft_offload_ctx *ctx,
 				  vlan_tpid, sizeof(__be16), reg);
 		nft_offload_set_dependency(ctx, NFT_OFFLOAD_DEP_NETWORK);
 		break;
-	case offsetof(struct vlan_ethhdr, h_vlan_TCI) + sizeof(struct vlan_hdr):
+	case sizeof(struct vlan_ethhdr) + offsetof(struct vlan_hdr, h_vlan_TCI):
 		if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
 			return -EOPNOTSUPP;
 
 		NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_CVLAN, vlan,
 				  vlan_tci, sizeof(__be16), reg);
 		break;
-	case offsetof(struct vlan_ethhdr, h_vlan_encapsulated_proto) +
-							sizeof(struct vlan_hdr):
+	case sizeof(struct vlan_ethhdr) + offsetof(struct vlan_hdr, h_vlan_encapsulated_proto):
 		if (!nft_payload_offload_mask(reg, priv->len, sizeof(__be16)))
 			return -EOPNOTSUPP;
 

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

[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)

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

[wenxu]

在 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);
                nft_offload_set_dependency(ctx, NFT_OFFLOAD_DEP_NETWORK);
                break;
        case offsetof(struct vlan_ethhdr, h_vlan_TCI) + sizeof(struct vlan_hdr):
@@ -249,8 +249,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_CVLAN, vlan,
-                                 vlan_tpid, sizeof(__be16), reg);
+               NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_BASIC, basic,
+                                 n_proto, sizeof(__be16), reg);
                break;
        default:
                return -EOPNOTSUPP;

>
>
>

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

[Pablo Neira Ayuso]

[-- 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;