[PATCH net] fib_rules: don't break ECN with TOS rules

[PATCH net] fib_rules: don't break ECN with TOS rules

[Hannes Frederic Sowa]

Users of ToS rules could accidentally break ECN, this patch tries to
fix this in a way so we don't break shell scripts depending on the old
behavior while still being transparent to ECN. This quietly fixes ECN
behavior for old setups.

For IPv6 we have no check if we check for ECN bits, in IPv4 we only
check for the last bit, which is specified to be '0' from pre-DSCP times
(because of implementation confusion).

This patch changes fib rules in a way that matches only for ecn bits
will never match from now on (I consider them illegal), as we simply
ignore those rules (it was easier to explain in a pr_warn). Opinions?

Cc: Florian Westphal <fw@strlen.de>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 include/net/inet_ecn.h | 5 +++++
 net/ipv4/fib_rules.c   | 6 +++++-
 net/ipv6/fib6_rules.c  | 7 ++++++-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/net/inet_ecn.h b/include/net/inet_ecn.h
index 0dc0a51da38faa..be65d94c05ee01 100644
--- a/include/net/inet_ecn.h
+++ b/include/net/inet_ecn.h
@@ -17,6 +17,11 @@ enum {
 
 extern int sysctl_tunnel_ecn_log;
 
+static inline __u8 INET_ECN_ignore(__u8 dsfield)
+{
+	return dsfield & ~INET_ECN_MASK;
+}
+
 static inline int INET_ECN_is_ce(__u8 dsfield)
 {
 	return (dsfield & INET_ECN_MASK) == INET_ECN_CE;
diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c
index 6e9ea69e5f751b..87efbc38589c76 100644
--- a/net/ipv4/fib_rules.c
+++ b/net/ipv4/fib_rules.c
@@ -145,7 +145,8 @@ static int fib4_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
 	    ((daddr ^ r->dst) & r->dstmask))
 		return 0;
 
-	if (r->tos && (r->tos != fl4->flowi4_tos))
+	if (!INET_ECN_ignore(r->tos) ||
+	    INET_ECN_ignore(r->tos) != INET_ECN_ignore(fl4->flowi4_tos))
 		return 0;
 
 	return 1;
@@ -215,6 +216,9 @@ static int fib4_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
 	rule4->dst_len = frh->dst_len;
 	rule4->dstmask = inet_make_mask(rule4->dst_len);
 	rule4->tos = frh->tos;
+	if (!INET_ECN_ignore(rule4->tos))
+		pr_warn("ipv4: never matching ipv4 rule with match for %x added\n",
+			rule4->tos);
 
 	net->ipv4.fib_has_custom_rules = true;
 	fib_flush_external(rule->fr_net);
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index 5857c1fc8b6721..4c99e0347d05b7 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -21,6 +21,7 @@
 #include <net/addrconf.h>
 #include <net/ip6_route.h>
 #include <net/netlink.h>
+#include <net/inet_ecn.h>
 
 struct fib6_rule {
 	struct fib_rule		common;
@@ -183,7 +184,8 @@ static int fib6_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
 			return 0;
 	}
 
-	if (r->tclass && r->tclass != ip6_tclass(fl6->flowlabel))
+	if (!INET_ECN_ignore(r->tclass) ||
+	    INET_ECN_ignore(r->tclass) != INET_ECN_ignore(ip6_tclass(fl6->flowlabel)))
 		return 0;
 
 	return 1;
@@ -220,6 +222,9 @@ static int fib6_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
 	rule6->src.plen = frh->src_len;
 	rule6->dst.plen = frh->dst_len;
 	rule6->tclass = frh->tos;
+	if (!INET_ECN_ignore(rule6->tclass))
+		pr_warn("ipv6: never matching ipv6 rule with match for tos %x added\n",
+			rule6->tclass);
 
 	err = 0;
 errout:
-- 
2.5.5

Re: [PATCH net] fib_rules: don't break ECN with TOS rules

[Julian Anastasov]

	Hello,

On Sat, 11 Jun 2016, Hannes Frederic Sowa wrote:

> Users of ToS rules could accidentally break ECN, this patch tries to
> fix this in a way so we don't break shell scripts depending on the old
> behavior while still being transparent to ECN. This quietly fixes ECN
> behavior for old setups.
> 
> For IPv6 we have no check if we check for ECN bits, in IPv4 we only
> check for the last bit, which is specified to be '0' from pre-DSCP times
> (because of implementation confusion).
> 
> This patch changes fib rules in a way that matches only for ecn bits
> will never match from now on (I consider them illegal), as we simply
> ignore those rules (it was easier to explain in a pr_warn). Opinions?

	Well, may be the confusion comes from commit 89aef8921bfb
("ipv4: Delete routing cache.") where the 'tos &= IPTOS_RT_MASK;'
line is lost from ip_route_input_common. I think, we should
add it back, so that we can properly match input routes with rules
that specify tos value. Old kernels didn't stored ECN bits in
flowi4_tos in the input path, so we should do the same.

> Cc: Florian Westphal <fw@strlen.de>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
>  include/net/inet_ecn.h | 5 +++++
>  net/ipv4/fib_rules.c   | 6 +++++-
>  net/ipv6/fib6_rules.c  | 7 ++++++-
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/inet_ecn.h b/include/net/inet_ecn.h
> index 0dc0a51da38faa..be65d94c05ee01 100644
> --- a/include/net/inet_ecn.h
> +++ b/include/net/inet_ecn.h
> @@ -17,6 +17,11 @@ enum {
>  
>  extern int sysctl_tunnel_ecn_log;
>  
> +static inline __u8 INET_ECN_ignore(__u8 dsfield)
> +{
> +	return dsfield & ~INET_ECN_MASK;
> +}
> +
>  static inline int INET_ECN_is_ce(__u8 dsfield)
>  {
>  	return (dsfield & INET_ECN_MASK) == INET_ECN_CE;
> diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c
> index 6e9ea69e5f751b..87efbc38589c76 100644
> --- a/net/ipv4/fib_rules.c
> +++ b/net/ipv4/fib_rules.c
> @@ -145,7 +145,8 @@ static int fib4_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
>  	    ((daddr ^ r->dst) & r->dstmask))
>  		return 0;
>  
> -	if (r->tos && (r->tos != fl4->flowi4_tos))
> +	if (!INET_ECN_ignore(r->tos) ||

	Above check will succeed for any rule that does not use
tos (r->tos = 0). So, rules without tos will return 0 (no match).

> +	    INET_ECN_ignore(r->tos) != INET_ECN_ignore(fl4->flowi4_tos))
>  		return 0;

	fib4_rule_configure already rejects ECN bits in r->tos,
so no need to filter them again in fast path.

>  	return 1;
> @@ -215,6 +216,9 @@ static int fib4_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
>  	rule4->dst_len = frh->dst_len;
>  	rule4->dstmask = inet_make_mask(rule4->dst_len);
>  	rule4->tos = frh->tos;
> +	if (!INET_ECN_ignore(rule4->tos))
> +		pr_warn("ipv4: never matching ipv4 rule with match for %x added\n",
> +			rule4->tos);

	You should see this warning for non-tos rules...

Regards

Re: [PATCH net] fib_rules: don't break ECN with TOS rules

[Hannes Frederic Sowa]

Hi Julian,

On Sun, Jun 12, 2016, at 02:09, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Sat, 11 Jun 2016, Hannes Frederic Sowa wrote:
> 
> > Users of ToS rules could accidentally break ECN, this patch tries to
> > fix this in a way so we don't break shell scripts depending on the old
> > behavior while still being transparent to ECN. This quietly fixes ECN
> > behavior for old setups.
> > 
> > For IPv6 we have no check if we check for ECN bits, in IPv4 we only
> > check for the last bit, which is specified to be '0' from pre-DSCP times
> > (because of implementation confusion).
> > 
> > This patch changes fib rules in a way that matches only for ecn bits
> > will never match from now on (I consider them illegal), as we simply
> > ignore those rules (it was easier to explain in a pr_warn). Opinions?
> 
> 	Well, may be the confusion comes from commit 89aef8921bfb
> ("ipv4: Delete routing cache.") where the 'tos &= IPTOS_RT_MASK;'
> line is lost from ip_route_input_common. I think, we should
> add it back, so that we can properly match input routes with rules
> that specify tos value. Old kernels didn't stored ECN bits in
> flowi4_tos in the input path, so we should do the same.

I would love to have done that but was fearing problems with user space
compatibility. Also IPTOS_RT_MASK is not enough for filtering, we need
to check for the whole INET_ECN_MASK.

> 
> > Cc: Florian Westphal <fw@strlen.de>
> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > ---
> >  include/net/inet_ecn.h | 5 +++++
> >  net/ipv4/fib_rules.c   | 6 +++++-
> >  net/ipv6/fib6_rules.c  | 7 ++++++-
> >  3 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/inet_ecn.h b/include/net/inet_ecn.h
> > index 0dc0a51da38faa..be65d94c05ee01 100644
> > --- a/include/net/inet_ecn.h
> > +++ b/include/net/inet_ecn.h
> > @@ -17,6 +17,11 @@ enum {
> >  
> >  extern int sysctl_tunnel_ecn_log;
> >  
> > +static inline __u8 INET_ECN_ignore(__u8 dsfield)
> > +{
> > +	return dsfield & ~INET_ECN_MASK;
> > +}
> > +
> >  static inline int INET_ECN_is_ce(__u8 dsfield)
> >  {
> >  	return (dsfield & INET_ECN_MASK) == INET_ECN_CE;
> > diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c
> > index 6e9ea69e5f751b..87efbc38589c76 100644
> > --- a/net/ipv4/fib_rules.c
> > +++ b/net/ipv4/fib_rules.c
> > @@ -145,7 +145,8 @@ static int fib4_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
> >  	    ((daddr ^ r->dst) & r->dstmask))
> >  		return 0;
> >  
> > -	if (r->tos && (r->tos != fl4->flowi4_tos))
> > +	if (!INET_ECN_ignore(r->tos) ||
> 
> 	Above check will succeed for any rule that does not use
> tos (r->tos = 0). So, rules without tos will return 0 (no match).

Oh yes, certainly. I switched back and forth between different schemas.
This obviously doesn't work, thanks! I actually only tested the cases
with tos rules. :(

> > +	    INET_ECN_ignore(r->tos) != INET_ECN_ignore(fl4->flowi4_tos))
> >  		return 0;
> 
> 	fib4_rule_configure already rejects ECN bits in r->tos,
> so no need to filter them again in fast path.

The problem is that IPTOS_TOS_MASK is just the masking of the 0x1 bit,
not both ECN bits. Thi stems from the pre-DSCP time where it was
forbidden to use bit 0x1 in TOS.

> >  	return 1;
> > @@ -215,6 +216,9 @@ static int fib4_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
> >  	rule4->dst_len = frh->dst_len;
> >  	rule4->dstmask = inet_make_mask(rule4->dst_len);
> >  	rule4->tos = frh->tos;
> > +	if (!INET_ECN_ignore(rule4->tos))
> > +		pr_warn("ipv4: never matching ipv4 rule with match for %x added\n",
> > +			rule4->tos);
> 
> 	You should see this warning for non-tos rules...

I added the warnings after testing, sorry...

Bye,
Hannes

Re: [PATCH net] fib_rules: don't break ECN with TOS rules

[Hannes Frederic Sowa]

On Sun, Jun 12, 2016, at 02:36, Hannes Frederic Sowa wrote:
> On Sun, Jun 12, 2016, at 02:09, Julian Anastasov wrote:
> > 
> > 	Hello,
> > 
> > On Sat, 11 Jun 2016, Hannes Frederic Sowa wrote:
> > 
> > > Users of ToS rules could accidentally break ECN, this patch tries to
> > > fix this in a way so we don't break shell scripts depending on the old
> > > behavior while still being transparent to ECN. This quietly fixes ECN
> > > behavior for old setups.
> > > 
> > > For IPv6 we have no check if we check for ECN bits, in IPv4 we only
> > > check for the last bit, which is specified to be '0' from pre-DSCP times
> > > (because of implementation confusion).
> > > 
> > > This patch changes fib rules in a way that matches only for ecn bits
> > > will never match from now on (I consider them illegal), as we simply
> > > ignore those rules (it was easier to explain in a pr_warn). Opinions?
> > 
> > 	Well, may be the confusion comes from commit 89aef8921bfb
> > ("ipv4: Delete routing cache.") where the 'tos &= IPTOS_RT_MASK;'
> > line is lost from ip_route_input_common. I think, we should
> > add it back, so that we can properly match input routes with rules
> > that specify tos value. Old kernels didn't stored ECN bits in
> > flowi4_tos in the input path, so we should do the same.
> 
> I would love to have done that but was fearing problems with user space
> compatibility. Also IPTOS_RT_MASK is not enough for filtering, we need
> to check for the whole INET_ECN_MASK.

Correction: IPTOS_RT_MASK would be fine, but fib4_rule_configure uses
IPTOS_TOS_MASK.

Bye,
Hannes

Re: [PATCH net] fib_rules: don't break ECN with TOS rules

[Julian Anastasov]

	Hello,

On Sun, 12 Jun 2016, Hannes Frederic Sowa wrote:

> On Sun, Jun 12, 2016, at 02:09, Julian Anastasov wrote:
> > 
> > 	Well, may be the confusion comes from commit 89aef8921bfb
> > ("ipv4: Delete routing cache.") where the 'tos &= IPTOS_RT_MASK;'
> > line is lost from ip_route_input_common. I think, we should
> > add it back, so that we can properly match input routes with rules
> > that specify tos value. Old kernels didn't stored ECN bits in
> > flowi4_tos in the input path, so we should do the same.
> 
> I would love to have done that but was fearing problems with user space
> compatibility. Also IPTOS_RT_MASK is not enough for filtering, we need
> to check for the whole INET_ECN_MASK.

	I checked even 2.4.33 for reference. Nobody sets bit 1
in key->tos but using IPTOS_TOS_MASK on configuration allows
rules with 'tos 2'. I guess, in this case such rule can not be
matched. And the TOS matching was used only under
CONFIG_IP_ROUTE_TOS, so the ip rules were supposed to match only
bits 2..4, not full TOS values.

> > > +	    INET_ECN_ignore(r->tos) != INET_ECN_ignore(fl4->flowi4_tos))
> > >  		return 0;
> > 
> > 	fib4_rule_configure already rejects ECN bits in r->tos,
> > so no need to filter them again in fast path.
> 
> The problem is that IPTOS_TOS_MASK is just the masking of the 0x1 bit,
> not both ECN bits. Thi stems from the pre-DSCP time where it was
> forbidden to use bit 0x1 in TOS.

	I see, I didn't noticed it. If we decide to reject
rules with tos 2 we can break scripts. May be we should just
filter it:

	if (r->tos && ((r->tos & IPTOS_RT_MASK) != fl4->flowi4_tos))

	This will allow the hidden feature 'ip rule add tos 2' to
match properly bits2..4 == 0 :) But may be we should not spend
extra cycles in fast path for such feature. As nobody complained
for problems with 3.6+, it seems match by tos is not used often.
May be we just need to restore the missing 'tos &= IPTOS_RT_MASK;'
line?

Regards

Re: [PATCH net] fib_rules: don't break ECN with TOS rules

[Hannes Frederic Sowa]

On Sun, Jun 12, 2016, at 11:28, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Sun, 12 Jun 2016, Hannes Frederic Sowa wrote:
> 
> > On Sun, Jun 12, 2016, at 02:09, Julian Anastasov wrote:
> > > 
> > > 	Well, may be the confusion comes from commit 89aef8921bfb
> > > ("ipv4: Delete routing cache.") where the 'tos &= IPTOS_RT_MASK;'
> > > line is lost from ip_route_input_common. I think, we should
> > > add it back, so that we can properly match input routes with rules
> > > that specify tos value. Old kernels didn't stored ECN bits in
> > > flowi4_tos in the input path, so we should do the same.
> > 
> > I would love to have done that but was fearing problems with user space
> > compatibility. Also IPTOS_RT_MASK is not enough for filtering, we need
> > to check for the whole INET_ECN_MASK.
> 
> 	I checked even 2.4.33 for reference. Nobody sets bit 1
> in key->tos but using IPTOS_TOS_MASK on configuration allows
> rules with 'tos 2'. I guess, in this case such rule can not be
> matched. And the TOS matching was used only under
> CONFIG_IP_ROUTE_TOS, so the ip rules were supposed to match only
> bits 2..4, not full TOS values.

Thanks for looking it up! :)

> > > > +	    INET_ECN_ignore(r->tos) != INET_ECN_ignore(fl4->flowi4_tos))
> > > >  		return 0;
> > > 
> > > 	fib4_rule_configure already rejects ECN bits in r->tos,
> > > so no need to filter them again in fast path.
> > 
> > The problem is that IPTOS_TOS_MASK is just the masking of the 0x1 bit,
> > not both ECN bits. Thi stems from the pre-DSCP time where it was
> > forbidden to use bit 0x1 in TOS.
> 
> 	I see, I didn't noticed it. If we decide to reject
> rules with tos 2 we can break scripts. May be we should just
> filter it:
> 
> 	if (r->tos && ((r->tos & IPTOS_RT_MASK) != fl4->flowi4_tos))
> 
> 	This will allow the hidden feature 'ip rule add tos 2' to
> match properly bits2..4 == 0 :) But may be we should not spend
> extra cycles in fast path for such feature. As nobody complained
> for problems with 3.6+, it seems match by tos is not used often.

