| Message ID | 20161231041058.GC2448@templeofstupid.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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); > }
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().
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.
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
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
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;
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(-)