| Message ID | 20180625233932.11531-1-Jason@zx2c4.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fib_rules: match rules based on suppress_* properties too | expand |
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; > > --
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.
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
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;
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