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

[PATCH net-next v2 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 user-specified 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.

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 use it
  net: sched: em_ipt: add support for addrtype matching

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

-- 
2.20.1

[PATCH net-next v2 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>
---
 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.20.1

[PATCH net-next v2 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>
---
 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.20.1

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

[Nikolay Aleksandrov]

For NFPROTO_UNSPEC xt_matches there's no way to restrict the matching to a
specific family, in order to do so we record the user-specified family
and later enforce it while doing the match.

v2: adjust changes to missing patch, was patch 04 in v1

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/sched/em_ipt.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index fd7f5b288c31..ce91f3cea0bd 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,16 @@ 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:
+	case NFPROTO_UNSPEC:
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	match = get_xt_match(tb);
 	if (IS_ERR(match)) {
 		pr_err("unable to load match\n");
@@ -140,6 +152,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);
@@ -182,8 +195,8 @@ 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;
+	u8 nfproto = im->nfproto;
 	int ret;
 
 	switch (tc_skb_protocol(skb)) {
@@ -231,7 +244,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.20.1

[PATCH net-next v2 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>
---
 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 ce91f3cea0bd..b08d87bd120b 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.20.1

Re: [PATCH net-next v2 3/4] net: sched: em_ipt: keep the user-specified nfproto and use it

[Eyal Birger]

Hi Nik,

On Wed, 26 Jun 2019 18:56:14 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> For NFPROTO_UNSPEC xt_matches there's no way to restrict the matching
> to a specific family, in order to do so we record the user-specified
> family and later enforce it while doing the match.
> 
> v2: adjust changes to missing patch, was patch 04 in v1
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
>  net/sched/em_ipt.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
..snip..
> @@ -182,8 +195,8 @@ 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;
> +	u8 nfproto = im->nfproto;

Maybe I'm missing something now - but it's not really clear to me now
why keeping im->nfproto would be useful:

If NFPROTO_UNSPEC was provided by userspace then the actual nfproto used
will be taken from the packet, and if NFPROTO_IPV4/IPV6 was specified
from userspace then it will equal im->match->family.

Is there any case where the resulting nfproto would differ as a result
of this patch?

Otherwise the patchset looks excellent to me.

Thanks!
Eyal.

Re: [PATCH net-next v2 3/4] net: sched: em_ipt: keep the user-specified nfproto and use it

[nikolay]

On 26 June 2019 19:18:35 EEST, Eyal Birger <eyal.birger@gmail.com> wrote:
>Hi Nik,
>
>On Wed, 26 Jun 2019 18:56:14 +0300
>Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>
>> For NFPROTO_UNSPEC xt_matches there's no way to restrict the matching
>> to a specific family, in order to do so we record the user-specified
>> family and later enforce it while doing the match.
>> 
>> v2: adjust changes to missing patch, was patch 04 in v1
>> 
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> ---
>>  net/sched/em_ipt.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>> 
>..snip..
>> @@ -182,8 +195,8 @@ 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;
>> +	u8 nfproto = im->nfproto;
>
>Maybe I'm missing something now - but it's not really clear to me now
>why keeping im->nfproto would be useful:
>
>If NFPROTO_UNSPEC was provided by userspace then the actual nfproto
>used
>will be taken from the packet, and if NFPROTO_IPV4/IPV6 was specified
>from userspace then it will equal im->match->family.
>
>Is there any case where the resulting nfproto would differ as a result
>of this patch?
>
>Otherwise the patchset looks excellent to me.
>
>Thanks!
>Eyal.

Hi,
It's needed to limit the match only to the user-specified family
for unspec xt matches. The problem is otherwise im->match->family
stays at nfproto_unspec regardless of the user choice.

Thanks for reviewing the set. 

Cheers,
  Nik

Re: [PATCH net-next v2 3/4] net: sched: em_ipt: keep the user-specified nfproto and use it

[nikolay]

On 26 June 2019 19:33:48 EEST, nikolay@cumulusnetworks.com wrote:
>On 26 June 2019 19:18:35 EEST, Eyal Birger <eyal.birger@gmail.com>
>wrote:
>>Hi Nik,
>>
>>On Wed, 26 Jun 2019 18:56:14 +0300
>>Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>
>>> For NFPROTO_UNSPEC xt_matches there's no way to restrict the
>matching
>>> to a specific family, in order to do so we record the user-specified
>>> family and later enforce it while doing the match.
>>> 
>>> v2: adjust changes to missing patch, was patch 04 in v1
>>> 
>>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>> ---
>>>  net/sched/em_ipt.c | 17 +++++++++++++++--
>>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>> 
>>..snip..
>>> @@ -182,8 +195,8 @@ 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;
>>> +	u8 nfproto = im->nfproto;
>>
>>Maybe I'm missing something now - but it's not really clear to me now
>>why keeping im->nfproto would be useful:
>>
>>If NFPROTO_UNSPEC was provided by userspace then the actual nfproto
>>used
>>will be taken from the packet, and if NFPROTO_IPV4/IPV6 was specified
>>from userspace then it will equal im->match->family.
>>
>>Is there any case where the resulting nfproto would differ as a result
>>of this patch?
>>
>>Otherwise the patchset looks excellent to me.
>>
>>Thanks!
>>Eyal.
>
>Hi,
>It's needed to limit the match only to the user-specified family
>for unspec xt matches. The problem is otherwise im->match->family
>stays at nfproto_unspec regardless of the user choice.
>
>Thanks for reviewing the set. 
>
>Cheers,
>  Nik

Hm, while that is true, thinking more about it - mixing the user proto and the real proto
could be problematic since we no longer enforce them to be equal, but we check
the network header len based on the packet only and we can end up checking v4
len and parsing it as nfproto v6. 

I'll spin v3 with unspec only and we can restrict it later if needed.