fib_rules: match rules based on suppress_* properties too

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

Commit Message

Jason A. Donenfeld June 23, 2018, 3:59 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")
---
 net/core/fib_rules.c | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
2.17.1

Comments

David Miller June 24, 2018, 2:25 a.m. | #1
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.
Roopa Prabhu June 25, 2018, 2:58 p.m. | #2
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

Patch

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;