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

[PATCH net-next 0/5] 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 4 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. They also add support for NFPROTO_UNSPEC xt matches.
The last patch allows to add addrtype rules via em_ipt.


Thank you,
  Nikolay Aleksandrov

Nikolay Aleksandrov (5):
  net: sched: em_ipt: match only on ip/ipv6 traffic
  net: sched: em_ipt: set the family based on the protocol when matching
  net: sched: em_ipt: restrict matching to the respective protocol
  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 | 52 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH net-next 1/5] 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 2/5] net: sched: em_ipt: set the family based on the protocol when matching

[Nikolay Aleksandrov]

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

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

diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index 64dbafe4e94c..23965a071177 100644
--- a/net/sched/em_ipt.c
+++ b/net/sched/em_ipt.c
@@ -189,10 +189,12 @@ 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;
+		state.pf = NFPROTO_IPV4;
 		break;
 	case htons(ETH_P_IPV6):
 		if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr)))
 			return 0;
+		state.pf = NFPROTO_IPV6;
 		break;
 	default:
 		return 0;
@@ -203,7 +205,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, state.pf,
 			   indev ?: skb->dev, skb->dev, NULL, em->net, NULL);
 
 	acpar.match = im->match;
-- 
2.20.1

[PATCH net-next 3/5] net: sched: em_ipt: restrict matching to the respective protocol

[Nikolay Aleksandrov]

Currently a match will continue even if the user-specified nfproto
doesn't match the packet's, so restrict it only to when they're equal or
the protocol is unspecified.

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

diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index 23965a071177..d4257f5f1d94 100644
--- a/net/sched/em_ipt.c
+++ b/net/sched/em_ipt.c
@@ -187,11 +187,17 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em,
 
 	switch (tc_skb_protocol(skb)) {
 	case htons(ETH_P_IP):
+		if (im->match->family != NFPROTO_UNSPEC &&
+		    im->match->family != NFPROTO_IPV4)
+			return 0;
 		if (!pskb_network_may_pull(skb, sizeof(struct iphdr)))
 			return 0;
 		state.pf = NFPROTO_IPV4;
 		break;
 	case htons(ETH_P_IPV6):
+		if (im->match->family != NFPROTO_UNSPEC &&
+		    im->match->family != NFPROTO_IPV6)
+			return 0;
 		if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr)))
 			return 0;
 		state.pf = NFPROTO_IPV6;
-- 
2.20.1

[PATCH net-next 4/5] 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.

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

diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index d4257f5f1d94..cfb93ce340da 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);
@@ -187,16 +200,16 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em,
 
 	switch (tc_skb_protocol(skb)) {
 	case htons(ETH_P_IP):
-		if (im->match->family != NFPROTO_UNSPEC &&
-		    im->match->family != NFPROTO_IPV4)
+		if (im->nfproto != NFPROTO_UNSPEC &&
+		    im->nfproto != NFPROTO_IPV4)
 			return 0;
 		if (!pskb_network_may_pull(skb, sizeof(struct iphdr)))
 			return 0;
 		state.pf = NFPROTO_IPV4;
 		break;
 	case htons(ETH_P_IPV6):
-		if (im->match->family != NFPROTO_UNSPEC &&
-		    im->match->family != NFPROTO_IPV6)
+		if (im->nfproto != NFPROTO_UNSPEC &&
+		    im->nfproto != NFPROTO_IPV6)
 			return 0;
 		if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr)))
 			return 0;
@@ -234,7 +247,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 5/5] 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.

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 cfb93ce340da..ce0798f6f1f7 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 2/5] net: sched: em_ipt: set the family based on the protocol when matching

[Eyal Birger]

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

> Set the family based on the protocol otherwise protocol-neutral
> matches will have wrong information (e.g. NFPROTO_UNSPEC). In
> preparation for using NFPROTO_UNSPEC xt matches.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
>  net/sched/em_ipt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
> index 64dbafe4e94c..23965a071177 100644
> --- a/net/sched/em_ipt.c
> +++ b/net/sched/em_ipt.c
> @@ -189,10 +189,12 @@ 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;
> +		state.pf = NFPROTO_IPV4;
>  		break;
>  	case htons(ETH_P_IPV6):
>  		if (!pskb_network_may_pull(skb, sizeof(struct
> ipv6hdr))) return 0;
> +		state.pf = NFPROTO_IPV6;
>  		break;
>  	default:
>  		return 0;
> @@ -203,7 +205,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, state.pf,
>  			   indev ?: skb->dev, skb->dev, NULL,
> em->net, NULL); 
>  	acpar.match = im->match;

