| Message ID | 1507653665-20540-2-git-send-email-dsahern@gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | mlxsw: spectrum_router: Add extack messages for RIF and VRF overflow | expand |
On 10/10/17 10:41 AM, David Ahern wrote: > @@ -988,16 +987,23 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, > goto out2; > } > > - i6vi.i6vi_addr = *addr; > - i6vi.i6vi_dev = idev; > - rcu_read_unlock_bh(); > + /* validator notifier needs to be blocking; > + * do not call in softirq context > + */ > + if (!in_softirq()) { > + struct in6_validator_info i6vi = { > + .i6vi_addr = *addr, > + .i6vi_dev = idev, > + }; > > - err = inet6addr_validator_notifier_call_chain(NETDEV_UP, &i6vi); > + rcu_read_unlock_bh(); > + err = inet6addr_validator_notifier_call_chain(NETDEV_UP, &i6vi); > + rcu_read_lock_bh(); > > - rcu_read_lock_bh(); > - err = notifier_to_errno(err); > - if (err) > - goto out2; > + err = notifier_to_errno(err); > + if (err) > + goto out2; > + } > > spin_lock(&addrconf_hash_lock); > The rcu_read_unlock_bh needs to be done before the in_softirq check. With the change below I get the RIF overload with IPv6 addresses and I verified the validator is skipped for RAs. $ ip -batch vlan-ipv6-addr-batch Error: spectrum: Exceeded number of supported router interfaces. Command failed vlan-ipv6-addr-batch:683 diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 0bad4a800f73..d9c5b29a3b8b 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -988,6 +988,8 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, goto out2; } + rcu_read_unlock_bh(); + /* validator notifier needs to be blocking; * do not call in softirq context */ @@ -998,15 +1000,14 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, .extack = extack, }; - rcu_read_unlock_bh(); err = inet6addr_validator_notifier_call_chain(NETDEV_UP, &i6vi); - rcu_read_lock_bh(); - err = notifier_to_errno(err); if (err) - goto out2; + goto out1; } + rcu_read_lock_bh(); + spin_lock(&addrconf_hash_lock); /* Ignore adding duplicate addresses on an interface */ @@ -1079,7 +1080,7 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, write_unlock(&idev->lock); out2: rcu_read_unlock_bh(); - +out1: if (likely(err == 0)) inet6addr_notifier_call_chain(NETDEV_UP, ifa); else {
On Tue, Oct 10, 2017 at 09:41:02AM -0700, David Ahern wrote: > inet6addr_validator chain was added by commit 3ad7d2468f79f ("Ipvlan > should return an error when an address is already in use") to allow > address validation before changes are committed and to be able to > fail the address change with an error back to the user. The address > validation is not done for addresses received from router > advertisements. > > Handling RAs in softirq context is the only reason for the notifier > chain to be atomic versus blocking. Since the only current user, ipvlan, > of the validator chain ignores softirq context, the notifier can be made > blocking and simply not invoked for softirq path. > > The blocking option is needed by spectrum for example to validate > resources for an adding an address to an interface. > > Signed-off-by: David Ahern <dsahern@gmail.com> With the fixup posted later: Reviewed-by: Ido Schimmel <idosch@mellanox.com> BTW, the in_softirq() check in ipvlan_addr6_validator_event() can be removed after this patch.
From: David Ahern <dsahern@gmail.com> Date: Tue, 10 Oct 2017 09:41:02 -0700 > + /* validator notifier needs to be blocking; > + * do not call in softirq context > + */ > + if (!in_softirq()) { I think we can test this better. You should be able to audit the call sites and for each one set the value of a new boolean argument properly, and this way you can also give the boolean argument a descriptive name. Furthermore, we can also then pull the inet6_addr allocation out of the locking paths and thus use GFP_KERNEL when possible.
On 10/11/17 3:13 PM, David Miller wrote: > From: David Ahern <dsahern@gmail.com> > Date: Tue, 10 Oct 2017 09:41:02 -0700 > >> + /* validator notifier needs to be blocking; >> + * do not call in softirq context >> + */ >> + if (!in_softirq()) { > > I think we can test this better. The callchain we are protecting against is 7fff8149d0dd ipv6_add_addr ([kernel.kallsyms]) 7fff814a161b addrconf_prefix_rcv ([kernel.kallsyms]) 7fff814afb8a ndisc_router_discovery ([kernel.kallsyms]) 7fff814b0310 ndisc_rcv ([kernel.kallsyms]) 7fff814b62da icmpv6_rcv ([kernel.kallsyms]) 7fff81499c37 ip6_input_finish ([kernel.kallsyms]) 7fff81499e96 ip6_input ([kernel.kallsyms]) 7fff8149a519 ip6_mc_input ([kernel.kallsyms]) 7fff81499f9d ip6_rcv_finish ([kernel.kallsyms]) 7fff8149a349 ipv6_rcv ([kernel.kallsyms]) 7fff813fbe12 __netif_receive_skb_core ([kernel.kallsyms]) 7fff813fc04c __netif_receive_skb ([kernel.kallsyms]) 7fff813ff97c netif_receive_skb_internal ([kernel.kallsyms]) > > You should be able to audit the call sites and for each one set the > value of a new boolean argument properly, and this way you can also > give the boolean argument a descriptive name. The safest is an in_atomic() check, but to your point I'll see if the caller can pass in atomic vs blocking option as a bool. > > Furthermore, we can also then pull the inet6_addr allocation out of > the locking paths and thus use GFP_KERNEL when possible. > Yes, I was thinking about that as a follow on -- how far down can the rcu_read_lock_bh be pushed.
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index d9f6226694eb..632cf4b26277 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -963,7 +963,6 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, struct net *net = dev_net(idev->dev); struct inet6_ifaddr *ifa = NULL; struct rt6_info *rt; - struct in6_validator_info i6vi; unsigned int hash; int err = 0; int addr_type = ipv6_addr_type(addr); @@ -988,16 +987,23 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, goto out2; } - i6vi.i6vi_addr = *addr; - i6vi.i6vi_dev = idev; - rcu_read_unlock_bh(); + /* validator notifier needs to be blocking; + * do not call in softirq context + */ + if (!in_softirq()) { + struct in6_validator_info i6vi = { + .i6vi_addr = *addr, + .i6vi_dev = idev, + }; - err = inet6addr_validator_notifier_call_chain(NETDEV_UP, &i6vi); + rcu_read_unlock_bh(); + err = inet6addr_validator_notifier_call_chain(NETDEV_UP, &i6vi); + rcu_read_lock_bh(); - rcu_read_lock_bh(); - err = notifier_to_errno(err); - if (err) - goto out2; + err = notifier_to_errno(err); + if (err) + goto out2; + } spin_lock(&addrconf_hash_lock); diff --git a/net/ipv6/addrconf_core.c b/net/ipv6/addrconf_core.c index 9e3488d50b15..32b564dfd02a 100644 --- a/net/ipv6/addrconf_core.c +++ b/net/ipv6/addrconf_core.c @@ -88,7 +88,7 @@ int __ipv6_addr_type(const struct in6_addr *addr) EXPORT_SYMBOL(__ipv6_addr_type); static ATOMIC_NOTIFIER_HEAD(inet6addr_chain); -static ATOMIC_NOTIFIER_HEAD(inet6addr_validator_chain); +static BLOCKING_NOTIFIER_HEAD(inet6addr_validator_chain); int register_inet6addr_notifier(struct notifier_block *nb) { @@ -110,19 +110,20 @@ EXPORT_SYMBOL(inet6addr_notifier_call_chain); int register_inet6addr_validator_notifier(struct notifier_block *nb) { - return atomic_notifier_chain_register(&inet6addr_validator_chain, nb); + return blocking_notifier_chain_register(&inet6addr_validator_chain, nb); } EXPORT_SYMBOL(register_inet6addr_validator_notifier); int unregister_inet6addr_validator_notifier(struct notifier_block *nb) { - return atomic_notifier_chain_unregister(&inet6addr_validator_chain, nb); + return blocking_notifier_chain_unregister(&inet6addr_validator_chain, + nb); } EXPORT_SYMBOL(unregister_inet6addr_validator_notifier); int inet6addr_validator_notifier_call_chain(unsigned long val, void *v) { - return atomic_notifier_call_chain(&inet6addr_validator_chain, val, v); + return blocking_notifier_call_chain(&inet6addr_validator_chain, val, v); } EXPORT_SYMBOL(inet6addr_validator_notifier_call_chain);
inet6addr_validator chain was added by commit 3ad7d2468f79f ("Ipvlan should return an error when an address is already in use") to allow address validation before changes are committed and to be able to fail the address change with an error back to the user. The address validation is not done for addresses received from router advertisements. Handling RAs in softirq context is the only reason for the notifier chain to be atomic versus blocking. Since the only current user, ipvlan, of the validator chain ignores softirq context, the notifier can be made blocking and simply not invoked for softirq path. The blocking option is needed by spectrum for example to validate resources for an adding an address to an interface. Signed-off-by: David Ahern <dsahern@gmail.com> --- net/ipv6/addrconf.c | 24 +++++++++++++++--------- net/ipv6/addrconf_core.c | 9 +++++---- 2 files changed, 20 insertions(+), 13 deletions(-)