| Message ID | 1497617087-26386-1-git-send-email-serhe.popovych@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Serhey Popovych <serhe.popovych@gmail.com> Date: Fri, 16 Jun 2017 15:44:47 +0300 > We should avoid marking goto rules unresolved when their > target is actually reachable after rule deletion. > > Consolder following sample scenario: > > # ip -4 ru sh > 0: from all lookup local > 32000: from all goto 32100 > 32100: from all lookup main > 32100: from all lookup default > 32766: from all lookup main > 32767: from all lookup default > > # ip -4 ru del pref 32100 table main > # ip -4 ru sh > 0: from all lookup local > 32000: from all goto 32100 [unresolved] > 32100: from all lookup default > 32766: from all lookup main > 32767: from all lookup default > > After removal of first rule with preference 32100 we > mark all goto rules as unreachable, even when rule with > same preference as removed one still present. > > Check if next rule with same preference is available > and make all rules with goto action pointing to it. > > Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com> Applied, thanks. It would be awesome if you could distill the above into a test case that could be run under tools/testing/selftests/networking. Thanks!
> From: Serhey Popovych <serhe.popovych@gmail.com> > Date: Fri, 16 Jun 2017 15:44:47 +0300 > >> We should avoid marking goto rules unresolved when their >> target is actually reachable after rule deletion. >> >> Consolder following sample scenario: >> >> # ip -4 ru sh >> 0: from all lookup local >> 32000: from all goto 32100 >> 32100: from all lookup main >> 32100: from all lookup default >> 32766: from all lookup main >> 32767: from all lookup default >> >> # ip -4 ru del pref 32100 table main >> # ip -4 ru sh >> 0: from all lookup local >> 32000: from all goto 32100 [unresolved] >> 32100: from all lookup default >> 32766: from all lookup main >> 32767: from all lookup default >> >> After removal of first rule with preference 32100 we >> mark all goto rules as unreachable, even when rule with >> same preference as removed one still present. >> >> Check if next rule with same preference is available >> and make all rules with goto action pointing to it. >> >> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com> > > Applied, thanks. > > It would be awesome if you could distill the above into a test case that > could be run under tools/testing/selftests/networking. Yes, I already think about testing too. Actually I have series of patches to improve fib_rules support both in core and per protocol (merge suppress, callback, reduce per network namespace memory consumption - useful for docker setups, more precise validation of netlink configuration messages, additional matching facilities etc) so testing is definitely required before my changes went upstream! About testing implementation: I see there is no infrastructure to test netlink based configuration in tools/testing. How to make this better? Use libnl, write scripts that use iproute2 utilities? Where I can get additional to Documentation/kselftest.txt information? > > Thanks! >
diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c index f21c4d3..3bba291 100644 --- a/net/core/fib_rules.c +++ b/net/core/fib_rules.c @@ -568,7 +568,7 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, struct net *net = sock_net(skb->sk); struct fib_rule_hdr *frh = nlmsg_data(nlh); struct fib_rules_ops *ops = NULL; - struct fib_rule *rule, *tmp; + struct fib_rule *rule, *r; struct nlattr *tb[FRA_MAX+1]; struct fib_kuid_range range; int err = -EINVAL; @@ -668,16 +668,23 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, /* * Check if this rule is a target to any of them. If so, + * adjust to the next one with the same preference or * disable them. As this operation is eventually very - * expensive, it is only performed if goto rules have - * actually been added. + * expensive, it is only performed if goto rules, except + * current if it is goto rule, have actually been added. */ if (ops->nr_goto_rules > 0) { - list_for_each_entry(tmp, &ops->rules_list, list) { - if (rtnl_dereference(tmp->ctarget) == rule) { - RCU_INIT_POINTER(tmp->ctarget, NULL); + struct fib_rule *n; + + n = list_next_entry(rule, list); + if (&n->list == &ops->rules_list || n->pref != rule->pref) + n = NULL; + list_for_each_entry(r, &ops->rules_list, list) { + if (rtnl_dereference(r->ctarget) != rule) + continue; + rcu_assign_pointer(r->ctarget, n); + if (!n) ops->unresolved_rules++; - } } }
We should avoid marking goto rules unresolved when their target is actually reachable after rule deletion. Consolder following sample scenario: # ip -4 ru sh 0: from all lookup local 32000: from all goto 32100 32100: from all lookup main 32100: from all lookup default 32766: from all lookup main 32767: from all lookup default # ip -4 ru del pref 32100 table main # ip -4 ru sh 0: from all lookup local 32000: from all goto 32100 [unresolved] 32100: from all lookup default 32766: from all lookup main 32767: from all lookup default After removal of first rule with preference 32100 we mark all goto rules as unreachable, even when rule with same preference as removed one still present. Check if next rule with same preference is available and make all rules with goto action pointing to it. Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com> --- net/core/fib_rules.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)