I think this change is incompatible with current behavior.

Consider the 'policy' match which matches the packet's xfrm state (sec_path)
with the provided user space parameters. The sec_path includes information
about the encapsulating packet's parameters whereas the current skb points to
the encapsulated packet, and the match is done on the encapsulating
packet's info.

So if you have an IPv6 packet encapsulated within an IPv4 packet, the match
parameters should be done using IPv4 parameters, not IPv6.

Maybe use the packet's family only if the match family is UNSPEC?

Eyal.

Re: [PATCH net-next 2/5] net: sched: em_ipt: set the family based on the protocol when matching

[Nikolay Aleksandrov]

On 26/06/2019 16:33, Eyal Birger wrote:
> Hi Nikolay,
>    
> On Wed, 26 Jun 2019 14:58:52 +0300
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> 
>> Set the family based on the protocol otherwise protocol-neutral
>> matches will have wrong information (e.g. NFPROTO_UNSPEC). In
>> preparation for using NFPROTO_UNSPEC xt matches.
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> ---
>>  net/sched/em_ipt.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
>> index 64dbafe4e94c..23965a071177 100644
>> --- a/net/sched/em_ipt.c
>> +++ b/net/sched/em_ipt.c
>> @@ -189,10 +189,12 @@ 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;
>> +		state.pf = NFPROTO_IPV4;
>>  		break;
>>  	case htons(ETH_P_IPV6):
>>  		if (!pskb_network_may_pull(skb, sizeof(struct
>> ipv6hdr))) return 0;
>> +		state.pf = NFPROTO_IPV6;
>>  		break;
>>  	default:
>>  		return 0;
>> @@ -203,7 +205,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, state.pf,
>>  			   indev ?: skb->dev, skb->dev, NULL,
>> em->net, NULL); 
>>  	acpar.match = im->match;
> 
> I think this change is incompatible with current behavior.
> 
> Consider the 'policy' match which matches the packet's xfrm state (sec_path)
> with the provided user space parameters. The sec_path includes information
> about the encapsulating packet's parameters whereas the current skb points to
> the encapsulated packet, and the match is done on the encapsulating
> packet's info.
> 
> So if you have an IPv6 packet encapsulated within an IPv4 packet, the match
> parameters should be done using IPv4 parameters, not IPv6.
> 
> Maybe use the packet's family only if the match family is UNSPEC?
> 
> Eyal.
> 

Hi Eyal,
I see your point, I was wondering about the xfrm cases. :)
In such case I think we can simplify the set and do it only on UNSPEC matches as you suggest.

Maybe we should enforce the tc protocol based on the user-specified nfproto at least from
iproute2 otherwise people can add mismatching rules (e.g. nfproto == v6, tc proto == v4).

Thanks,
 Nik

Re: [PATCH net-next 2/5] net: sched: em_ipt: set the family based on the protocol when matching

[Eyal Birger]

On Wed, 26 Jun 2019 16:45:28 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> On 26/06/2019 16:33, Eyal Birger wrote:
> > Hi Nikolay,
> >    
> > On Wed, 26 Jun 2019 14:58:52 +0300
> > Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >   
> >> Set the family based on the protocol otherwise protocol-neutral
> >> matches will have wrong information (e.g. NFPROTO_UNSPEC). In
> >> preparation for using NFPROTO_UNSPEC xt matches.
> >>
> >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> >> ---
> >>  net/sched/em_ipt.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
...
> >> -	nf_hook_state_init(&state, im->hook, im->match->family,
> >> +	nf_hook_state_init(&state, im->hook, state.pf,
> >>  			   indev ?: skb->dev, skb->dev, NULL,
> >> em->net, NULL); 
> >>  	acpar.match = im->match;  
> > 
> > I think this change is incompatible with current behavior.
> > 
> > Consider the 'policy' match which matches the packet's xfrm state
> > (sec_path) with the provided user space parameters. The sec_path
> > includes information about the encapsulating packet's parameters
> > whereas the current skb points to the encapsulated packet, and the
> > match is done on the encapsulating packet's info.
> > 
> > So if you have an IPv6 packet encapsulated within an IPv4 packet,
> > the match parameters should be done using IPv4 parameters, not IPv6.
> > 
> > Maybe use the packet's family only if the match family is UNSPEC?
> > 
> > Eyal.
> >   
> 
> Hi Eyal,
> I see your point, I was wondering about the xfrm cases. :)
> In such case I think we can simplify the set and do it only on UNSPEC
> matches as you suggest.
> 
> Maybe we should enforce the tc protocol based on the user-specified
> nfproto at least from iproute2 otherwise people can add mismatching
> rules (e.g. nfproto == v6, tc proto == v4).
> 
Hi Nik,

