| Message ID | 20250206084629.16602-7-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fib: rules: Convert RTM_NEWRULE and RTM_DELRULE to per-netns RTNL. | expand |
On Thu, Feb 6, 2025 at 9:49 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > fib_nl_delrule() is the doit() handler for RTM_DELRULE but also called > 1;95;0cfrom vrf_newlink() in case something fails in vrf_add_fib_rules(). > > In the latter case, RTNL is already held and the 3rd arg extack is NULL. > > Let's hold per-netns RTNL in fib_nl_delrule() if extack is NULL. > > Now we can place ASSERT_RTNL_NET() in call_fib_rule_notifiers(). > > While at it, fib_rule r is moved to the suitable scope. > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > --- > net/core/fib_rules.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c > index cc26c762fa9e..3430d026134d 100644 > --- a/net/core/fib_rules.c > +++ b/net/core/fib_rules.c > @@ -371,7 +371,8 @@ static int call_fib_rule_notifiers(struct net *net, > .rule = rule, > }; > > - ASSERT_RTNL(); > + ASSERT_RTNL_NET(net); This warning will then fire in the vrf case, because vrf_fib_rule() is only holding the real RTNL, but not yet the net->rtnl_mutex ? > + > /* Paired with READ_ONCE() in fib_rules_seq() */ > WRITE_ONCE(ops->fib_rules_seq, ops->fib_rules_seq + 1); > return call_fib_notifiers(net, event_type, &info.info); > @@ -909,13 +910,13 @@ EXPORT_SYMBOL_GPL(fib_nl_newrule); > int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, > struct netlink_ext_ack *extack) > { > - struct net *net = sock_net(skb->sk); > + bool user_priority = false, hold_rtnl = !!extack; I am not pleased with this heuristic hidden here. At the very least a fat comment in drivers/net/vrf.c would be welcomed. > + struct fib_rule *rule = NULL, *nlrule = NULL; > struct fib_rule_hdr *frh = nlmsg_data(nlh); > + struct net *net = sock_net(skb->sk); > struct fib_rules_ops *ops = NULL; > - struct fib_rule *rule = NULL, *r, *nlrule = NULL; > struct nlattr *tb[FRA_MAX+1]; > int err = -EINVAL; > - bool user_priority = false; > > if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*frh))) { > NL_SET_ERR_MSG(extack, "Invalid msg length"); > @@ -940,6 +941,9 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > goto errout; > > + if (hold_rtnl) > + rtnl_net_lock(net); > + > err = fib_nl2rule_rtnl(nlrule, ops, tb, extack); > if (err) > goto errout_free; > @@ -980,7 +984,7 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, > * current if it is goto rule, have actually been added. > */ > if (ops->nr_goto_rules > 0) { > - struct fib_rule *n; > + struct fib_rule *n, *r; > > n = list_next_entry(rule, list); > if (&n->list == &ops->rules_list || n->pref != rule->pref) > @@ -994,10 +998,12 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, > } > } > > - call_fib_rule_notifiers(net, FIB_EVENT_RULE_DEL, rule, ops, > - NULL); > - notify_rule_change(RTM_DELRULE, rule, ops, nlh, > - NETLINK_CB(skb).portid); > + call_fib_rule_notifiers(net, FIB_EVENT_RULE_DEL, rule, ops, NULL); > + > + if (hold_rtnl) > + rtnl_net_unlock(net); > + > + notify_rule_change(RTM_DELRULE, rule, ops, nlh, NETLINK_CB(skb).portid); > fib_rule_put(rule); > flush_route_cache(ops); > rules_ops_put(ops); > @@ -1005,6 +1011,8 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, > return 0; > > errout_free: > + if (hold_rtnl) > + rtnl_net_unlock(net); > kfree(nlrule); > errout: > rules_ops_put(ops); > @@ -1324,7 +1332,8 @@ static struct pernet_operations fib_rules_net_ops = { > static const struct rtnl_msg_handler fib_rules_rtnl_msg_handlers[] __initconst = { > {.msgtype = RTM_NEWRULE, .doit = fib_nl_newrule, > .flags = RTNL_FLAG_DOIT_PERNET}, > - {.msgtype = RTM_DELRULE, .doit = fib_nl_delrule}, > + {.msgtype = RTM_DELRULE, .doit = fib_nl_delrule, > + .flags = RTNL_FLAG_DOIT_PERNET}, > {.msgtype = RTM_GETRULE, .dumpit = fib_nl_dumprule, > .flags = RTNL_FLAG_DUMP_UNLOCKED}, > }; > -- > 2.39.5 (Apple Git-154) >
From: Eric Dumazet <edumazet@google.com> Date: Thu, 6 Feb 2025 10:41:12 +0100 > On Thu, Feb 6, 2025 at 9:49 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > fib_nl_delrule() is the doit() handler for RTM_DELRULE but also called > > 1;95;0cfrom vrf_newlink() in case something fails in vrf_add_fib_rules(). > > > > In the latter case, RTNL is already held and the 3rd arg extack is NULL. > > > > Let's hold per-netns RTNL in fib_nl_delrule() if extack is NULL. > > > > Now we can place ASSERT_RTNL_NET() in call_fib_rule_notifiers(). > > > > While at it, fib_rule r is moved to the suitable scope. > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > --- > > net/core/fib_rules.c | 29 +++++++++++++++++++---------- > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c > > index cc26c762fa9e..3430d026134d 100644 > > --- a/net/core/fib_rules.c > > +++ b/net/core/fib_rules.c > > @@ -371,7 +371,8 @@ static int call_fib_rule_notifiers(struct net *net, > > .rule = rule, > > }; > > > > - ASSERT_RTNL(); > > + ASSERT_RTNL_NET(net); > > This warning will then fire in the vrf case, because vrf_fib_rule() is > only holding the real RTNL, > but not yet the net->rtnl_mutex ? As it's RTM_NEWLINK, dev_net(net)'s per-netns RTNL is held here and vrf_fib_rule() sets skb->sk = dev_net(dev)->rtnl, so I think it won't fire. > > > + > > /* Paired with READ_ONCE() in fib_rules_seq() */ > > WRITE_ONCE(ops->fib_rules_seq, ops->fib_rules_seq + 1); > > return call_fib_notifiers(net, event_type, &info.info); > > @@ -909,13 +910,13 @@ EXPORT_SYMBOL_GPL(fib_nl_newrule); > > int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, > > struct netlink_ext_ack *extack) > > { > > - struct net *net = sock_net(skb->sk); > > + bool user_priority = false, hold_rtnl = !!extack; > > I am not pleased with this heuristic hidden here. > > At the very least a fat comment in drivers/net/vrf.c would be welcomed. Will add a comment there in v2. Thanks!
On Thu, Feb 06, 2025 at 06:52:21PM +0900, Kuniyuki Iwashima wrote: > From: Eric Dumazet <edumazet@google.com> > Date: Thu, 6 Feb 2025 10:41:12 +0100 > > On Thu, Feb 6, 2025 at 9:49 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > > > fib_nl_delrule() is the doit() handler for RTM_DELRULE but also called > > > 1;95;0cfrom vrf_newlink() in case something fails in vrf_add_fib_rules(). > > > > > > In the latter case, RTNL is already held and the 3rd arg extack is NULL. > > > > > > Let's hold per-netns RTNL in fib_nl_delrule() if extack is NULL. > > > > > > Now we can place ASSERT_RTNL_NET() in call_fib_rule_notifiers(). > > > > > > While at it, fib_rule r is moved to the suitable scope. > > > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > > --- > > > net/core/fib_rules.c | 29 +++++++++++++++++++---------- > > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > > > diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c > > > index cc26c762fa9e..3430d026134d 100644 > > > --- a/net/core/fib_rules.c > > > +++ b/net/core/fib_rules.c > > > @@ -371,7 +371,8 @@ static int call_fib_rule_notifiers(struct net *net, > > > .rule = rule, > > > }; > > > > > > - ASSERT_RTNL(); > > > + ASSERT_RTNL_NET(net); > > > > This warning will then fire in the vrf case, because vrf_fib_rule() is > > only holding the real RTNL, > > but not yet the net->rtnl_mutex ? > > As it's RTM_NEWLINK, dev_net(net)'s per-netns RTNL is held here and > vrf_fib_rule() sets skb->sk = dev_net(dev)->rtnl, so I think it won't fire. Yes, I believe you're correct. I ran fib_rule_tests.sh with a debug config and CONFIG_DEBUG_NET_SMALL_RTNL=y and didn't see any splats. BTW, did you consider adding this config option to kernel/configs/debug.config under "Networking Debugging"?
From: Ido Schimmel <idosch@idosch.org> Date: Thu, 6 Feb 2025 13:22:28 +0200 > On Thu, Feb 06, 2025 at 06:52:21PM +0900, Kuniyuki Iwashima wrote: > > From: Eric Dumazet <edumazet@google.com> > > Date: Thu, 6 Feb 2025 10:41:12 +0100 > > > On Thu, Feb 6, 2025 at 9:49 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > > > > > fib_nl_delrule() is the doit() handler for RTM_DELRULE but also called > > > > 1;95;0cfrom vrf_newlink() in case something fails in vrf_add_fib_rules(). > > > > > > > > In the latter case, RTNL is already held and the 3rd arg extack is NULL. > > > > > > > > Let's hold per-netns RTNL in fib_nl_delrule() if extack is NULL. > > > > > > > > Now we can place ASSERT_RTNL_NET() in call_fib_rule_notifiers(). > > > > > > > > While at it, fib_rule r is moved to the suitable scope. > > > > > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > > > --- > > > > net/core/fib_rules.c | 29 +++++++++++++++++++---------- > > > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c > > > > index cc26c762fa9e..3430d026134d 100644 > > > > --- a/net/core/fib_rules.c > > > > +++ b/net/core/fib_rules.c > > > > @@ -371,7 +371,8 @@ static int call_fib_rule_notifiers(struct net *net, > > > > .rule = rule, > > > > }; > > > > > > > > - ASSERT_RTNL(); > > > > + ASSERT_RTNL_NET(net); > > > > > > This warning will then fire in the vrf case, because vrf_fib_rule() is > > > only holding the real RTNL, > > > but not yet the net->rtnl_mutex ? > > > > As it's RTM_NEWLINK, dev_net(net)'s per-netns RTNL is held here and > > vrf_fib_rule() sets skb->sk = dev_net(dev)->rtnl, so I think it won't fire. > > Yes, I believe you're correct. I ran fib_rule_tests.sh with a debug > config and CONFIG_DEBUG_NET_SMALL_RTNL=y and didn't see any splats. > > BTW, did you consider adding this config option to > kernel/configs/debug.config under "Networking Debugging"? I haven't because CONFIG_DEBUG_NET_SMALL_RTNL is not strictly a debugging config and will not help debugging for real issues like other DEBUG_NET configs, but I don't have strong preference.
diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c index cc26c762fa9e..3430d026134d 100644 --- a/net/core/fib_rules.c +++ b/net/core/fib_rules.c @@ -371,7 +371,8 @@ static int call_fib_rule_notifiers(struct net *net, .rule = rule, }; - ASSERT_RTNL(); + ASSERT_RTNL_NET(net); + /* Paired with READ_ONCE() in fib_rules_seq() */ WRITE_ONCE(ops->fib_rules_seq, ops->fib_rules_seq + 1); return call_fib_notifiers(net, event_type, &info.info); @@ -909,13 +910,13 @@ EXPORT_SYMBOL_GPL(fib_nl_newrule); int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, struct netlink_ext_ack *extack) { - struct net *net = sock_net(skb->sk); + bool user_priority = false, hold_rtnl = !!extack; + struct fib_rule *rule = NULL, *nlrule = NULL; struct fib_rule_hdr *frh = nlmsg_data(nlh); + struct net *net = sock_net(skb->sk); struct fib_rules_ops *ops = NULL; - struct fib_rule *rule = NULL, *r, *nlrule = NULL; struct nlattr *tb[FRA_MAX+1]; int err = -EINVAL; - bool user_priority = false; if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*frh))) { NL_SET_ERR_MSG(extack, "Invalid msg length"); @@ -940,6 +941,9 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, if (err) goto errout; + if (hold_rtnl) + rtnl_net_lock(net); + err = fib_nl2rule_rtnl(nlrule, ops, tb, extack); if (err) goto errout_free; @@ -980,7 +984,7 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, * current if it is goto rule, have actually been added. */ if (ops->nr_goto_rules > 0) { - struct fib_rule *n; + struct fib_rule *n, *r; n = list_next_entry(rule, list); if (&n->list == &ops->rules_list || n->pref != rule->pref) @@ -994,10 +998,12 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, } } - call_fib_rule_notifiers(net, FIB_EVENT_RULE_DEL, rule, ops, - NULL); - notify_rule_change(RTM_DELRULE, rule, ops, nlh, - NETLINK_CB(skb).portid); + call_fib_rule_notifiers(net, FIB_EVENT_RULE_DEL, rule, ops, NULL); + + if (hold_rtnl) + rtnl_net_unlock(net); + + notify_rule_change(RTM_DELRULE, rule, ops, nlh, NETLINK_CB(skb).portid); fib_rule_put(rule); flush_route_cache(ops); rules_ops_put(ops); @@ -1005,6 +1011,8 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, return 0; errout_free: + if (hold_rtnl) + rtnl_net_unlock(net); kfree(nlrule); errout: rules_ops_put(ops); @@ -1324,7 +1332,8 @@ static struct pernet_operations fib_rules_net_ops = { static const struct rtnl_msg_handler fib_rules_rtnl_msg_handlers[] __initconst = { {.msgtype = RTM_NEWRULE, .doit = fib_nl_newrule, .flags = RTNL_FLAG_DOIT_PERNET}, - {.msgtype = RTM_DELRULE, .doit = fib_nl_delrule}, + {.msgtype = RTM_DELRULE, .doit = fib_nl_delrule, + .flags = RTNL_FLAG_DOIT_PERNET}, {.msgtype = RTM_GETRULE, .dumpit = fib_nl_dumprule, .flags = RTNL_FLAG_DUMP_UNLOCKED}, };
fib_nl_delrule() is the doit() handler for RTM_DELRULE but also called 1;95;0cfrom vrf_newlink() in case something fails in vrf_add_fib_rules(). In the latter case, RTNL is already held and the 3rd arg extack is NULL. Let's hold per-netns RTNL in fib_nl_delrule() if extack is NULL. Now we can place ASSERT_RTNL_NET() in call_fib_rule_notifiers(). While at it, fib_rule r is moved to the suitable scope. Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> --- net/core/fib_rules.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)