[PATCH net-next v3 0/4] em_ipt: add support for addrtype

[PATCH net-next v3 0/4] em_ipt: add support for addrtype

[Nikolay Aleksandrov]

Hi,
We would like to be able to use the addrtype from tc for ACL rules and
em_ipt seems the best place to add support for the already existing xt
match. The biggest issue is that addrtype revision 1 (with ipv6 support)
is NFPROTO_UNSPEC and currently em_ipt can't differentiate between v4/v6
if such xt match is used because it passes the match's family instead of
the packet one. The first 3 patches make em_ipt match only on IP
traffic (currently both policy and addrtype recognize such traffic
only) and make it pass the actual packet's protocol instead of the xt
match family when it's unspecified. They also add support for NFPROTO_UNSPEC
xt matches. The last patch allows to add addrtype rules via em_ipt.
We need to keep the user-specified nfproto for dumping in order to be
compatible with libxtables, we cannot dump NFPROTO_UNSPEC as the nfproto
or we'll get an error from libxtables, thus the nfproto is limited to
ipv4/ipv6 in patch 03 and is recorded.

v3: don't use the user nfproto for matching, only for dumping, more
    information is available in the commit message in patch 03
v2: change patch 02 to set the nfproto only when unspecified and drop
    patch 04 from v1 (Eyal Birger)

Thank you,
  Nikolay Aleksandrov


Nikolay Aleksandrov (4):
  net: sched: em_ipt: match only on ip/ipv6 traffic
  net: sched: em_ipt: set the family based on the packet if it's
    unspecified
  net: sched: em_ipt: keep the user-specified nfproto and dump it
  net: sched: em_ipt: add support for addrtype matching

 net/sched/em_ipt.c | 48 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

-- 
2.21.0

[PATCH net-next v3 1/4] net: sched: em_ipt: match only on ip/ipv6 traffic

[Nikolay Aleksandrov]

Restrict matching only to ip/ipv6 traffic and make sure we can use the
headers, otherwise matches will be attempted on any protocol which can
be unexpected by the xt matches. Currently policy supports only ipv4/6.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
v3: no change
v2: no change

 net/sched/em_ipt.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index 243fd22f2248..64dbafe4e94c 100644
--- a/net/sched/em_ipt.c
+++ b/net/sched/em_ipt.c
@@ -185,6 +185,19 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em,
 	struct nf_hook_state state;
 	int ret;
 
+	switch (tc_skb_protocol(skb)) {
+	case htons(ETH_P_IP):
+		if (!pskb_network_may_pull(skb, sizeof(struct iphdr)))
+			return 0;
+		break;
+	case htons(ETH_P_IPV6):
+		if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr)))
+			return 0;
+		break;
+	default:
+		return 0;
+	}
+
 	rcu_read_lock();
 
 	if (skb->skb_iif)
-- 
2.21.0

[PATCH net-next v3 2/4] net: sched: em_ipt: set the family based on the packet if it's unspecified

[Nikolay Aleksandrov]

Set the family based on the packet if it's unspecified otherwise
protocol-neutral matches will have wrong information (e.g. NFPROTO_UNSPEC).
In preparation for using NFPROTO_UNSPEC xt matches.

v2: set the nfproto only when unspecified

Suggested-by: Eyal Birger <eyal.birger@gmail.com>
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
v3: no change

 net/sched/em_ipt.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index 64dbafe4e94c..fd7f5b288c31 100644
--- a/net/sched/em_ipt.c
+++ b/net/sched/em_ipt.c
@@ -182,6 +182,7 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em,
 	const struct em_ipt_match *im = (const void *)em->data;
 	struct xt_action_param acpar = {};
 	struct net_device *indev = NULL;
+	u8 nfproto = im->match->family;
 	struct nf_hook_state state;
 	int ret;
 
@@ -189,10 +190,14 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em,
 	case htons(ETH_P_IP):
 		if (!pskb_network_may_pull(skb, sizeof(struct iphdr)))
 			return 0;
+		if (nfproto == NFPROTO_UNSPEC)
+			nfproto = NFPROTO_IPV4;
 		break;
 	case htons(ETH_P_IPV6):
 		if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr)))
 			return 0;
