Re: [PATCH net] net: fib_rules: add protocol check in rule_find
On Thu, Jun 28, 2018 at 9:59 PM, Roopa Prabhu wrote: > On Wed, Jun 27, 2018 at 6:27 PM, Roopa Prabhu > wrote: >> From: Roopa Prabhu >> >> After commit f9d4b0c1e969 ("fib_rules: move common handling of newrule >> delrule msgs into fib_nl2rule"), rule_find is strict about checking >> for an existing rule. rule_find must check against all >> user given attributes, else it may match against a subset >> of attributes and return an existing rule. >> >> In the below case, without support for protocol match, rule_find >> will match only against 'table main' and return an existing rule. >> >> $ip -4 rule add table main protocol boot >> RTNETLINK answers: File exists >> >> This patch adds protocol support to rule_find, forcing it to >> check protocol match if given by the user. >> >> Fixes: f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule >> msgs into fib_nl2rule") >> Signed-off-by: Roopa Prabhu >> --- >> I spent some time looking at all match keys today and protocol >> was the only missing one (protocol is not in a released kernel yet). >> The only way this could be avoided is to move back to the old loose >> rule_find. I am worried about this new strict checking surprising users, >> but going back to the previous loose checking does not seem right either. >> If there is a reason to believe that users did rely on the previous >> behaviour, I will be happy to revert. Here is another example of old and >> new behaviour. >> >> old rule_find behaviour: >> $ip -4 rule add table main protocol boot >> $ip -4 rule add table main protocol boot >> $ip -4 rule add table main protocol boot >> $ip rule show >> 0: from all lookup local >> 32763: from all lookup main proto boot >> 32764: from all lookup main proto boot >> 32765: from all lookup main proto boot >> 32766: from all lookup main >> 32767: from all lookup default >> >> new rule_find behaviour (after this patch): >> $ip -4 rule add table main protocol boot >> $ip -4 rule add table main protocol boot >> RTNETLINK answers: File exists >> > > I found the case where the new rule_find breaks for add. > $ip -4 rule add table main tos 10 fwmark 1 > $ip -4 rule add table main tos 10 > RTNETLINK answers: File exists > > The key masks in the new and old rule need to be compared . > And it cannot be easily compared today without an elaborate if-else block. > Its best to introduce key masks for easier and accurate rule comparison. > But this is best done in net-next. I will submit an incremental patch > tomorrow to > restore previous rule_exists for the add case and the rest should be good. > > The current patch in context is needed regardless. > > Thanks (and sorry about the iterations). as I write the commit msg for the new incremental patch, it seems better to merge this one with the new one. Please ignore this patch, I will send an updated patch in a bit. thanks.
Re: [PATCH net] net: fib_rules: add protocol check in rule_find
On Wed, Jun 27, 2018 at 6:27 PM, Roopa Prabhu wrote: > From: Roopa Prabhu > > After commit f9d4b0c1e969 ("fib_rules: move common handling of newrule > delrule msgs into fib_nl2rule"), rule_find is strict about checking > for an existing rule. rule_find must check against all > user given attributes, else it may match against a subset > of attributes and return an existing rule. > > In the below case, without support for protocol match, rule_find > will match only against 'table main' and return an existing rule. > > $ip -4 rule add table main protocol boot > RTNETLINK answers: File exists > > This patch adds protocol support to rule_find, forcing it to > check protocol match if given by the user. > > Fixes: f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule msgs > into fib_nl2rule") > Signed-off-by: Roopa Prabhu > --- > I spent some time looking at all match keys today and protocol > was the only missing one (protocol is not in a released kernel yet). > The only way this could be avoided is to move back to the old loose > rule_find. I am worried about this new strict checking surprising users, > but going back to the previous loose checking does not seem right either. > If there is a reason to believe that users did rely on the previous > behaviour, I will be happy to revert. Here is another example of old and > new behaviour. > > old rule_find behaviour: > $ip -4 rule add table main protocol boot > $ip -4 rule add table main protocol boot > $ip -4 rule add table main protocol boot > $ip rule show > 0: from all lookup local > 32763: from all lookup main proto boot > 32764: from all lookup main proto boot > 32765: from all lookup main proto boot > 32766: from all lookup main > 32767: from all lookup default > > new rule_find behaviour (after this patch): > $ip -4 rule add table main protocol boot > $ip -4 rule add table main protocol boot > RTNETLINK answers: File exists > I found the case where the new rule_find breaks for add. $ip -4 rule add table main tos 10 fwmark 1 $ip -4 rule add table main tos 10 RTNETLINK answers: File exists The key masks in the new and old rule need to be compared . And it cannot be easily compared today without an elaborate if-else block. Its best to introduce key masks for easier and accurate rule comparison. But this is best done in net-next. I will submit an incremental patch tomorrow to restore previous rule_exists for the add case and the rest should be good. The current patch in context is needed regardless. Thanks (and sorry about the iterations).
Re: [PATCH net] net: fib_rules: add protocol check in rule_find
On 6/27/18 7:27 PM, Roopa Prabhu wrote: > From: Roopa Prabhu > > After commit f9d4b0c1e969 ("fib_rules: move common handling of newrule > delrule msgs into fib_nl2rule"), rule_find is strict about checking > for an existing rule. rule_find must check against all > user given attributes, else it may match against a subset > of attributes and return an existing rule. > > In the below case, without support for protocol match, rule_find > will match only against 'table main' and return an existing rule. > > $ip -4 rule add table main protocol boot > RTNETLINK answers: File exists > > This patch adds protocol support to rule_find, forcing it to > check protocol match if given by the user. > > Fixes: f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule msgs > into fib_nl2rule") > Signed-off-by: Roopa Prabhu > --- Reviewed-by: David Ahern
[PATCH net] net: fib_rules: add protocol check in rule_find
From: Roopa Prabhu After commit f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule msgs into fib_nl2rule"), rule_find is strict about checking for an existing rule. rule_find must check against all user given attributes, else it may match against a subset of attributes and return an existing rule. In the below case, without support for protocol match, rule_find will match only against 'table main' and return an existing rule. $ip -4 rule add table main protocol boot RTNETLINK answers: File exists This patch adds protocol support to rule_find, forcing it to check protocol match if given by the user. Fixes: f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule msgs into fib_nl2rule") Signed-off-by: Roopa Prabhu --- I spent some time looking at all match keys today and protocol was the only missing one (protocol is not in a released kernel yet). The only way this could be avoided is to move back to the old loose rule_find. I am worried about this new strict checking surprising users, but going back to the previous loose checking does not seem right either. If there is a reason to believe that users did rely on the previous behaviour, I will be happy to revert. Here is another example of old and new behaviour. old rule_find behaviour: $ip -4 rule add table main protocol boot $ip -4 rule add table main protocol boot $ip -4 rule add table main protocol boot $ip rule show 0: from all lookup local 32763: from all lookup main proto boot 32764: from all lookup main proto boot 32765: from all lookup main proto boot 32766: from all lookup main 32767: from all lookup default new rule_find behaviour (after this patch): $ip -4 rule add table main protocol boot $ip -4 rule add table main protocol boot RTNETLINK answers: File exists net/core/fib_rules.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c index bc8425d..5905567 100644 --- a/net/core/fib_rules.c +++ b/net/core/fib_rules.c @@ -444,6 +444,9 @@ static struct fib_rule *rule_find(struct fib_rules_ops *ops, if (rule->ip_proto && r->ip_proto != rule->ip_proto) continue; + if (rule->proto && r->proto != rule->proto) + continue; + if (fib_rule_port_range_set(&rule->sport_range) && !fib_rule_port_range_compare(&r->sport_range, &rule->sport_range)) -- 2.1.4