[net regression] "fib_rules: move common handling of newrule delrule msgs into fib_nl2rule" breaks suppress_prefixlength

[net regression] "fib_rules: move common handling of newrule delrule msgs into fib_nl2rule" breaks suppress_prefixlength

[Jason A. Donenfeld]

Hey Roopa,

On a kernel with a minimal networking config,
CONFIG_IP_MULTIPLE_TABLES appears to be broken for certain rules after
f9d4b0c1e9695e3de7af3768205bacc27312320c.

Try, for example, running:

$ ip -4 rule add table main suppress_prefixlength 0

It returns with EEXIST.

Perhaps the reason is that the new rule_find function does not match
on suppress_prefixlength? However, rule_exist from before didn't do
that either. I'll keep playing and see if I can track it down myself,
but thought I should let you know first.

A relevant .config can be found at https://א.cc/iq5HoUY0

Jason

[PATCH] fib_rules: match rules based on suppress_* properties too

[Jason A. Donenfeld]

Two rules with different values of suppress_prefix or suppress_ifgroup
are not the same. This fixes an -EEXIST when running:

   $ ip -4 rule add table main suppress_prefixlength 0

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Fixes: f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule msgs into fib_nl2rule")
---
 net/core/fib_rules.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
index 126ffc5bc630..665799311b98 100644
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c
@@ -416,6 +416,12 @@ static struct fib_rule *rule_find(struct fib_rules_ops *ops,
 		if (rule->mark && r->mark != rule->mark)
 			continue;
 
+		if (r->suppress_ifgroup != rule->suppress_ifgroup)
+			continue;
+
+		if (r->suppress_prefixlen != rule->suppress_prefixlen)
+			continue;
+
 		if (rule->mark_mask && r->mark_mask != rule->mark_mask)
 			continue;
 
-- 
2.17.1

Re: [PATCH] fib_rules: match rules based on suppress_* properties too

[David Miller]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Sat, 23 Jun 2018 17:59:30 +0200

> Two rules with different values of suppress_prefix or suppress_ifgroup
> are not the same. This fixes an -EEXIST when running:
> 
>    $ ip -4 rule add table main suppress_prefixlength 0
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Fixes: f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule msgs into fib_nl2rule")

But the old rule_find() code didn't check this key either, so I can't
see how the behavior in this area changed.

I think the behavior changed for a different reason.

The commit mentioned in your Fixes: tag changed newrule semantics
wrt. defaults or "any" values.

The original code matched on pure values of the keys, whereas the new
code only compares the keys when the new rule is not specifying an
"any" value.

-		if (r->table != rule->table)
+		if (rule->table && r->table != rule->table)
 			continue;

And I think these changes are what makes your test case fail after the
commit.  Some other key didn't match previous due to the handling of
"any" values.

Re: [PATCH] fib_rules: match rules based on suppress_* properties too

[Roopa Prabhu]

On Sat, Jun 23, 2018 at 8:59 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Two rules with different values of suppress_prefix or suppress_ifgroup
> are not the same. This fixes an -EEXIST when running:
>
>    $ ip -4 rule add table main suppress_prefixlength 0
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Fixes: f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule msgs into fib_nl2rule")
> ---
>  net/core/fib_rules.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
> index 126ffc5bc630..665799311b98 100644
> --- a/net/core/fib_rules.c
> +++ b/net/core/fib_rules.c
> @@ -416,6 +416,12 @@ static struct fib_rule *rule_find(struct fib_rules_ops *ops,
>                 if (rule->mark && r->mark != rule->mark)
>                         continue;
>
> +               if (r->suppress_ifgroup != rule->suppress_ifgroup)
> +                       continue;
> +
> +               if (r->suppress_prefixlen != rule->suppress_prefixlen)
> +                       continue;
> +
>                 if (rule->mark_mask && r->mark_mask != rule->mark_mask)
>                         continue;
>

Can you please change the check to compare only if the new rule has
the attributes set ?

eg:

if (rule->suppress_ifgroup != -1 && (r->suppress_ifgroup !=
rule->suppress_ifgroup))

same thing for suppress_prefixlen

Re: [net regression] "fib_rules: move common handling of newrule delrule msgs into fib_nl2rule" breaks suppress_prefixlength

[Roopa Prabhu]

On Sat, Jun 23, 2018 at 8:46 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Hey Roopa,
>
> On a kernel with a minimal networking config,
> CONFIG_IP_MULTIPLE_TABLES appears to be broken for certain rules after
> f9d4b0c1e9695e3de7af3768205bacc27312320c.
>
> Try, for example, running:
>
> $ ip -4 rule add table main suppress_prefixlength 0
>
> It returns with EEXIST.
>
> Perhaps the reason is that the new rule_find function does not match
> on suppress_prefixlength? However, rule_exist from before didn't do
> that either. I'll keep playing and see if I can track it down myself,
> but thought I should let you know first.

I am surprised at that also. I cannot find prior rule_exist looking at
suppress_prefixlength.
I will dig deeper also today. But your patch LGTM with a small change
I commented on it.

>
> A relevant .config can be found at https://א.cc/iq5HoUY0
>

thanks.

Re: [net regression] "fib_rules: move common handling of newrule delrule msgs into fib_nl2rule" breaks suppress_prefixlength

[Roopa Prabhu]

On Mon, Jun 25, 2018 at 8:23 AM, Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
> On Sat, Jun 23, 2018 at 8:46 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> Hey Roopa,
>>
>> On a kernel with a minimal networking config,
>> CONFIG_IP_MULTIPLE_TABLES appears to be broken for certain rules after
>> f9d4b0c1e9695e3de7af3768205bacc27312320c.
>>
>> Try, for example, running:
>>
>> $ ip -4 rule add table main suppress_prefixlength 0
>>
>> It returns with EEXIST.
>>
>> Perhaps the reason is that the new rule_find function does not match
>> on suppress_prefixlength? However, rule_exist from before didn't do
>> that either. I'll keep playing and see if I can track it down myself,
>> but thought I should let you know first.
>
> I am surprised at that also. I cannot find prior rule_exist looking at
> suppress_prefixlength.
> I will dig deeper also today. But your patch LGTM with a small change
> I commented on it.
>

So the previous rule_exists code did not check for attribute matches correctly.
It would ignore a rule at the first non-existent attribute mis-match.
eg in your case, it would
return at pref mismatch.


$ip -4 rule add table main suppress_prefixlength 0
$ip -4 rule add table main suppress_prefixlength 0
$ip -4 rule add table main suppress_prefixlength 0

$ip rule show
0:      from all lookup local

32763:  from all lookup main suppress_prefixlength 0

32764:  from all lookup main suppress_prefixlength 0

32765:  from all lookup main suppress_prefixlength 0

32766:  from all lookup main

32767:  from all lookup default


With your patch (with my proposed fixes), you should get proper EXISTS check

$ git diff HEAD

diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c

index 126ffc5..de4c171 100644

--- a/net/core/fib_rules.c

+++ b/net/core/fib_rules.c

@@ -428,6 +428,14 @@ static struct fib_rule *rule_find(struct
fib_rules_ops *ops,

                if (rule->l3mdev && r->l3mdev != rule->l3mdev)

                        continue;



+               if (rule->suppress_ifgroup != -1 &&

+                   (r->suppress_ifgroup != rule->suppress_ifgroup))

+                       continue;

+

+               if (rule->suppress_prefixlen != -1 &&

+                   (r->suppress_prefixlen != rule->suppress_prefixlen))

+                       continue;

+

                if (uid_range_set(&rule->uid_range) &&

                    (!uid_eq(r->uid_range.start, rule->uid_range.start) ||

                    !uid_eq(r->uid_range.end, rule->uid_range.end)))



$ ip -4 rule add table main suppress_prefixlength 0

$ ip -4 rule add table main suppress_prefixlength 0

RTNETLINK answers: File exists

[PATCH v2] fib_rules: match rules based on suppress_* properties too

[Jason A. Donenfeld]

Two rules with different values of suppress_prefix or suppress_ifgroup
are not the same. This fixes an -EEXIST when running:

   $ ip -4 rule add table main suppress_prefixlength 0

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Fixes: f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule msgs into fib_nl2rule")
---
This adds the new condition you mentioned. I'm not sure what you make of
DaveM's remark about this not being in the original code, but here is
nonetheless the requested change.

 net/core/fib_rules.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
index 126ffc5bc630..bc8425d81022 100644
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c
@@ -416,6 +416,14 @@ static struct fib_rule *rule_find(struct fib_rules_ops *ops,
 		if (rule->mark && r->mark != rule->mark)
 			continue;
 
+		if (rule->suppress_ifgroup != -1 &&
+		    r->suppress_ifgroup != rule->suppress_ifgroup)
+			continue;
+
+		if (rule->suppress_prefixlen != -1 &&
+		    r->suppress_prefixlen != rule->suppress_prefixlen)
+			continue;
+
 		if (rule->mark_mask && r->mark_mask != rule->mark_mask)
 			continue;
 
-- 
2.17.1

Re: [PATCH v2] fib_rules: match rules based on suppress_* properties too

[Roopa Prabhu]

On Mon, Jun 25, 2018 at 4:39 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Two rules with different values of suppress_prefix or suppress_ifgroup
> are not the same. This fixes an -EEXIST when running:
>
>    $ ip -4 rule add table main suppress_prefixlength 0
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Fixes: f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule msgs into fib_nl2rule")
> ---
> This adds the new condition you mentioned. I'm not sure what you make of
> DaveM's remark about this not being in the original code, but here is
> nonetheless the requested change.

I just saw DaveM's comment and agree the new rule_find is different
but that was intentional and it merged
the finding of the rule in the newlink and dellink paths. I did port
each of the conditions from previous rule_exists
to new rule_find, but forgot to add the new keys which now became
necessary. I replied with details on your
other bug report thread. Also pasting that response here:

So the previous rule_exists code did not check for attribute matches correctly.
It would ignore a rule at the first non-existent attribute mis-match.
And rule_find will always
be called with a valid key.
eg in your case, it would
return at pref mismatch...and never match an existing rule.

$ip -4 rule add table main suppress_prefixlength 0
$ip -4 rule add table main suppress_prefixlength 0
$ip -4 rule add table main suppress_prefixlength 0

$ip rule show
0:      from all lookup local
32763:  from all lookup main suppress_prefixlength 0
32764:  from all lookup main suppress_prefixlength 0
32765:  from all lookup main suppress_prefixlength 0
32766:  from all lookup main
32767:  from all lookup default

With your patch, you should get proper EXISTS check
$ ip -4 rule add table main suppress_prefixlength 0
$ ip -4 rule add table main suppress_prefixlength 0

RTNETLINK answers: File exists

Dave, pls let me know if this is acceptable. If not
I can easily restore the previous rule_exists func. Will also submit a
patch to cover this in self-tests.

thanks.



>
>  net/core/fib_rules.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
> index 126ffc5bc630..bc8425d81022 100644
> --- a/net/core/fib_rules.c
> +++ b/net/core/fib_rules.c
> @@ -416,6 +416,14 @@ static struct fib_rule *rule_find(struct fib_rules_ops *ops,
>                 if (rule->mark && r->mark != rule->mark)
>                         continue;
>
> +               if (rule->suppress_ifgroup != -1 &&
> +                   r->suppress_ifgroup != rule->suppress_ifgroup)
> +                       continue;
> +
> +               if (rule->suppress_prefixlen != -1 &&
> +                   r->suppress_prefixlen != rule->suppress_prefixlen)
> +                       continue;
> +
>                 if (rule->mark_mask && r->mark_mask != rule->mark_mask)
>                         continue;
>
> --

Re: [PATCH v2] fib_rules: match rules based on suppress_* properties too

[David Miller]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Tue, 26 Jun 2018 01:39:32 +0200

> Two rules with different values of suppress_prefix or suppress_ifgroup
> are not the same. This fixes an -EEXIST when running:
> 
>    $ ip -4 rule add table main suppress_prefixlength 0
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Fixes: f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule msgs into fib_nl2rule")

Roopa, thanks for doing all of that analysis.

I think applying this patch makes the most sense at this point,
so that it what I have done.

Re: [PATCH v2] fib_rules: match rules based on suppress_* properties too

[Roopa Prabhu]

On Tue, Jun 26, 2018 at 6:34 PM, David Miller <davem@davemloft.net> wrote:
> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Date: Tue, 26 Jun 2018 01:39:32 +0200
>
>> Two rules with different values of suppress_prefix or suppress_ifgroup
>> are not the same. This fixes an -EEXIST when running:
>>
>>    $ ip -4 rule add table main suppress_prefixlength 0
>>
>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>> Fixes: f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule msgs into fib_nl2rule")
>
> Roopa, thanks for doing all of that analysis.
>
> I think applying this patch makes the most sense at this point,
> so that it what I have done.


Thanks, will keep an eye out and add some more tests