I think for iproute2 the issue is the same. For encapsulated IPv6 in
IPv4 for example, tc proto will be IPv6 (tc sees the encapsulated
packet after decryption) whereas nfproto will be IPv4 (policy match is
done on the encapsulating state metadata which is IPv4).

I think the part missing in iproute2 is the ability to specify
NFPROTO_UNSPEC.

Thanks,
Eyal

Re: [PATCH net-next 2/5] net: sched: em_ipt: set the family based on the protocol when matching

[nikolay]

On 26 June 2019 19:22:54 EEST, Eyal Birger <eyal.birger@gmail.com> wrote:
>On Wed, 26 Jun 2019 16:45:28 +0300
>Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>
>> On 26/06/2019 16:33, Eyal Birger wrote:
>> > Hi Nikolay,
>> >    
>> > On Wed, 26 Jun 2019 14:58:52 +0300
>> > Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>> >   
>> >> Set the family based on the protocol otherwise protocol-neutral
>> >> matches will have wrong information (e.g. NFPROTO_UNSPEC). In
>> >> preparation for using NFPROTO_UNSPEC xt matches.
>> >>
>> >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> >> ---
>> >>  net/sched/em_ipt.c | 4 +++-
>> >>  1 file changed, 3 insertions(+), 1 deletion(-)
>> >>
>...
>> >> -	nf_hook_state_init(&state, im->hook, im->match->family,
>> >> +	nf_hook_state_init(&state, im->hook, state.pf,
>> >>  			   indev ?: skb->dev, skb->dev, NULL,
>> >> em->net, NULL); 
>> >>  	acpar.match = im->match;  
>> > 
>> > I think this change is incompatible with current behavior.
>> > 
>> > Consider the 'policy' match which matches the packet's xfrm state
>> > (sec_path) with the provided user space parameters. The sec_path
>> > includes information about the encapsulating packet's parameters
>> > whereas the current skb points to the encapsulated packet, and the
>> > match is done on the encapsulating packet's info.
>> > 
>> > So if you have an IPv6 packet encapsulated within an IPv4 packet,
>> > the match parameters should be done using IPv4 parameters, not
>IPv6.
>> > 
>> > Maybe use the packet's family only if the match family is UNSPEC?
>> > 
>> > Eyal.
>> >   
>> 
>> Hi Eyal,
>> I see your point, I was wondering about the xfrm cases. :)
>> In such case I think we can simplify the set and do it only on UNSPEC
>> matches as you suggest.
>> 
>> Maybe we should enforce the tc protocol based on the user-specified
>> nfproto at least from iproute2 otherwise people can add mismatching
>> rules (e.g. nfproto == v6, tc proto == v4).
>> 
>Hi Nik,
>
>I think for iproute2 the issue is the same. For encapsulated IPv6 in
>IPv4 for example, tc proto will be IPv6 (tc sees the encapsulated
>packet after decryption) whereas nfproto will be IPv4 (policy match is
>done on the encapsulating state metadata which is IPv4).
>
>I think the part missing in iproute2 is the ability to specify
>NFPROTO_UNSPEC.
>
>Thanks,
>Eyal

Right, I answered too quickly, it makes sense to mix them for xt policy.
I also plan to add support for clsact, it should be trivial and iproute2-only
change.
  
Thanks, 
  Nik