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

Message ID 20180625233932.11531-1-Jason@zx2c4.com
State New
Headers show
Series
  • [v2] fib_rules: match rules based on suppress_* properties too
Related show

Commit Message

Jason A. Donenfeld June 25, 2018, 11:39 p.m.
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(+)

-- 
2.17.1

Comments

Roopa Prabhu June 26, 2018, 2:51 p.m. | #1
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;

>

> --
David Miller June 27, 2018, 1:34 a.m. | #2
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.
Roopa Prabhu June 27, 2018, 4:53 a.m. | #3
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

Patch

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;