+		if (nfproto == NFPROTO_UNSPEC)
+			nfproto = NFPROTO_IPV6;
 		break;
 	default:
 		return 0;
@@ -203,7 +208,7 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em,
 	if (skb->skb_iif)
 		indev = dev_get_by_index_rcu(em->net, skb->skb_iif);
 
-	nf_hook_state_init(&state, im->hook, im->match->family,
+	nf_hook_state_init(&state, im->hook, nfproto,
 			   indev ?: skb->dev, skb->dev, NULL, em->net, NULL);
 
 	acpar.match = im->match;
-- 
2.21.0

[PATCH net-next v3 3/4] net: sched: em_ipt: keep the user-specified nfproto and dump it

[Nikolay Aleksandrov]

If we dump NFPROTO_UNSPEC as nfproto user-space libxtables can't handle
it and would exit with an error like:
"libxtables: unhandled NFPROTO in xtables_set_nfproto"
In order to avoid the error return the user-specified nfproto. If we
don't record it then the match family is used which can be
NFPROTO_UNSPEC. Even if we add support to mask NFPROTO_UNSPEC in
iproute2 we have to be compatible with older versions which would be
also be allowed to add NFPROTO_UNSPEC matches (e.g. addrtype after the
last patch).

v3: don't use the user nfproto for matching, only for dumping the rule,
    also don't allow the nfproto to be unspecified (explained above)
v2: adjust changes to missing patch, was patch 04 in v1

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
Unfortunately we still have to save the user-nfproto for dumping
otherwise we'll break user-space because it can add a rule which it
won't be able to dump later and in fact will terminate the whole dump.
I also thought about masking it but that seems more hacky, I'd prefer
to return an expected value which was passed when the rule was created.

 net/sched/em_ipt.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index fd7f5b288c31..3c356d6f719a 100644
--- a/net/sched/em_ipt.c
+++ b/net/sched/em_ipt.c
@@ -21,6 +21,7 @@
 struct em_ipt_match {
 	const struct xt_match *match;
 	u32 hook;
+	u8 nfproto;
 	u8 match_data[0] __aligned(8);
 };
 
@@ -115,6 +116,7 @@ static int em_ipt_change(struct net *net, void *data, int data_len,
 	struct em_ipt_match *im = NULL;
 	struct xt_match *match;
 	int mdata_len, ret;
+	u8 nfproto;
 
 	ret = nla_parse_deprecated(tb, TCA_EM_IPT_MAX, data, data_len,
 				   em_ipt_policy, NULL);
@@ -125,6 +127,15 @@ static int em_ipt_change(struct net *net, void *data, int data_len,
 	    !tb[TCA_EM_IPT_MATCH_DATA] || !tb[TCA_EM_IPT_NFPROTO])
 		return -EINVAL;
 
+	nfproto = nla_get_u8(tb[TCA_EM_IPT_NFPROTO]);
+	switch (nfproto) {
+	case NFPROTO_IPV4:
+	case NFPROTO_IPV6:
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	match = get_xt_match(tb);
 	if (IS_ERR(match)) {
 		pr_err("unable to load match\n");
@@ -140,6 +151,7 @@ static int em_ipt_change(struct net *net, void *data, int data_len,
 
 	im->match = match;
 	im->hook = nla_get_u32(tb[TCA_EM_IPT_HOOK]);
+	im->nfproto = nfproto;
 	nla_memcpy(im->match_data, tb[TCA_EM_IPT_MATCH_DATA], mdata_len);
 
 	ret = check_match(net, im, mdata_len);
@@ -231,7 +243,7 @@ static int em_ipt_dump(struct sk_buff *skb, struct tcf_ematch *em)
 		return -EMSGSIZE;
 	if (nla_put_u8(skb, TCA_EM_IPT_MATCH_REVISION, im->match->revision) < 0)
 		return -EMSGSIZE;
-	if (nla_put_u8(skb, TCA_EM_IPT_NFPROTO, im->match->family) < 0)
+	if (nla_put_u8(skb, TCA_EM_IPT_NFPROTO, im->nfproto) < 0)
 		return -EMSGSIZE;
 	if (nla_put(skb, TCA_EM_IPT_MATCH_DATA,
 		    im->match->usersize ?: im->match->matchsize,
-- 
2.21.0

[PATCH net-next v3 4/4] net: sched: em_ipt: add support for addrtype matching

[Nikolay Aleksandrov]

Allow em_ipt to use addrtype for matching. Restrict the use only to
revision 1 which has IPv6 support. Since it's a NFPROTO_UNSPEC xt match
we use the user-specified nfproto for matching, in case it's unspecified
both v4/v6 will be matched by the rule.

v2: no changes, was patch 5 in v1

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
v3: no changes

 net/sched/em_ipt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index 3c356d6f719a..9fff6480acc6 100644
--- a/net/sched/em_ipt.c
+++ b/net/sched/em_ipt.c
@@ -72,11 +72,25 @@ static int policy_validate_match_data(struct nlattr **tb, u8 mrev)
 	return 0;
 }
 
+static int addrtype_validate_match_data(struct nlattr **tb, u8 mrev)
+{
+	if (mrev != 1) {
+		pr_err("only addrtype match revision 1 supported");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct em_ipt_xt_match em_ipt_xt_matches[] = {
 	{
 		.match_name = "policy",
 		.validate_match_data = policy_validate_match_data
 	},
+	{
+		.match_name = "addrtype",
+		.validate_match_data = addrtype_validate_match_data
+	},
 	{}
 };
 
-- 
2.21.0

Re: [PATCH net-next v3 0/4] em_ipt: add support for addrtype

[Eyal Birger]

On Thu, 27 Jun 2019 11:10:43 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> Hi,
> We would like to be able to use the addrtype from tc for ACL rules and
> em_ipt seems the best place to add support for the already existing xt
> match. The biggest issue is that addrtype revision 1 (with ipv6
> support) is NFPROTO_UNSPEC and currently em_ipt can't differentiate
> between v4/v6 if such xt match is used because it passes the match's
> family instead of the packet one. The first 3 patches make em_ipt
> match only on IP traffic (currently both policy and addrtype
> recognize such traffic only) and make it pass the actual packet's
> protocol instead of the xt match family when it's unspecified. They
> also add support for NFPROTO_UNSPEC xt matches. The last patch allows
> to add addrtype rules via em_ipt. We need to keep the user-specified
> nfproto for dumping in order to be compatible with libxtables, we
> cannot dump NFPROTO_UNSPEC as the nfproto or we'll get an error from
> libxtables, thus the nfproto is limited to ipv4/ipv6 in patch 03 and
> is recorded.
> 
> v3: don't use the user nfproto for matching, only for dumping, more
>     information is available in the commit message in patch 03
> v2: change patch 02 to set the nfproto only when unspecified and drop
>     patch 04 from v1 (Eyal Birger)
> 
> Thank you,
>   Nikolay Aleksandrov
> 
> 
> Nikolay Aleksandrov (4):
>   net: sched: em_ipt: match only on ip/ipv6 traffic
>   net: sched: em_ipt: set the family based on the packet if it's
>     unspecified
>   net: sched: em_ipt: keep the user-specified nfproto and dump it
>   net: sched: em_ipt: add support for addrtype matching
> 
>  net/sched/em_ipt.c | 48
> ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 46
> insertions(+), 2 deletions(-)
> 

Looks great! thanks for adding this!

For the series:

Acked-by: Eyal Birger <eyal.birger@gmail.com>

Re: [PATCH net-next v3 1/4] net: sched: em_ipt: match only on ip/ipv6 traffic

[Eyal Birger]

Hi Nik,

On Thu, 27 Jun 2019 11:10:44 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> Restrict matching only to ip/ipv6 traffic and make sure we can use the
> headers, otherwise matches will be attempted on any protocol which can
> be unexpected by the xt matches. Currently policy supports only
> ipv4/6.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
> v3: no change
> v2: no change
> 
>  net/sched/em_ipt.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
> index 243fd22f2248..64dbafe4e94c 100644
> --- a/net/sched/em_ipt.c
> +++ b/net/sched/em_ipt.c
> @@ -185,6 +185,19 @@ static int em_ipt_match(struct sk_buff *skb,
> struct tcf_ematch *em, struct nf_hook_state state;
>  	int ret;
>  
> +	switch (tc_skb_protocol(skb)) {
> +	case htons(ETH_P_IP):
> +		if (!pskb_network_may_pull(skb, sizeof(struct
> iphdr)))
> +			return 0;
> +		break;
> +	case htons(ETH_P_IPV6):
> +		if (!pskb_network_may_pull(skb, sizeof(struct
> ipv6hdr)))
> +			return 0;
> +		break;
> +	default:
> +		return 0;
> +	}
> +

I just realized that I didn't consider the egress direction in my review.
Don't we need an skb_pull() in that direction to make the skb->data point
to L3? I see this is done e.g. in em_ipset.

Eyal.

Re: [PATCH net-next v3 1/4] net: sched: em_ipt: match only on ip/ipv6 traffic

[nikolay]

On 27 June 2019 19:02:37 EEST, Eyal Birger <eyal.birger@gmail.com> wrote:
>Hi Nik,
>
>On Thu, 27 Jun 2019 11:10:44 +0300
>Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>
>> Restrict matching only to ip/ipv6 traffic and make sure we can use
>the
>> headers, otherwise matches will be attempted on any protocol which
>can
>> be unexpected by the xt matches. Currently policy supports only
>> ipv4/6.
>> 
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> ---
>> v3: no change
>> v2: no change
>> 
>>  net/sched/em_ipt.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>> 
>> diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
>> index 243fd22f2248..64dbafe4e94c 100644
>> --- a/net/sched/em_ipt.c
>> +++ b/net/sched/em_ipt.c
>> @@ -185,6 +185,19 @@ static int em_ipt_match(struct sk_buff *skb,
>> struct tcf_ematch *em, struct nf_hook_state state;
>>  	int ret;
>>  
>> +	switch (tc_skb_protocol(skb)) {
>> +	case htons(ETH_P_IP):
>> +		if (!pskb_network_may_pull(skb, sizeof(struct
>> iphdr)))
>> +			return 0;
>> +		break;
>> +	case htons(ETH_P_IPV6):
>> +		if (!pskb_network_may_pull(skb, sizeof(struct
>> ipv6hdr)))
>> +			return 0;
>> +		break;
>> +	default:
>> +		return 0;
>> +	}
>> +
>
>I just realized that I didn't consider the egress direction in my
>review.
>Don't we need an skb_pull() in that direction to make the skb->data
>point
>to L3? I see this is done e.g. in em_ipset.
>
>Eyal.

Hi Eyal,
Not for addrtype, it doesn't have such expectations.
I also tested it, everything matches properly.

Cheers,
  Nik

Re: [PATCH net-next v3 0/4] em_ipt: add support for addrtype

[David Miller]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Thu, 27 Jun 2019 11:10:43 +0300

> We would like to be able to use the addrtype from tc for ACL rules and
> em_ipt seems the best place to add support for the already existing xt
> match. The biggest issue is that addrtype revision 1 (with ipv6 support)
> is NFPROTO_UNSPEC and currently em_ipt can't differentiate between v4/v6
> if such xt match is used because it passes the match's family instead of
> the packet one. The first 3 patches make em_ipt match only on IP
> traffic (currently both policy and addrtype recognize such traffic
> only) and make it pass the actual packet's protocol instead of the xt
> match family when it's unspecified. They also add support for NFPROTO_UNSPEC
> xt matches. The last patch allows to add addrtype rules via em_ipt.
> We need to keep the user-specified nfproto for dumping in order to be
> compatible with libxtables, we cannot dump NFPROTO_UNSPEC as the nfproto
> or we'll get an error from libxtables, thus the nfproto is limited to
> ipv4/ipv6 in patch 03 and is recorded.
> 
> v3: don't use the user nfproto for matching, only for dumping, more
>     information is available in the commit message in patch 03
> v2: change patch 02 to set the nfproto only when unspecified and drop
>     patch 04 from v1 (Eyal Birger)

Series applied, thanks Nikolay.