Message ID | 20160325025621.32476.93348.stgit@bhelgaas-glaptop2.roam.corp.google.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, Mar 24, 2016 at 09:56:21PM -0500, Bjorn Helgaas wrote: > netpoll_setup() does a dev_hold() on np->dev, the netpoll device. If it > fails, it correctly does a dev_put() but leaves np->dev set. If we call > netpoll_cleanup() after the failure, np->dev is still set so we do another > dev_put(), which decrements the refcount an extra time. > > It's questionable to call netpoll_cleanup() after netpoll_setup() fails, > but it can be difficult to find the problem, and we can easily avoid it in > this case. The extra decrements can lead to hangs like this: > > unregister_netdevice: waiting for bond0 to become free. Usage count = -3 > > Set and clear np->dev at the points where we dev_hold() and dev_put() the > device. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > net/core/netpoll.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/core/netpoll.c b/net/core/netpoll.c > index 94acfc8..a57bd17 100644 > --- a/net/core/netpoll.c > +++ b/net/core/netpoll.c > @@ -603,7 +603,6 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev) > const struct net_device_ops *ops; > int err; > > - np->dev = ndev; > strlcpy(np->dev_name, ndev->name, IFNAMSIZ); > INIT_WORK(&np->cleanup_work, netpoll_async_cleanup); > > @@ -670,6 +669,7 @@ int netpoll_setup(struct netpoll *np) > goto unlock; > } > dev_hold(ndev); > + np->dev = ndev; > > if (netdev_master_upper_dev_get(ndev)) { > np_err(np, "%s is a slave device, aborting\n", np->dev_name); > @@ -770,6 +770,7 @@ int netpoll_setup(struct netpoll *np) > return 0; > > put: > + np->dev = NULL; > dev_put(ndev); > unlock: > rtnl_unlock(); > Is this safe for stacked devices? It makes good sense for the typical case, but if you attempt to setup a netpoll client on a bridge/bond/vlan, etc, the lower device will get its own netpoll struct registered and have no associated np->dev pointer. It not be a real problem in practice, But you probably want to check to make sure that stacked devices which recursively call the netpoll api don't do anyting with the np->dev pointer. Regards Neil
From: Bjorn Helgaas <bhelgaas@google.com> Date: Thu, 24 Mar 2016 21:56:21 -0500 > netpoll_setup() does a dev_hold() on np->dev, the netpoll device. If it > fails, it correctly does a dev_put() but leaves np->dev set. If we call > netpoll_cleanup() after the failure, np->dev is still set so we do another > dev_put(), which decrements the refcount an extra time. > > It's questionable to call netpoll_cleanup() after netpoll_setup() fails, > but it can be difficult to find the problem, and we can easily avoid it in > this case. The extra decrements can lead to hangs like this: > > unregister_netdevice: waiting for bond0 to become free. Usage count = -3 > > Set and clear np->dev at the points where we dev_hold() and dev_put() the > device. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Looks good, applied and queued up for -stable. But you probably do want to look into the stacked device issue Neil pointed out. Thanks!
On Fri, Mar 25, 2016 at 07:33:42AM -0400, Neil Horman wrote: > On Thu, Mar 24, 2016 at 09:56:21PM -0500, Bjorn Helgaas wrote: > > netpoll_setup() does a dev_hold() on np->dev, the netpoll device. If it > > fails, it correctly does a dev_put() but leaves np->dev set. If we call > > netpoll_cleanup() after the failure, np->dev is still set so we do another > > dev_put(), which decrements the refcount an extra time. > > > > It's questionable to call netpoll_cleanup() after netpoll_setup() fails, > > but it can be difficult to find the problem, and we can easily avoid it in > > this case. The extra decrements can lead to hangs like this: > > > > unregister_netdevice: waiting for bond0 to become free. Usage count = -3 > > > > Set and clear np->dev at the points where we dev_hold() and dev_put() the > > device. > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > --- > > net/core/netpoll.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/net/core/netpoll.c b/net/core/netpoll.c > > index 94acfc8..a57bd17 100644 > > --- a/net/core/netpoll.c > > +++ b/net/core/netpoll.c > > @@ -603,7 +603,6 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev) > > const struct net_device_ops *ops; > > int err; > > > > - np->dev = ndev; > > strlcpy(np->dev_name, ndev->name, IFNAMSIZ); > > INIT_WORK(&np->cleanup_work, netpoll_async_cleanup); > > > > @@ -670,6 +669,7 @@ int netpoll_setup(struct netpoll *np) > > goto unlock; > > } > > dev_hold(ndev); > > + np->dev = ndev; > > > > if (netdev_master_upper_dev_get(ndev)) { > > np_err(np, "%s is a slave device, aborting\n", np->dev_name); > > @@ -770,6 +770,7 @@ int netpoll_setup(struct netpoll *np) > > return 0; > > > > put: > > + np->dev = NULL; > > dev_put(ndev); > > unlock: > > rtnl_unlock(); > > > > Is this safe for stacked devices? It makes good sense for the typical case, but > if you attempt to setup a netpoll client on a bridge/bond/vlan, etc, the lower > device will get its own netpoll struct registered and have no associated np->dev > pointer. It not be a real problem in practice, But you probably want to check > to make sure that stacked devices which recursively call the netpoll api don't > do anyting with the np->dev pointer. You're right, there is an issue here. I reproduced a problem with a bond device. bond_netpoll_setup() calls __netpoll_setup() directly (not netpoll_setup()). I'll debug it more; just wanted to let you know there *is* a problem with this patch. Bjorn
From: Bjorn Helgaas <helgaas@kernel.org> Date: Fri, 25 Mar 2016 11:46:39 -0500 > You're right, there is an issue here. I reproduced a problem with a > bond device. bond_netpoll_setup() calls __netpoll_setup() directly > (not netpoll_setup()). I'll debug it more; just wanted to let you > know there *is* a problem with this patch. I bet that's why the assignment to np->dev and the reference counting were separated in the first place :-/ Indeed, commit 30fdd8a082a00126a6feec994e43e8dc12f5bccb: commit 30fdd8a082a00126a6feec994e43e8dc12f5bccb Author: Jiri Pirko <jiri@resnulli.us> Date: Tue Jul 17 05:22:35 2012 +0000 netpoll: move np->dev and np->dev_name init into __netpoll_setup() Signed-off-by: Jiri Pirko <jiri@resnulli.us> Signed-off-by: David S. Miller <davem@davemloft.net>
On Fri, Mar 25, 2016 at 03:16:36PM -0400, David Miller wrote: > From: Bjorn Helgaas <helgaas@kernel.org> > Date: Fri, 25 Mar 2016 11:46:39 -0500 > > > You're right, there is an issue here. I reproduced a problem with a > > bond device. bond_netpoll_setup() calls __netpoll_setup() directly > > (not netpoll_setup()). I'll debug it more; just wanted to let you > > know there *is* a problem with this patch. > > I bet that's why the assignment to np->dev and the reference counting > were separated in the first place :-/ > > Indeed, commit 30fdd8a082a00126a6feec994e43e8dc12f5bccb: > > commit 30fdd8a082a00126a6feec994e43e8dc12f5bccb > Author: Jiri Pirko <jiri@resnulli.us> > Date: Tue Jul 17 05:22:35 2012 +0000 > > netpoll: move np->dev and np->dev_name init into __netpoll_setup() > > Signed-off-by: Jiri Pirko <jiri@resnulli.us> > Signed-off-by: David S. Miller <davem@davemloft.net> We probably just want to balance the setting/clearing of np->dev in __netpoll_setup, so that any error return (that would result in a drop of the refcount in netpoll_setup) correlates to a setting of np->dev to NULL in __netpoll_setup. That leaves us with the problem of having to watch for future imbalances as you mentioned previously Dave, but it seems a potential problem tomorrow is preferable to an actual problem today. Another option would be to move the dev_hold/put into __netpoll_setup, but doing so would I think require some additional refactoring in netpoll_setup. Namely that we would still need a dev_hold/put in netpoll_setup to prevent the device from being removed during the period where we release the rtnl lock in the if (!netif_running(ndev)) clause. We would have to hold the device, unlock rtnl, then put the device after re-aquiring rtnl at the end of that if block. Neil
diff --git a/net/core/netpoll.c b/net/core/netpoll.c index 94acfc8..a57bd17 100644 --- a/net/core/netpoll.c +++ b/net/core/netpoll.c @@ -603,7 +603,6 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev) const struct net_device_ops *ops; int err; - np->dev = ndev; strlcpy(np->dev_name, ndev->name, IFNAMSIZ); INIT_WORK(&np->cleanup_work, netpoll_async_cleanup); @@ -670,6 +669,7 @@ int netpoll_setup(struct netpoll *np) goto unlock; } dev_hold(ndev); + np->dev = ndev; if (netdev_master_upper_dev_get(ndev)) { np_err(np, "%s is a slave device, aborting\n", np->dev_name); @@ -770,6 +770,7 @@ int netpoll_setup(struct netpoll *np) return 0; put: + np->dev = NULL; dev_put(ndev); unlock: rtnl_unlock();
netpoll_setup() does a dev_hold() on np->dev, the netpoll device. If it fails, it correctly does a dev_put() but leaves np->dev set. If we call netpoll_cleanup() after the failure, np->dev is still set so we do another dev_put(), which decrements the refcount an extra time. It's questionable to call netpoll_cleanup() after netpoll_setup() fails, but it can be difficult to find the problem, and we can easily avoid it in this case. The extra decrements can lead to hangs like this: unregister_netdevice: waiting for bond0 to become free. Usage count = -3 Set and clear np->dev at the points where we dev_hold() and dev_put() the device. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- net/core/netpoll.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)