Re: [PATCH v2] fib_rules: match rules based on suppress_* properties too
On Tue, Jun 26, 2018 at 6:34 PM, David Miller wrote:
> From: "Jason A. Donenfeld"
> 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
>> 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
Re: [PATCH v2] fib_rules: match rules based on suppress_* properties too
From: "Jason A. Donenfeld"
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
> 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
On Mon, Jun 25, 2018 at 4:39 PM, Jason A. Donenfeld 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
> 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;
>
> --
[PATCH v2] fib_rules: match rules based on suppress_* properties too
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
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
