* [PATCH] Ipvlan should return an error when an address is already in use. @ 2016-12-31 4:10 Krister Johansen 2017-01-02 3:26 ` David Miller 2017-01-03 15:50 ` [PATCH] " Aaron Conole 0 siblings, 2 replies; 12+ messages in thread From: Krister Johansen @ 2016-12-31 4:10 UTC (permalink / raw) To: David S. Miller; +Cc: Mahesh Bandewar, netdev The ipvlan code already knows how to detect when a duplicate address is about to be assigned to an ipvlan device. However, that failure is not propogated outward and leads to a silent failure. This teaches the ip address addition functions how to report this error to the user applications so that a notifier chain failure during ip address addition will not appear to succeed when it actually has not. This can be especially useful if it is necessary to provision many ipvlans in containers. The provisioning software (or operator) can use this to detect situations where an ip address is unexpectedly in use. Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> --- drivers/net/ipvlan/ipvlan_main.c | 12 ++++++++---- net/ipv4/devinet.c | 8 +++++++- net/ipv6/addrconf.c | 23 ++++++++++++++++++----- 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c index 975f9dd..3bd2506 100644 --- a/drivers/net/ipvlan/ipvlan_main.c +++ b/drivers/net/ipvlan/ipvlan_main.c @@ -739,6 +739,7 @@ static int ipvlan_addr6_event(struct notifier_block *unused, struct inet6_ifaddr *if6 = (struct inet6_ifaddr *)ptr; struct net_device *dev = (struct net_device *)if6->idev->dev; struct ipvl_dev *ipvlan = netdev_priv(dev); + int err; /* FIXME IPv6 autoconf calls us from bh without RTNL */ if (in_softirq()) @@ -752,8 +753,9 @@ static int ipvlan_addr6_event(struct notifier_block *unused, switch (event) { case NETDEV_UP: - if (ipvlan_add_addr6(ipvlan, &if6->addr)) - return NOTIFY_BAD; + err = ipvlan_add_addr6(ipvlan, &if6->addr); + if (err) + return notifier_from_errno(err); break; case NETDEV_DOWN: @@ -788,6 +790,7 @@ static int ipvlan_addr4_event(struct notifier_block *unused, struct net_device *dev = (struct net_device *)if4->ifa_dev->dev; struct ipvl_dev *ipvlan = netdev_priv(dev); struct in_addr ip4_addr; + int err; if (!netif_is_ipvlan(dev)) return NOTIFY_DONE; @@ -798,8 +801,9 @@ static int ipvlan_addr4_event(struct notifier_block *unused, switch (event) { case NETDEV_UP: ip4_addr.s_addr = if4->ifa_address; - if (ipvlan_add_addr4(ipvlan, &ip4_addr)) - return NOTIFY_BAD; + err = ipvlan_add_addr4(ipvlan, &ip4_addr); + if (err) + return notifier_from_errno(err); break; case NETDEV_DOWN: diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index 4cd2ee8..5bc26f8e 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -442,6 +442,7 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh, { struct in_device *in_dev = ifa->ifa_dev; struct in_ifaddr *ifa1, **ifap, **last_primary; + int ret; ASSERT_RTNL(); @@ -489,7 +490,12 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh, Notifier will trigger FIB update, so that listeners of netlink will know about new ifaddr */ rtmsg_ifa(RTM_NEWADDR, ifa, nlh, portid); - blocking_notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa); + ret = blocking_notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa); + ret = notifier_to_errno(ret); + if (ret) { + __inet_del_ifa(in_dev, ifap, 1, NULL, portid); + return ret; + } return 0; } diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index c1e124b..24d89f3 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -149,6 +149,7 @@ static inline void addrconf_sysctl_unregister(struct inet6_dev *idev) static void ipv6_regen_rndid(struct inet6_dev *idev); static void ipv6_try_regen_rndid(struct inet6_dev *idev, struct in6_addr *tmpaddr); +static void __ipv6_del_addr(struct inet6_ifaddr *ifp, bool notify); static int ipv6_generate_eui64(u8 *eui, struct net_device *dev); static int ipv6_count_addresses(struct inet6_dev *idev); @@ -1031,9 +1032,15 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, out2: rcu_read_unlock_bh(); - if (likely(err == 0)) - inet6addr_notifier_call_chain(NETDEV_UP, ifa); - else { + if (likely(err == 0)) { + err = inet6addr_notifier_call_chain(NETDEV_UP, ifa); + err = notifier_to_errno(err); + if (err) { + __ipv6_del_addr(ifa, false); + ifa = ERR_PTR(err); + return ifa; + } + } else { kfree(ifa); ifa = ERR_PTR(err); } @@ -1128,7 +1135,7 @@ cleanup_prefix_route(struct inet6_ifaddr *ifp, unsigned long expires, bool del_r /* This function wants to get referenced ifp and releases it before return */ -static void ipv6_del_addr(struct inet6_ifaddr *ifp) +static void __ipv6_del_addr(struct inet6_ifaddr *ifp, bool notify) { int state; enum cleanup_prefix_rt_t action = CLEANUP_PREFIX_RT_NOP; @@ -1169,7 +1176,8 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp) addrconf_del_dad_work(ifp); - ipv6_ifa_notify(RTM_DELADDR, ifp); + if (notify) + ipv6_ifa_notify(RTM_DELADDR, ifp); inet6addr_notifier_call_chain(NETDEV_DOWN, ifp); @@ -1184,6 +1192,11 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp) in6_ifa_put(ifp); } +static void ipv6_del_addr(struct inet6_ifaddr *ifp) +{ + __ipv6_del_addr(ifp, true); +} + static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *ift) { struct inet6_dev *idev = ifp->idev; -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Ipvlan should return an error when an address is already in use. 2016-12-31 4:10 [PATCH] Ipvlan should return an error when an address is already in use Krister Johansen @ 2017-01-02 3:26 ` David Miller 2017-01-04 10:04 ` Krister Johansen 2017-06-08 20:12 ` [PATCH v2 net-next] " Krister Johansen 2017-01-03 15:50 ` [PATCH] " Aaron Conole 1 sibling, 2 replies; 12+ messages in thread From: David Miller @ 2017-01-02 3:26 UTC (permalink / raw) To: kjlx; +Cc: maheshb, netdev From: Krister Johansen <kjlx@templeofstupid.com> Date: Fri, 30 Dec 2016 20:10:58 -0800 > The ipvlan code already knows how to detect when a duplicate address is > about to be assigned to an ipvlan device. However, that failure is not > propogated outward and leads to a silent failure. This teaches the ip > address addition functions how to report this error to the user > applications so that a notifier chain failure during ip address addition > will not appear to succeed when it actually has not. > > This can be especially useful if it is necessary to provision many > ipvlans in containers. The provisioning software (or operator) can use > this to detect situations where an ip address is unexpectedly in use. > > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> Your patch isn't handling the case of primary address promotions, which also issue NETDEV_UP events on these notifier chains. But on a more basic level, it's extremely important that once you start using the notifier_{from,to}_errno() handling for a notifier, you must start doing so for all such cases of that notifier. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Ipvlan should return an error when an address is already in use. 2017-01-02 3:26 ` David Miller @ 2017-01-04 10:04 ` Krister Johansen 2017-06-08 20:12 ` [PATCH v2 net-next] " Krister Johansen 1 sibling, 0 replies; 12+ messages in thread From: Krister Johansen @ 2017-01-04 10:04 UTC (permalink / raw) To: David Miller; +Cc: kjlx, maheshb, netdev On Sun, Jan 01, 2017 at 10:26:32PM -0500, David Miller wrote: > From: Krister Johansen <kjlx@templeofstupid.com> > Date: Fri, 30 Dec 2016 20:10:58 -0800 > > > The ipvlan code already knows how to detect when a duplicate address is > > about to be assigned to an ipvlan device. However, that failure is not > > propogated outward and leads to a silent failure. This teaches the ip > > address addition functions how to report this error to the user > > applications so that a notifier chain failure during ip address addition > > will not appear to succeed when it actually has not. > > > > This can be especially useful if it is necessary to provision many > > ipvlans in containers. The provisioning software (or operator) can use > > this to detect situations where an ip address is unexpectedly in use. > > > > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> > > Your patch isn't handling the case of primary address promotions, > which also issue NETDEV_UP events on these notifier chains. > > But on a more basic level, it's extremely important that once you > start using the notifier_{from,to}_errno() handling for a notifier, > you must start doing so for all such cases of that notifier. Thanks for taking a look. I'm relatively new to this area of the code. I do appreciate the feedback. In terms of interpreting your final comment, does that mean any call on the inetaddr_chain or inet6addr_chain, and if so would that include all callers of call_netdevice_notifiers()? Another concern that I had with my approach was that the blocking_notifier_call_chain() occurs after a rtmsg_ifa(RTM_NEWADDR) call. In the places where I put rollback code, the address delete ends up generating a corresponding rtmsg_ifa(RTM_DELADDR). This is likely to emit messages that make it look like the address was created and deleted, except that the validation failed before creation was completed. Is the rtmsg_ifa() call consumed by other listeners in the kernel, or is this purely a generation of a rtnetlink message for applications listening in userland? More generally, is using the notifier_call_chain a reasonable way of getting this error information back to userland or would a different approach be better? I'm also concerned about the cases where the code currently treats these invocations as atomic. I had considered introducing an alternate chain that might allow us to check that the operation is valid prior to committing the change, but that seemed potentially racy and that it might involve a lot of extra mechanism. -K ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 net-next] Ipvlan should return an error when an address is already in use. 2017-01-02 3:26 ` David Miller 2017-01-04 10:04 ` Krister Johansen @ 2017-06-08 20:12 ` Krister Johansen 2017-06-09 16:26 ` David Miller 1 sibling, 1 reply; 12+ messages in thread From: Krister Johansen @ 2017-06-08 20:12 UTC (permalink / raw) To: David Miller; +Cc: maheshb, netdev The ipvlan code already knows how to detect when a duplicate address is about to be assigned to an ipvlan device. However, that failure is not propogated outward and leads to a silent failure. Introduce a validation step at ip address creation time and allow device drivers to register to validate the incoming ip addresses. The ipvlan code is the first consumer. If it detects an address in use, we can return an error to the user before beginning to commit the new ifa in the networking code. This can be especially useful if it is necessary to provision many ipvlans in containers. The provisioning software (or operator) can use this to detect situations where an ip address is unexpectedly in use. Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> --- drivers/net/ipvlan/ipvlan_main.c | 69 ++++++++++++++++++++++++++++++++++++++++ include/linux/inetdevice.h | 7 ++++ include/net/addrconf.h | 10 +++++- net/ipv4/devinet.c | 33 +++++++++++++++++++ net/ipv6/addrconf.c | 17 +++++++++- net/ipv6/addrconf_core.c | 19 +++++++++++ 6 files changed, 153 insertions(+), 2 deletions(-) Apologies for letting this one languish for so long. This iteration is more code than I hoped it would be, but I believe it addresses the concerns raised in the prior iteration of the review. Changes v1 -> v2: - Add a separate validator chain with ipvlan as the first consumer. [Address Dave M.'s comment about needing all chain users to agree about use of notifier_[to|from]_errno] - Run validator chain only during address creation. [Address Dave M.'s comment about prior version failing to handle primary address promotions.] - Run validator step before the atomic section in the ip address create path. [Address my own dissatisfaction with having to rollback and potentially issue an immediate up and then down RTNETLINK event.] diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c index 618ed88..e4141d6 100644 --- a/drivers/net/ipvlan/ipvlan_main.c +++ b/drivers/net/ipvlan/ipvlan_main.c @@ -824,6 +824,33 @@ static int ipvlan_addr6_event(struct notifier_block *unused, return NOTIFY_OK; } +static int ipvlan_addr6_validator_event(struct notifier_block *unused, + unsigned long event, void *ptr) +{ + struct in6_validator_info *i6vi = (struct in6_validator_info *)ptr; + struct net_device *dev = (struct net_device *)i6vi->i6vi_dev->dev; + struct ipvl_dev *ipvlan = netdev_priv(dev); + + /* FIXME IPv6 autoconf calls us from bh without RTNL */ + if (in_softirq()) + return NOTIFY_DONE; + + if (!netif_is_ipvlan(dev)) + return NOTIFY_DONE; + + if (!ipvlan || !ipvlan->port) + return NOTIFY_DONE; + + switch (event) { + case NETDEV_UP: + if (ipvlan_addr_busy(ipvlan->port, &i6vi->i6vi_addr, true)) + return notifier_from_errno(-EADDRINUSE); + break; + } + + return NOTIFY_OK; +} + static int ipvlan_add_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr) { if (ipvlan_addr_busy(ipvlan->port, ip4_addr, false)) { @@ -871,10 +898,37 @@ static int ipvlan_addr4_event(struct notifier_block *unused, return NOTIFY_OK; } +static int ipvlan_addr4_validator_event(struct notifier_block *unused, + unsigned long event, void *ptr) +{ + struct in_validator_info *ivi = (struct in_validator_info *)ptr; + struct net_device *dev = (struct net_device *)ivi->ivi_dev->dev; + struct ipvl_dev *ipvlan = netdev_priv(dev); + + if (!netif_is_ipvlan(dev)) + return NOTIFY_DONE; + + if (!ipvlan || !ipvlan->port) + return NOTIFY_DONE; + + switch (event) { + case NETDEV_UP: + if (ipvlan_addr_busy(ipvlan->port, &ivi->ivi_addr, false)) + return notifier_from_errno(-EADDRINUSE); + break; + } + + return NOTIFY_OK; +} + static struct notifier_block ipvlan_addr4_notifier_block __read_mostly = { .notifier_call = ipvlan_addr4_event, }; +static struct notifier_block ipvlan_addr4_vtor_notifier_block __read_mostly = { + .notifier_call = ipvlan_addr4_validator_event, +}; + static struct notifier_block ipvlan_notifier_block __read_mostly = { .notifier_call = ipvlan_device_event, }; @@ -883,6 +937,10 @@ static struct notifier_block ipvlan_addr6_notifier_block __read_mostly = { .notifier_call = ipvlan_addr6_event, }; +static struct notifier_block ipvlan_addr6_vtor_notifier_block __read_mostly = { + .notifier_call = ipvlan_addr6_validator_event, +}; + static void ipvlan_ns_exit(struct net *net) { struct ipvlan_netns *vnet = net_generic(net, ipvlan_netid); @@ -907,7 +965,10 @@ static int __init ipvlan_init_module(void) ipvlan_init_secret(); register_netdevice_notifier(&ipvlan_notifier_block); register_inet6addr_notifier(&ipvlan_addr6_notifier_block); + register_inet6addr_validator_notifier( + &ipvlan_addr6_vtor_notifier_block); register_inetaddr_notifier(&ipvlan_addr4_notifier_block); + register_inetaddr_validator_notifier(&ipvlan_addr4_vtor_notifier_block); err = register_pernet_subsys(&ipvlan_net_ops); if (err < 0) @@ -922,7 +983,11 @@ static int __init ipvlan_init_module(void) return 0; error: unregister_inetaddr_notifier(&ipvlan_addr4_notifier_block); + unregister_inetaddr_validator_notifier( + &ipvlan_addr4_vtor_notifier_block); unregister_inet6addr_notifier(&ipvlan_addr6_notifier_block); + unregister_inet6addr_validator_notifier( + &ipvlan_addr6_vtor_notifier_block); unregister_netdevice_notifier(&ipvlan_notifier_block); return err; } @@ -933,7 +998,11 @@ static void __exit ipvlan_cleanup_module(void) unregister_pernet_subsys(&ipvlan_net_ops); unregister_netdevice_notifier(&ipvlan_notifier_block); unregister_inetaddr_notifier(&ipvlan_addr4_notifier_block); + unregister_inetaddr_validator_notifier( + &ipvlan_addr4_vtor_notifier_block); unregister_inet6addr_notifier(&ipvlan_addr6_notifier_block); + unregister_inet6addr_validator_notifier( + &ipvlan_addr6_vtor_notifier_block); } module_init(ipvlan_init_module); diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h index a2e9d6e..e7c04c4 100644 --- a/include/linux/inetdevice.h +++ b/include/linux/inetdevice.h @@ -150,8 +150,15 @@ struct in_ifaddr { unsigned long ifa_tstamp; /* updated timestamp */ }; +struct in_validator_info { + __be32 ivi_addr; + struct in_device *ivi_dev; +}; + int register_inetaddr_notifier(struct notifier_block *nb); int unregister_inetaddr_notifier(struct notifier_block *nb); +int register_inetaddr_validator_notifier(struct notifier_block *nb); +int unregister_inetaddr_validator_notifier(struct notifier_block *nb); void inet_netconf_notify_devconf(struct net *net, int event, int type, int ifindex, struct ipv4_devconf *devconf); diff --git a/include/net/addrconf.h b/include/net/addrconf.h index b43a4ee..d0889cb 100644 --- a/include/net/addrconf.h +++ b/include/net/addrconf.h @@ -48,11 +48,15 @@ struct prefix_info { struct in6_addr prefix; }; - #include <linux/netdevice.h> #include <net/if_inet6.h> #include <net/ipv6.h> +struct in6_validator_info { + struct in6_addr i6vi_addr; + struct inet6_dev *i6vi_dev; +}; + #define IN6_ADDR_HSIZE_SHIFT 4 #define IN6_ADDR_HSIZE (1 << IN6_ADDR_HSIZE_SHIFT) @@ -278,6 +282,10 @@ int register_inet6addr_notifier(struct notifier_block *nb); int unregister_inet6addr_notifier(struct notifier_block *nb); int inet6addr_notifier_call_chain(unsigned long val, void *v); +int register_inet6addr_validator_notifier(struct notifier_block *nb); +int unregister_inet6addr_validator_notifier(struct notifier_block *nb); +int inet6addr_validator_notifier_call_chain(unsigned long val, void *v); + void inet6_netconf_notify_devconf(struct net *net, int event, int type, int ifindex, struct ipv6_devconf *devconf); diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index df14815..a7dd088 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -176,6 +176,7 @@ EXPORT_SYMBOL(__ip_dev_find); static void rtmsg_ifa(int event, struct in_ifaddr *, struct nlmsghdr *, u32); static BLOCKING_NOTIFIER_HEAD(inetaddr_chain); +static BLOCKING_NOTIFIER_HEAD(inetaddr_validator_chain); static void inet_del_ifa(struct in_device *in_dev, struct in_ifaddr **ifap, int destroy); #ifdef CONFIG_SYSCTL @@ -441,6 +442,8 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh, { struct in_device *in_dev = ifa->ifa_dev; struct in_ifaddr *ifa1, **ifap, **last_primary; + struct in_validator_info ivi; + int ret; ASSERT_RTNL(); @@ -471,6 +474,23 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh, } } + /* Allow any devices that wish to register ifaddr validtors to weigh + * in now, before changes are committed. The rntl lock is serializing + * access here, so the state should not change between a validator call + * and a final notify on commit. This isn't invoked on promotion under + * the assumption that validators are checking the address itself, and + * not the flags. + */ + ivi.ivi_addr = ifa->ifa_address; + ivi.ivi_dev = ifa->ifa_dev; + ret = blocking_notifier_call_chain(&inetaddr_validator_chain, + NETDEV_UP, &ivi); + ret = notifier_to_errno(ret); + if (ret) { + inet_free_ifa(ifa); + return ret; + } + if (!(ifa->ifa_flags & IFA_F_SECONDARY)) { prandom_seed((__force u32) ifa->ifa_local); ifap = last_primary; @@ -1356,6 +1376,19 @@ int unregister_inetaddr_notifier(struct notifier_block *nb) } EXPORT_SYMBOL(unregister_inetaddr_notifier); +int register_inetaddr_validator_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_register(&inetaddr_validator_chain, nb); +} +EXPORT_SYMBOL(register_inetaddr_validator_notifier); + +int unregister_inetaddr_validator_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_unregister(&inetaddr_validator_chain, + nb); +} +EXPORT_SYMBOL(unregister_inetaddr_validator_notifier); + /* Rename ifa_labels for a device name change. Make some effort to preserve * existing alias numbering and to create unique labels if possible. */ diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 25443fd..0aa36b0 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -963,6 +963,7 @@ 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); @@ -974,6 +975,9 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, return ERR_PTR(-EADDRNOTAVAIL); rcu_read_lock_bh(); + + in6_dev_hold(idev); + if (idev->dead) { err = -ENODEV; /*XXX*/ goto out2; @@ -984,6 +988,17 @@ 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(); + + err = inet6addr_validator_notifier_call_chain(NETDEV_UP, &i6vi); + + rcu_read_lock_bh(); + err = notifier_to_errno(err); + if (err) + goto out2; + spin_lock(&addrconf_hash_lock); /* Ignore adding duplicate addresses on an interface */ @@ -1034,7 +1049,6 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, ifa->rt = rt; ifa->idev = idev; - in6_dev_hold(idev); /* For caller */ in6_ifa_hold(ifa); @@ -1062,6 +1076,7 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, inet6addr_notifier_call_chain(NETDEV_UP, ifa); else { kfree(ifa); + in6_dev_put(idev); ifa = ERR_PTR(err); } diff --git a/net/ipv6/addrconf_core.c b/net/ipv6/addrconf_core.c index bfa941f..9e3488d 100644 --- a/net/ipv6/addrconf_core.c +++ b/net/ipv6/addrconf_core.c @@ -88,6 +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); int register_inet6addr_notifier(struct notifier_block *nb) { @@ -107,6 +108,24 @@ int inet6addr_notifier_call_chain(unsigned long val, void *v) } EXPORT_SYMBOL(inet6addr_notifier_call_chain); +int register_inet6addr_validator_notifier(struct notifier_block *nb) +{ + return atomic_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); +} +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); +} +EXPORT_SYMBOL(inet6addr_validator_notifier_call_chain); + static int eafnosupport_ipv6_dst_lookup(struct net *net, struct sock *u1, struct dst_entry **u2, struct flowi6 *u3) -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net-next] Ipvlan should return an error when an address is already in use. 2017-06-08 20:12 ` [PATCH v2 net-next] " Krister Johansen @ 2017-06-09 16:26 ` David Miller 2017-06-09 17:13 ` Krister Johansen 0 siblings, 1 reply; 12+ messages in thread From: David Miller @ 2017-06-09 16:26 UTC (permalink / raw) To: kjlx; +Cc: maheshb, netdev From: Krister Johansen <kjlx@templeofstupid.com> Date: Thu, 8 Jun 2017 13:12:14 -0700 > The ipvlan code already knows how to detect when a duplicate address is > about to be assigned to an ipvlan device. However, that failure is not > propogated outward and leads to a silent failure. > > Introduce a validation step at ip address creation time and allow device > drivers to register to validate the incoming ip addresses. The ipvlan > code is the first consumer. If it detects an address in use, we can > return an error to the user before beginning to commit the new ifa in > the networking code. > > This can be especially useful if it is necessary to provision many > ipvlans in containers. The provisioning software (or operator) can use > this to detect situations where an ip address is unexpectedly in use. > > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> Ok, applied, thank you. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net-next] Ipvlan should return an error when an address is already in use. 2017-06-09 16:26 ` David Miller @ 2017-06-09 17:13 ` Krister Johansen 2017-06-09 17:15 ` David Miller 0 siblings, 1 reply; 12+ messages in thread From: Krister Johansen @ 2017-06-09 17:13 UTC (permalink / raw) To: David Miller; +Cc: kjlx, maheshb, netdev On Fri, Jun 09, 2017 at 12:26:46PM -0400, David Miller wrote: > From: Krister Johansen <kjlx@templeofstupid.com> > Date: Thu, 8 Jun 2017 13:12:14 -0700 > > > The ipvlan code already knows how to detect when a duplicate address is > > about to be assigned to an ipvlan device. However, that failure is not > > propogated outward and leads to a silent failure. > > > > Introduce a validation step at ip address creation time and allow device > > drivers to register to validate the incoming ip addresses. The ipvlan > > code is the first consumer. If it detects an address in use, we can > > return an error to the user before beginning to commit the new ifa in > > the networking code. > > > > This can be especially useful if it is necessary to provision many > > ipvlans in containers. The provisioning software (or operator) can use > > this to detect situations where an ip address is unexpectedly in use. > > > > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> > > Ok, applied, thank you. Thanks, did this look otherwise alright? I was a little nervous about dropping and re-acquiring the rcu_read_lock_bh() in net/ipv6/addrconf.c around line 975, but in the current design holding rcu_read_lock_bh() causes the in_softirq() check in the validator (and the add/remove ipvlan code itself) to return NOTIFY_DONE immediately. AFAICT, the rcu_read_lock was to protect the idev. I changed that to get subsequently released in the outbound path. I was also unsure if it's safe to call in6_dev_put from a bh context. Thanks, -K ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net-next] Ipvlan should return an error when an address is already in use. 2017-06-09 17:13 ` Krister Johansen @ 2017-06-09 17:15 ` David Miller 2017-06-09 17:25 ` Krister Johansen 0 siblings, 1 reply; 12+ messages in thread From: David Miller @ 2017-06-09 17:15 UTC (permalink / raw) To: kjlx; +Cc: maheshb, netdev From: Krister Johansen <kjlx@templeofstupid.com> Date: Fri, 9 Jun 2017 10:13:10 -0700 > On Fri, Jun 09, 2017 at 12:26:46PM -0400, David Miller wrote: >> From: Krister Johansen <kjlx@templeofstupid.com> >> Date: Thu, 8 Jun 2017 13:12:14 -0700 >> >> > The ipvlan code already knows how to detect when a duplicate address is >> > about to be assigned to an ipvlan device. However, that failure is not >> > propogated outward and leads to a silent failure. >> > >> > Introduce a validation step at ip address creation time and allow device >> > drivers to register to validate the incoming ip addresses. The ipvlan >> > code is the first consumer. If it detects an address in use, we can >> > return an error to the user before beginning to commit the new ifa in >> > the networking code. >> > >> > This can be especially useful if it is necessary to provision many >> > ipvlans in containers. The provisioning software (or operator) can use >> > this to detect situations where an ip address is unexpectedly in use. >> > >> > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> >> >> Ok, applied, thank you. > > Thanks, did this look otherwise alright? Yes, I was mildly unsatisfied with the ipv6 addrconf situation but I know very well about that, and those kinds of addresses aren't of interest for what you are trying to achieve right? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 net-next] Ipvlan should return an error when an address is already in use. 2017-06-09 17:15 ` David Miller @ 2017-06-09 17:25 ` Krister Johansen 0 siblings, 0 replies; 12+ messages in thread From: Krister Johansen @ 2017-06-09 17:25 UTC (permalink / raw) To: David Miller; +Cc: kjlx, maheshb, netdev On Fri, Jun 09, 2017 at 01:15:10PM -0400, David Miller wrote: > From: Krister Johansen <kjlx@templeofstupid.com> > Date: Fri, 9 Jun 2017 10:13:10 -0700 > > > On Fri, Jun 09, 2017 at 12:26:46PM -0400, David Miller wrote: > >> From: Krister Johansen <kjlx@templeofstupid.com> > >> Date: Thu, 8 Jun 2017 13:12:14 -0700 > >> > >> > The ipvlan code already knows how to detect when a duplicate address is > >> > about to be assigned to an ipvlan device. However, that failure is not > >> > propogated outward and leads to a silent failure. > >> > > >> > Introduce a validation step at ip address creation time and allow device > >> > drivers to register to validate the incoming ip addresses. The ipvlan > >> > code is the first consumer. If it detects an address in use, we can > >> > return an error to the user before beginning to commit the new ifa in > >> > the networking code. > >> > > >> > This can be especially useful if it is necessary to provision many > >> > ipvlans in containers. The provisioning software (or operator) can use > >> > this to detect situations where an ip address is unexpectedly in use. > >> > > >> > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> > >> > >> Ok, applied, thank you. > > > > Thanks, did this look otherwise alright? > > Yes, I was mildly unsatisfied with the ipv6 addrconf situation but I know > very well about that, and those kinds of addresses aren't of interest > for what you are trying to achieve right? That's correct. I'm basically trying to catch the case where 'ip addr' or its equivalent rtnetlink invocation manually configure an ip address on a ipvlan. -K ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Ipvlan should return an error when an address is already in use. 2016-12-31 4:10 [PATCH] Ipvlan should return an error when an address is already in use Krister Johansen 2017-01-02 3:26 ` David Miller @ 2017-01-03 15:50 ` Aaron Conole 2017-01-03 15:55 ` David Miller 1 sibling, 1 reply; 12+ messages in thread From: Aaron Conole @ 2017-01-03 15:50 UTC (permalink / raw) To: Krister Johansen; +Cc: David S. Miller, Mahesh Bandewar, netdev Hi Krister, Krister Johansen <kjlx@templeofstupid.com> writes: > The ipvlan code already knows how to detect when a duplicate address is > about to be assigned to an ipvlan device. However, that failure is not > propogated outward and leads to a silent failure. This teaches the ip > address addition functions how to report this error to the user > applications so that a notifier chain failure during ip address addition > will not appear to succeed when it actually has not. > > This can be especially useful if it is necessary to provision many > ipvlans in containers. The provisioning software (or operator) can use > this to detect situations where an ip address is unexpectedly in use. > > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> > --- ... > @@ -489,7 +490,12 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh, > Notifier will trigger FIB update, so that > listeners of netlink will know about new ifaddr */ > rtmsg_ifa(RTM_NEWADDR, ifa, nlh, portid); > - blocking_notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa); > + ret = blocking_notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa); Why are you doing this assignment if you aren't using the result? > + ret = notifier_to_errno(ret); > + if (ret) { > + __inet_del_ifa(in_dev, ifap, 1, NULL, portid); > + return ret; > + } > > return 0; > } <<snip>> > @@ -1031,9 +1032,15 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, > out2: > rcu_read_unlock_bh(); > > - if (likely(err == 0)) > - inet6addr_notifier_call_chain(NETDEV_UP, ifa); > - else { > + if (likely(err == 0)) { > + err = inet6addr_notifier_call_chain(NETDEV_UP, ifa); Same here... > + err = notifier_to_errno(err); > + if (err) { > + __ipv6_del_addr(ifa, false); > + ifa = ERR_PTR(err); > + return ifa; > + } > + } else { > kfree(ifa); > ifa = ERR_PTR(err); > } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Ipvlan should return an error when an address is already in use. 2017-01-03 15:50 ` [PATCH] " Aaron Conole @ 2017-01-03 15:55 ` David Miller 2017-01-03 19:24 ` Aaron Conole 0 siblings, 1 reply; 12+ messages in thread From: David Miller @ 2017-01-03 15:55 UTC (permalink / raw) To: aconole; +Cc: kjlx, maheshb, netdev From: Aaron Conole <aconole@redhat.com> Date: Tue, 03 Jan 2017 10:50:00 -0500 >> @@ -489,7 +490,12 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh, >> Notifier will trigger FIB update, so that >> listeners of netlink will know about new ifaddr */ >> rtmsg_ifa(RTM_NEWADDR, ifa, nlh, portid); >> - blocking_notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa); >> + ret = blocking_notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa); > > Why are you doing this assignment if you aren't using the result? > >> + ret = notifier_to_errno(ret); >> + if (ret) { >> + __inet_del_ifa(in_dev, ifap, 1, NULL, portid); >> + return ret; >> + } 'ret' assignment is being used, via notifier_to_errno(). ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Ipvlan should return an error when an address is already in use. 2017-01-03 15:55 ` David Miller @ 2017-01-03 19:24 ` Aaron Conole 2017-01-04 10:09 ` Krister Johansen 0 siblings, 1 reply; 12+ messages in thread From: Aaron Conole @ 2017-01-03 19:24 UTC (permalink / raw) To: David Miller; +Cc: kjlx, maheshb, netdev David Miller <davem@davemloft.net> writes: > From: Aaron Conole <aconole@redhat.com> > Date: Tue, 03 Jan 2017 10:50:00 -0500 > >>> @@ -489,7 +490,12 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh, >>> Notifier will trigger FIB update, so that >>> listeners of netlink will know about new ifaddr */ >>> rtmsg_ifa(RTM_NEWADDR, ifa, nlh, portid); >>> - blocking_notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa); >>> + ret = blocking_notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa); >> >> Why are you doing this assignment if you aren't using the result? >> >>> + ret = notifier_to_errno(ret); >>> + if (ret) { >>> + __inet_del_ifa(in_dev, ifap, 1, NULL, portid); >>> + return ret; >>> + } > > 'ret' assignment is being used, via notifier_to_errno(). d'oh! should have had more coffee - sorry for the noise. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Ipvlan should return an error when an address is already in use. 2017-01-03 19:24 ` Aaron Conole @ 2017-01-04 10:09 ` Krister Johansen 0 siblings, 0 replies; 12+ messages in thread From: Krister Johansen @ 2017-01-04 10:09 UTC (permalink / raw) To: Aaron Conole; +Cc: David Miller, kjlx, maheshb, netdev On Tue, Jan 03, 2017 at 02:24:43PM -0500, Aaron Conole wrote: > David Miller <davem@davemloft.net> writes: > > > From: Aaron Conole <aconole@redhat.com> > > Date: Tue, 03 Jan 2017 10:50:00 -0500 > > > >>> @@ -489,7 +490,12 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh, > >>> Notifier will trigger FIB update, so that > >>> listeners of netlink will know about new ifaddr */ > >>> rtmsg_ifa(RTM_NEWADDR, ifa, nlh, portid); > >>> - blocking_notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa); > >>> + ret = blocking_notifier_call_chain(&inetaddr_chain, NETDEV_UP, ifa); > >> > >> Why are you doing this assignment if you aren't using the result? > >> > >>> + ret = notifier_to_errno(ret); > >>> + if (ret) { > >>> + __inet_del_ifa(in_dev, ifap, 1, NULL, portid); > >>> + return ret; > >>> + } > > > > 'ret' assignment is being used, via notifier_to_errno(). > > d'oh! should have had more coffee - sorry for the noise. No worries, and thanks for taking a look. That was from checkpatch, which complained when I wrote it as: if ((ret = notifier_to_errno(ret)) != 0) { -K ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-06-09 17:24 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-31 4:10 [PATCH] Ipvlan should return an error when an address is already in use Krister Johansen 2017-01-02 3:26 ` David Miller 2017-01-04 10:04 ` Krister Johansen 2017-06-08 20:12 ` [PATCH v2 net-next] " Krister Johansen 2017-06-09 16:26 ` David Miller 2017-06-09 17:13 ` Krister Johansen 2017-06-09 17:15 ` David Miller 2017-06-09 17:25 ` Krister Johansen 2017-01-03 15:50 ` [PATCH] " Aaron Conole 2017-01-03 15:55 ` David Miller 2017-01-03 19:24 ` Aaron Conole 2017-01-04 10:09 ` Krister Johansen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).