diff mbox series

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

Message ID 1617347632-19283-1-git-send-email-wenxu@ucloud.cn
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series [nf] netfilter: nft_payload: fix vlan_tpid get from h_vlan_proto | expand

Commit Message

wenxu April 2, 2021, 7:13 a.m. UTC 
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(-)

Comments

kernel test robot April 2, 2021, 6:51 p.m. UTC | #1 
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
Pablo Neira Ayuso April 2, 2021, 7:54 p.m. UTC | #2 
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?
wenxu April 3, 2021, 1:33 p.m. UTC | #3 
在 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)
wenxu April 3, 2021, 2:59 p.m. UTC | #4 
在 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;

>
>
>
Pablo Neira Ayuso April 8, 2021, 10:48 p.m. UTC | #5 
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.
diff mbox series

Patch

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;