[PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[wenxu]

From: wenxu <wenxu@ucloud.cn>

nft add rule bridge firewall rule-100-ingress ip protocol icmp drop

The rule like above "ip protocol icmp", the packet will not be
matched, It tracelate base=NFT_PAYLOAD_LL_HEADER off=12 &&
base=NFT_PAYLOAD_NETWORK_HEADER  off=11
if the packet contained with tag info. But the user don't care about
the vlan tag.

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 include/uapi/linux/netfilter/nf_tables.h |  2 ++
 net/netfilter/nft_payload.c              | 10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 505393c..345787f 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -673,11 +673,13 @@ enum nft_dynset_attributes {
  * @NFT_PAYLOAD_LL_HEADER: link layer header
  * @NFT_PAYLOAD_NETWORK_HEADER: network header
  * @NFT_PAYLOAD_TRANSPORT_HEADER: transport header
+ * @NFT_PAYLOAD_LL_HEADER_NO_TAG: link layer header ignore vlan tag
  */
 enum nft_payload_bases {
 	NFT_PAYLOAD_LL_HEADER,
 	NFT_PAYLOAD_NETWORK_HEADER,
 	NFT_PAYLOAD_TRANSPORT_HEADER,
+	NFT_PAYLOAD_LL_HEADER_NO_TAG,
 };
 
 /**
diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
index 1465b7d..3cc7398 100644
--- a/net/netfilter/nft_payload.c
+++ b/net/netfilter/nft_payload.c
@@ -93,6 +93,12 @@ void nft_payload_eval(const struct nft_expr *expr,
 		}
 		offset = skb_mac_header(skb) - skb->data;
 		break;
+	case NFT_PAYLOAD_LL_HEADER_NO_TAG:
+		if (!skb_mac_header_was_set(skb))
+			goto err;
+
+		offset = skb_mac_header(skb) - skb->data;
+		break;
 	case NFT_PAYLOAD_NETWORK_HEADER:
 		offset = skb_network_offset(skb);
 		break;
@@ -403,6 +409,7 @@ static int nft_payload_set_dump(struct sk_buff *skb, const struct nft_expr *expr
 	case NFT_PAYLOAD_LL_HEADER:
 	case NFT_PAYLOAD_NETWORK_HEADER:
 	case NFT_PAYLOAD_TRANSPORT_HEADER:
+	case NFT_PAYLOAD_LL_HEADER_NO_TAG:
 		break;
 	default:
 		return ERR_PTR(-EOPNOTSUPP);
@@ -421,7 +428,8 @@ static int nft_payload_set_dump(struct sk_buff *skb, const struct nft_expr *expr
 	len    = ntohl(nla_get_be32(tb[NFTA_PAYLOAD_LEN]));
 
 	if (len <= 4 && is_power_of_2(len) && IS_ALIGNED(offset, len) &&
-	    base != NFT_PAYLOAD_LL_HEADER)
+	    base != NFT_PAYLOAD_LL_HEADER &&
+	    base != NFT_PAYLOAD_LL_HEADER_NO_TAG)
 		return &nft_payload_fast_ops;
 	else
 		return &nft_payload_ops;
-- 
1.8.3.1

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[Florian Westphal]

wenxu@ucloud.cn <wenxu@ucloud.cn> wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> nft add rule bridge firewall rule-100-ingress ip protocol icmp drop

nft --debug=netlink add rule bridge firewall rule-100-ingress ip protocol icmp drop
bridge firewall rule-100-ingress
  [ payload load 2b @ link header + 12 => reg 1 ]
  [ cmp eq reg 1 0x00000008 ]
  [ payload load 1b @ network header + 9 => reg 1 ]
  [ cmp eq reg 1 0x00000001 ]
  [ immediate reg 0 drop ]

so problem is that nft inserts a dependency on the ethernet protocol
type (0x800).

But when vlan is involved, that will fail to compare.

It would also fail for qinq etc.

Because of vlan tag offload, the rule about will probably already work
just fine when nft userspace is patched to insert the dependency based
on 'meta protocol'.  Can you see if this patch works?

Subject: Change bridge l3 dependency to meta protocol

This examines skb->protocol instead of ethernet header type, which
might be different when vlan is involved.

nft payload expression will re-insert the vlan tag so ether type
will not be ETH_P_IP.

---
 src/meta.c    |  6 +++++-
 src/payload.c | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/src/meta.c b/src/meta.c
index 583e790ff47d..1e8964eb48c4 100644
--- a/src/meta.c
+++ b/src/meta.c
@@ -539,7 +539,11 @@ static void meta_expr_pctx_update(struct proto_ctx *ctx,
 		proto_ctx_update(ctx, PROTO_BASE_TRANSPORT_HDR, &expr->location, desc);
 		break;
 	case NFT_META_PROTOCOL:
-		if (h->base < PROTO_BASE_NETWORK_HDR && ctx->family != NFPROTO_NETDEV)
+		if (h->base != PROTO_BASE_LL_HDR)
+			return;
+
+		if (ctx->family != NFPROTO_NETDEV &&
+		    ctx->family != NFPROTO_BRIDGE)
 			return;
 
 		desc = proto_find_upper(h->desc, ntohs(mpz_get_uint16(right->value)));
diff --git a/src/payload.c b/src/payload.c
index 6a8118ece890..c99bb2f69977 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -18,6 +18,7 @@
 #include <net/if_arp.h>
 #include <arpa/inet.h>
 #include <linux/netfilter.h>
+#include <linux/if_ether.h>
 
 #include <rule.h>
 #include <expression.h>
@@ -307,6 +308,19 @@ payload_gen_special_dependency(struct eval_ctx *ctx, const struct expr *expr)
 	return NULL;
 }
 
+static const struct proto_desc proto_metaeth = {
+	.name		= "ethmeta",
+	.base		= PROTO_BASE_LL_HDR,
+	.protocols	= {
+		PROTO_LINK(__constant_htons(ETH_P_IP),	 &proto_ip),
+		PROTO_LINK(__constant_htons(ETH_P_ARP),	 &proto_arp),
+		PROTO_LINK(__constant_htons(ETH_P_IPV6), &proto_ip6),
+	},
+	.templates	= {
+		[0]	= PROTO_META_TEMPLATE("protocol", &ethertype_type, NFT_META_PROTOCOL, 16),
+	},
+};
+
 /**
  * payload_gen_dependency - generate match expression on payload dependency
  *
@@ -369,6 +383,12 @@ int payload_gen_dependency(struct eval_ctx *ctx, const struct expr *expr,
 				  "no %s protocol specified",
 				  proto_base_names[expr->payload.base - 1]);
 
+	if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
+		if (expr->payload.desc == &proto_ip ||
+		    expr->payload.desc == &proto_ip6)
+			desc = &proto_metaeth;
+	}
+
 	return payload_add_dependency(ctx, desc, expr->payload.desc, expr, res);
 }
 
-- 
2.21.0

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[wenxu]

Hi Florian,


Thx,  the patch is work!


Br

wenxu

On 6/10/2019 5:44 PM, Florian Westphal wrote:
> wenxu@ucloud.cn <wenxu@ucloud.cn> wrote:
>> From: wenxu <wenxu@ucloud.cn>
>>
>> nft add rule bridge firewall rule-100-ingress ip protocol icmp drop
> nft --debug=netlink add rule bridge firewall rule-100-ingress ip protocol icmp drop
> bridge firewall rule-100-ingress
>   [ payload load 2b @ link header + 12 => reg 1 ]
>   [ cmp eq reg 1 0x00000008 ]
>   [ payload load 1b @ network header + 9 => reg 1 ]
>   [ cmp eq reg 1 0x00000001 ]
>   [ immediate reg 0 drop ]
>
> so problem is that nft inserts a dependency on the ethernet protocol
> type (0x800).
>
> But when vlan is involved, that will fail to compare.
>
> It would also fail for qinq etc.
>
> Because of vlan tag offload, the rule about will probably already work
> just fine when nft userspace is patched to insert the dependency based
> on 'meta protocol'.  Can you see if this patch works?
>
> Subject: Change bridge l3 dependency to meta protocol
>
> This examines skb->protocol instead of ethernet header type, which
> might be different when vlan is involved.
>
> nft payload expression will re-insert the vlan tag so ether type
> will not be ETH_P_IP.
>
> ---
>  src/meta.c    |  6 +++++-
>  src/payload.c | 20 ++++++++++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/src/meta.c b/src/meta.c
> index 583e790ff47d..1e8964eb48c4 100644
> --- a/src/meta.c
> +++ b/src/meta.c
> @@ -539,7 +539,11 @@ static void meta_expr_pctx_update(struct proto_ctx *ctx,
>  		proto_ctx_update(ctx, PROTO_BASE_TRANSPORT_HDR, &expr->location, desc);
>  		break;
>  	case NFT_META_PROTOCOL:
> -		if (h->base < PROTO_BASE_NETWORK_HDR && ctx->family != NFPROTO_NETDEV)
> +		if (h->base != PROTO_BASE_LL_HDR)
> +			return;
> +
> +		if (ctx->family != NFPROTO_NETDEV &&
> +		    ctx->family != NFPROTO_BRIDGE)
>  			return;
>  
>  		desc = proto_find_upper(h->desc, ntohs(mpz_get_uint16(right->value)));
> diff --git a/src/payload.c b/src/payload.c
> index 6a8118ece890..c99bb2f69977 100644
> --- a/src/payload.c
> +++ b/src/payload.c
> @@ -18,6 +18,7 @@
>  #include <net/if_arp.h>
>  #include <arpa/inet.h>
>  #include <linux/netfilter.h>
> +#include <linux/if_ether.h>
>  
>  #include <rule.h>
>  #include <expression.h>
> @@ -307,6 +308,19 @@ payload_gen_special_dependency(struct eval_ctx *ctx, const struct expr *expr)
>  	return NULL;
>  }
>  
> +static const struct proto_desc proto_metaeth = {
> +	.name		= "ethmeta",
> +	.base		= PROTO_BASE_LL_HDR,
> +	.protocols	= {
> +		PROTO_LINK(__constant_htons(ETH_P_IP),	 &proto_ip),
> +		PROTO_LINK(__constant_htons(ETH_P_ARP),	 &proto_arp),
> +		PROTO_LINK(__constant_htons(ETH_P_IPV6), &proto_ip6),
> +	},
> +	.templates	= {
> +		[0]	= PROTO_META_TEMPLATE("protocol", &ethertype_type, NFT_META_PROTOCOL, 16),
> +	},
> +};
> +
>  /**
>   * payload_gen_dependency - generate match expression on payload dependency
>   *
> @@ -369,6 +383,12 @@ int payload_gen_dependency(struct eval_ctx *ctx, const struct expr *expr,
>  				  "no %s protocol specified",
>  				  proto_base_names[expr->payload.base - 1]);
>  
> +	if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
> +		if (expr->payload.desc == &proto_ip ||
> +		    expr->payload.desc == &proto_ip6)
> +			desc = &proto_metaeth;
> +	}
> +
>  	return payload_add_dependency(ctx, desc, expr->payload.desc, expr, res);
>  }
>  

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[Pablo Neira Ayuso]

Hi Florian,

On Mon, Jun 10, 2019 at 11:44:33AM +0200, Florian Westphal wrote:
> wenxu@ucloud.cn <wenxu@ucloud.cn> wrote:
> > From: wenxu <wenxu@ucloud.cn>
> > 
> > nft add rule bridge firewall rule-100-ingress ip protocol icmp drop
> 
> nft --debug=netlink add rule bridge firewall rule-100-ingress ip protocol icmp drop
> bridge firewall rule-100-ingress
>   [ payload load 2b @ link header + 12 => reg 1 ]
>   [ cmp eq reg 1 0x00000008 ]
>   [ payload load 1b @ network header + 9 => reg 1 ]
>   [ cmp eq reg 1 0x00000001 ]
>   [ immediate reg 0 drop ]
> 
> so problem is that nft inserts a dependency on the ethernet protocol
> type (0x800).
> 
> But when vlan is involved, that will fail to compare.
> 
> It would also fail for qinq etc.
> 
> Because of vlan tag offload, the rule about will probably already work
> just fine when nft userspace is patched to insert the dependency based
> on 'meta protocol'.  Can you see if this patch works?
> 
> Subject: Change bridge l3 dependency to meta protocol
> 
> This examines skb->protocol instead of ethernet header type, which
> might be different when vlan is involved.
> 
> nft payload expression will re-insert the vlan tag so ether type
> will not be ETH_P_IP.
> 
> ---
>  src/meta.c    |  6 +++++-
>  src/payload.c | 20 ++++++++++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/src/meta.c b/src/meta.c
> index 583e790ff47d..1e8964eb48c4 100644
> --- a/src/meta.c
> +++ b/src/meta.c
> @@ -539,7 +539,11 @@ static void meta_expr_pctx_update(struct proto_ctx *ctx,
>  		proto_ctx_update(ctx, PROTO_BASE_TRANSPORT_HDR, &expr->location, desc);
>  		break;
>  	case NFT_META_PROTOCOL:
> -		if (h->base < PROTO_BASE_NETWORK_HDR && ctx->family != NFPROTO_NETDEV)
> +		if (h->base != PROTO_BASE_LL_HDR)
> +			return;
> +
> +		if (ctx->family != NFPROTO_NETDEV &&
> +		    ctx->family != NFPROTO_BRIDGE)
>  			return;
>  
>  		desc = proto_find_upper(h->desc, ntohs(mpz_get_uint16(right->value)));
> diff --git a/src/payload.c b/src/payload.c
> index 6a8118ece890..c99bb2f69977 100644
> --- a/src/payload.c
> +++ b/src/payload.c
> @@ -18,6 +18,7 @@
>  #include <net/if_arp.h>
>  #include <arpa/inet.h>
>  #include <linux/netfilter.h>
> +#include <linux/if_ether.h>
>  
>  #include <rule.h>
>  #include <expression.h>
> @@ -307,6 +308,19 @@ payload_gen_special_dependency(struct eval_ctx *ctx, const struct expr *expr)
>  	return NULL;
>  }
>  
> +static const struct proto_desc proto_metaeth = {
> +	.name		= "ethmeta",
> +	.base		= PROTO_BASE_LL_HDR,
> +	.protocols	= {
> +		PROTO_LINK(__constant_htons(ETH_P_IP),	 &proto_ip),
> +		PROTO_LINK(__constant_htons(ETH_P_ARP),	 &proto_arp),
> +		PROTO_LINK(__constant_htons(ETH_P_IPV6), &proto_ip6),
> +	},
> +	.templates	= {
> +		[0]	= PROTO_META_TEMPLATE("protocol", &ethertype_type, NFT_META_PROTOCOL, 16),
> +	},
> +};
> +
>  /**
>   * payload_gen_dependency - generate match expression on payload dependency
>   *
> @@ -369,6 +383,12 @@ int payload_gen_dependency(struct eval_ctx *ctx, const struct expr *expr,
>  				  "no %s protocol specified",
>  				  proto_base_names[expr->payload.base - 1]);
>  
> +	if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
> +		if (expr->payload.desc == &proto_ip ||
> +		    expr->payload.desc == &proto_ip6)
> +			desc = &proto_metaeth;
> +	}i

Is this sufficient to restrict the matching? Is this still buggy from
ingress?

I wonder if an explicit NFT_PAYLOAD_CHECK_VLAN flag would be useful in
the kernel, if so we could rename NFTA_PAYLOAD_CSUM_FLAGS to
NFTA_PAYLOAD_FLAGS and place it there. Just an idea.

Thanks!

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Subject: Change bridge l3 dependency to meta protocol
> > 
> > This examines skb->protocol instead of ethernet header type, which
> > might be different when vlan is involved.
> >  
> > +	if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
> > +		if (expr->payload.desc == &proto_ip ||
> > +		    expr->payload.desc == &proto_ip6)
> > +			desc = &proto_metaeth;
> > +	}i
> 
> Is this sufficient to restrict the matching? Is this still buggy from
> ingress?

This is what netdev family uses as well (skb->protocol i mean).
I'm not sure it will work for output however (haven't checked).

> I wonder if an explicit NFT_PAYLOAD_CHECK_VLAN flag would be useful in
> the kernel, if so we could rename NFTA_PAYLOAD_CSUM_FLAGS to
> NFTA_PAYLOAD_FLAGS and place it there. Just an idea.

What would NFT_PAYLOAD_CHECK_VLAN do?
You mean disable/enable the 'vlan is there' illusion that nft_payload
provides?  That would work as well of course, but I would prefer to
switch to meta dependencies where possible so we don't rely on
particular layout of a different header class (e.g. meta l4proto doesn't
depend on ip version, and meta protocol won't depend on particular
ethernet frame).

What might be useful is an nft switch to turn off dependeny
insertion, this would also avoid the problem (if users restrict the
matching properly...).

Another unresolved issue is presence of multiple vlan tags, so we might
have to add yet another meta key to retrieve the l3 protocol in use

(the problem at hand was 'ip protocol icmp' not matching traffic inside
 a vlan).

The other issue is lack of vlan awareness in some bridge/netdev
expressions, e.g. reject.

I think we could apply this patch to nft after making sure it works
for output as thats probably the only solution that won't need
changes in the kernel.

If it doesn't, we will need to find a different solution in any case.

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[wenxu]

On 6/18/2019 6:42 AM, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>>> Subject: Change bridge l3 dependency to meta protocol
>>>
>>> This examines skb->protocol instead of ethernet header type, which
>>> might be different when vlan is involved.
>>>  
>>> +	if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
>>> +		if (expr->payload.desc == &proto_ip ||
>>> +		    expr->payload.desc == &proto_ip6)
>>> +			desc = &proto_metaeth;
>>> +	}i
>> Is this sufficient to restrict the matching? Is this still buggy from
>> ingress?
> This is what netdev family uses as well (skb->protocol i mean).
> I'm not sure it will work for output however (haven't checked).
>
>> I wonder if an explicit NFT_PAYLOAD_CHECK_VLAN flag would be useful in
>> the kernel, if so we could rename NFTA_PAYLOAD_CSUM_FLAGS to
>> NFTA_PAYLOAD_FLAGS and place it there. Just an idea.
>
> Another unresolved issue is presence of multiple vlan tags, so we might
> have to add yet another meta key to retrieve the l3 protocol in use

Maybe add a l3proto meta key can handle the multiple vlan tags case with the l3proto dependency.  It

should travese all the vlan tags and find the real l3 proto.


>
>
>

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[Florian Westphal]

wenxu <wenxu@ucloud.cn> wrote:
> On 6/18/2019 6:42 AM, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >>> Subject: Change bridge l3 dependency to meta protocol
> >>>
> >>> This examines skb->protocol instead of ethernet header type, which
> >>> might be different when vlan is involved.
> >>>  
> >>> +	if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
> >>> +		if (expr->payload.desc == &proto_ip ||
> >>> +		    expr->payload.desc == &proto_ip6)
> >>> +			desc = &proto_metaeth;
> >>> +	}i
> >> Is this sufficient to restrict the matching? Is this still buggy from
> >> ingress?
> > This is what netdev family uses as well (skb->protocol i mean).
> > I'm not sure it will work for output however (haven't checked).
> >
> >> I wonder if an explicit NFT_PAYLOAD_CHECK_VLAN flag would be useful in
> >> the kernel, if so we could rename NFTA_PAYLOAD_CSUM_FLAGS to
> >> NFTA_PAYLOAD_FLAGS and place it there. Just an idea.
> >
> > Another unresolved issue is presence of multiple vlan tags, so we might
> > have to add yet another meta key to retrieve the l3 protocol in use
> 
> Maybe add a l3proto meta key can handle the multiple vlan tags case with the l3proto dependency.  It
> should travese all the vlan tags and find the real l3 proto.

Yes, something like this.

We also need to audit netdev and bridge expressions (reject is known broken)
to handle vlans properly.

Still, switching nft to prefer skb->protocol instead of eth_hdr->type
for dependencies would be good as this doesn't need kernel changes and solves
the immediate problem of 'ip ...' not matching in case of vlan.

If you have time, could you check if using skb->protocol works for nft
bridge in output, i.e. does 'nft ip protocol icmp' match when its used
from bridge output path with meta protocol dependency with and without
vlan in use?

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[wenxu]

在 2019/6/18 17:37, Florian Westphal 写道:
> wenxu <wenxu@ucloud.cn> wrote:
>> On 6/18/2019 6:42 AM, Florian Westphal wrote:
>>> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>>>>> Subject: Change bridge l3 dependency to meta protocol
>>>>>
>>>>> This examines skb->protocol instead of ethernet header type, which
>>>>> might be different when vlan is involved.
>>>>>  
>>>>> +	if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
>>>>> +		if (expr->payload.desc == &proto_ip ||
>>>>> +		    expr->payload.desc == &proto_ip6)
>>>>> +			desc = &proto_metaeth;
>>>>> +	}i
>>>> Is this sufficient to restrict the matching? Is this still buggy from
>>>> ingress?
>>> This is what netdev family uses as well (skb->protocol i mean).
>>> I'm not sure it will work for output however (haven't checked).
>>>
>>>> I wonder if an explicit NFT_PAYLOAD_CHECK_VLAN flag would be useful in
>>>> the kernel, if so we could rename NFTA_PAYLOAD_CSUM_FLAGS to
>>>> NFTA_PAYLOAD_FLAGS and place it there. Just an idea.
>>> Another unresolved issue is presence of multiple vlan tags, so we might
>>> have to add yet another meta key to retrieve the l3 protocol in use
>> Maybe add a l3proto meta key can handle the multiple vlan tags case with the l3proto dependency.  It
>> should travese all the vlan tags and find the real l3 proto.
> Yes, something like this.
>
> We also need to audit netdev and bridge expressions (reject is known broken)
> to handle vlans properly.
>
> Still, switching nft to prefer skb->protocol instead of eth_hdr->type
> for dependencies would be good as this doesn't need kernel changes and solves
> the immediate problem of 'ip ...' not matching in case of vlan.
>
> If you have time, could you check if using skb->protocol works for nft
> bridge in output, i.e. does 'nft ip protocol icmp' match when its used
> from bridge output path with meta protocol dependency with and without
> vlan in use?

I just check the kernel codes and test with the output chain, the meta protocol dependency can

also work in the outpu chain.

But this patch can't resolve the multiple vlan tags, It need another meta l3proto which do care about

how many vlan tags in the frame.

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[Pablo Neira Ayuso]

On Tue, Jun 18, 2019 at 10:27:12PM +0800, wenxu wrote:
> 
> 在 2019/6/18 17:37, Florian Westphal 写道:
> > wenxu <wenxu@ucloud.cn> wrote:
> >> On 6/18/2019 6:42 AM, Florian Westphal wrote:
> >>> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >>>>> Subject: Change bridge l3 dependency to meta protocol
> >>>>>
> >>>>> This examines skb->protocol instead of ethernet header type, which
> >>>>> might be different when vlan is involved.
> >>>>>  
> >>>>> +	if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
> >>>>> +		if (expr->payload.desc == &proto_ip ||
> >>>>> +		    expr->payload.desc == &proto_ip6)
> >>>>> +			desc = &proto_metaeth;
> >>>>> +	}i
> >>>> Is this sufficient to restrict the matching? Is this still buggy from
> >>>> ingress?
> >>> This is what netdev family uses as well (skb->protocol i mean).
> >>> I'm not sure it will work for output however (haven't checked).
> >>>
> >>>> I wonder if an explicit NFT_PAYLOAD_CHECK_VLAN flag would be useful in
> >>>> the kernel, if so we could rename NFTA_PAYLOAD_CSUM_FLAGS to
> >>>> NFTA_PAYLOAD_FLAGS and place it there. Just an idea.
> >>> Another unresolved issue is presence of multiple vlan tags, so we might
> >>> have to add yet another meta key to retrieve the l3 protocol in use
> >> Maybe add a l3proto meta key can handle the multiple vlan tags case with the l3proto dependency.  It
> >> should travese all the vlan tags and find the real l3 proto.
> > Yes, something like this.
> >
> > We also need to audit netdev and bridge expressions (reject is known broken)
> > to handle vlans properly.
> >
> > Still, switching nft to prefer skb->protocol instead of eth_hdr->type
> > for dependencies would be good as this doesn't need kernel changes and solves
> > the immediate problem of 'ip ...' not matching in case of vlan.
> >
> > If you have time, could you check if using skb->protocol works for nft
> > bridge in output, i.e. does 'nft ip protocol icmp' match when its used
> > from bridge output path with meta protocol dependency with and without
> > vlan in use?
> 
> I just check the kernel codes and test with the output chain, the
> meta protocol dependency can also work in the outpu chain.

OK.

Florian, would you submit a patch, including a test for this?

Thanks!

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[Pablo Neira Ayuso]

On Tue, Jun 18, 2019 at 12:42:32AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Subject: Change bridge l3 dependency to meta protocol
> > > 
> > > This examines skb->protocol instead of ethernet header type, which
> > > might be different when vlan is involved.
> > >  
> > > +	if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
> > > +		if (expr->payload.desc == &proto_ip ||
> > > +		    expr->payload.desc == &proto_ip6)
> > > +			desc = &proto_metaeth;
> > > +	}i
> > 
> > Is this sufficient to restrict the matching? Is this still buggy from
> > ingress?
> 
> This is what netdev family uses as well (skb->protocol i mean).
> I'm not sure it will work for output however (haven't checked).

You mean for locally generated traffic?

> > I wonder if an explicit NFT_PAYLOAD_CHECK_VLAN flag would be useful in
> > the kernel, if so we could rename NFTA_PAYLOAD_CSUM_FLAGS to
> > NFTA_PAYLOAD_FLAGS and place it there. Just an idea.
> 
> What would NFT_PAYLOAD_CHECK_VLAN do?

Similar to the checksum approach, it provides a hint to the kernel to
say that "I want to look at the vlan header" from the link layer.

> You mean disable/enable the 'vlan is there' illusion that nft_payload
> provides?  That would work as well of course, but I would prefer to
> switch to meta dependencies where possible so we don't rely on
> particular layout of a different header class (e.g. meta l4proto doesn't
> depend on ip version, and meta protocol won't depend on particular
> ethernet frame).

If we can fix all cases from userspace, that's fine.

> What might be useful is an nft switch to turn off dependeny
> insertion, this would also avoid the problem (if users restrict the
> matching properly...).

Hm. How does this toggle would look like?

> Another unresolved issue is presence of multiple vlan tags, so we might
> have to add yet another meta key to retrieve the l3 protocol in use
> 
> (the problem at hand was 'ip protocol icmp' not matching traffic inside
>  a vlan).

Could you describe this problem a bit more? Small example rule plus
scenario.

> The other issue is lack of vlan awareness in some bridge/netdev
> expressions, e.g. reject.

This needs to be fixed for bridge. There is no support for netdev yet,
IIRC.

> I think we could apply this patch to nft after making sure it works
> for output as thats probably the only solution that won't need
> changes in the kernel.

That's fine with me.

> If it doesn't, we will need to find a different solution in any case.

OK.

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Jun 18, 2019 at 12:42:32AM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > Subject: Change bridge l3 dependency to meta protocol
> > > > 
> > > > This examines skb->protocol instead of ethernet header type, which
> > > > might be different when vlan is involved.
> > > >  
> > > > +	if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
> > > > +		if (expr->payload.desc == &proto_ip ||
> > > > +		    expr->payload.desc == &proto_ip6)
> > > > +			desc = &proto_metaeth;
> > > > +	}i
> > > 
> > > Is this sufficient to restrict the matching? Is this still buggy from
> > > ingress?
> > 
> > This is what netdev family uses as well (skb->protocol i mean).
> > I'm not sure it will work for output however (haven't checked).
> 
> You mean for locally generated traffic?

Yes.

> > > I wonder if an explicit NFT_PAYLOAD_CHECK_VLAN flag would be useful in
> > > the kernel, if so we could rename NFTA_PAYLOAD_CSUM_FLAGS to
> > > NFTA_PAYLOAD_FLAGS and place it there. Just an idea.
> > 
> > What would NFT_PAYLOAD_CHECK_VLAN do?
> 
> Similar to the checksum approach, it provides a hint to the kernel to
> say that "I want to look at the vlan header" from the link layer.

I see.  Its a bit of a furhter problem because tags can be nested,
so we would have to provide a more dynamic approach, similar to tunnel
matching (vlan header 0 id 42 vlan header 1 id 23' etc).

> > What might be useful is an nft switch to turn off dependeny
> > insertion, this would also avoid the problem (if users restrict the
> > matching properly...).
> 
> Hm. How does this toggle would look like?

nft --nodep add rule bridge filter input ip protocol icmp # broken, has false positives
nft --nodep add rule bridge filter input ip version 4 ip protocol icmp # might work
nft --nodep add rule bridge filter input meta protocol ip ip protocol icmp # might work too

Its kind of I-Know-What-I-Am-Doing switch ...

We can already do this with raw payload expressions but those aren't that user
friendly.

> > Another unresolved issue is presence of multiple vlan tags, so we might
> > have to add yet another meta key to retrieve the l3 protocol in use
> > 
> > (the problem at hand was 'ip protocol icmp' not matching traffic inside
> >  a vlan).
> 
> Could you describe this problem a bit more? Small example rule plus
> scenario.

It was what wenxu reported originally:

nft add rule bridge filter forward ip protocol counter ..

The rule only matches if the ip packet is contained in an ethernet frame
without vlan tag -- and thats neither expected nor desirable.

This rule works when using 'meta protocol ip' as dependency instead
of ether type ip (what we do now), because VLAN stripping will fix/alter
skb->protocol to the inner type when the VLAN tag gets removes.

It will still fail in case there are several VLAN tags, so we might
need another meta expression that can figure out the l3 protocol type.

Does that make sense so far?

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[Pablo Neira Ayuso]

On Tue, Jun 18, 2019 at 11:46:13AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
[...]
> > Could you describe this problem a bit more? Small example rule plus
> > scenario.
> 
> It was what wenxu reported originally:
> 
> nft add rule bridge filter forward ip protocol counter ..
> 
> The rule only matches if the ip packet is contained in an ethernet frame
> without vlan tag -- and thats neither expected nor desirable.
> 
> This rule works when using 'meta protocol ip' as dependency instead
> of ether type ip (what we do now), because VLAN stripping will fix/alter
> skb->protocol to the inner type when the VLAN tag gets removes.
> 
> It will still fail in case there are several VLAN tags, so we might
> need another meta expression that can figure out the l3 protocol type.

How would that new meta expression would look like?

Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Jun 18, 2019 at 11:46:13AM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> [...]
> > > Could you describe this problem a bit more? Small example rule plus
> > > scenario.
> > 
> > It was what wenxu reported originally:
> > 
> > nft add rule bridge filter forward ip protocol counter ..
> > 
> > The rule only matches if the ip packet is contained in an ethernet frame
> > without vlan tag -- and thats neither expected nor desirable.
> > 
> > This rule works when using 'meta protocol ip' as dependency instead
> > of ether type ip (what we do now), because VLAN stripping will fix/alter
> > skb->protocol to the inner type when the VLAN tag gets removes.
> > 
> > It will still fail in case there are several VLAN tags, so we might
> > need another meta expression that can figure out the l3 protocol type.
> 
> How would that new meta expression would look like?

I thought about extending nft_exthdr.c for L2, i.e. take
ether->type, and then advance to next vlan header (if vlan type)
until it either reaches skb network offset or an unknown type
(which would then be considered the last/topmost one and the one
 carrying the l3 protocol number).