Not sure if people really noticed or it just quietly broke ecn. :/

> May be we just need to restore the missing 'tos &= IPTOS_RT_MASK;'
> line?

I fear we already use those lower bits for other flags, like RTO_ONLINK.
I will study this today and will report back.

My last proposal would be to simply enclose my block with a if (r->tos)
{ ... }. Not sure, yet.

Bye,
Hannes

Re: [PATCH net] fib_rules: don't break ECN with TOS rules

[David Ahern]

On 6/11/16 3:10 PM, Hannes Frederic Sowa wrote:
> @@ -215,6 +216,9 @@ static int fib4_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
>  	rule4->dst_len = frh->dst_len;
>  	rule4->dstmask = inet_make_mask(rule4->dst_len);
>  	rule4->tos = frh->tos;
> +	if (!INET_ECN_ignore(rule4->tos))
> +		pr_warn("ipv4: never matching ipv4 rule with match for %x added\n",
> +			rule4->tos);
>

The wording of the warning message is not very clear on what the problem is.

Re: [PATCH net] fib_rules: don't break ECN with TOS rules

[Hannes Frederic Sowa]

On Mon, Jun 13, 2016, at 00:18, David Ahern wrote:
> On 6/11/16 3:10 PM, Hannes Frederic Sowa wrote:
> > @@ -215,6 +216,9 @@ static int fib4_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
> >  	rule4->dst_len = frh->dst_len;
> >  	rule4->dstmask = inet_make_mask(rule4->dst_len);
> >  	rule4->tos = frh->tos;
> > +	if (!INET_ECN_ignore(rule4->tos))
> > +		pr_warn("ipv4: never matching ipv4 rule with match for %x added\n",
> > +			rule4->tos);
> >
> 
> The wording of the warning message is not very clear on what the problem
> is.

I will redesign this whole patch and will improve those warnings, too.

Thanks,
Hannes