Message ID | 1358393418.3855.3.camel@cr0 |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
From: Cong Wang <amwang@redhat.com> Date: Thu, 17 Jan 2013 11:30:18 +0800 > On Wed, 2013-01-16 at 17:24 -0800, Eric Dumazet wrote: >> On Tue, 2013-01-15 at 17:34 +0800, Cong Wang wrote: >> > if (np->dev_name) >> > - ndev = dev_get_by_name(&init_net, np->dev_name); >> > + ndev = __dev_get_by_name(&init_net, np->dev_name); >> >> This change brings interesting bugs. > > Hmm, I didn't realize __dev_get_by_name() doesn't hold the device, so > just call dev_hold() after this? Why not just... call dev_get_by_name()? It doesn't hurt to over-RCU lock. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-01-16 at 22:54 -0500, David Miller wrote: > From: Cong Wang <amwang@redhat.com> > Date: Thu, 17 Jan 2013 11:30:18 +0800 > > > On Wed, 2013-01-16 at 17:24 -0800, Eric Dumazet wrote: > >> On Tue, 2013-01-15 at 17:34 +0800, Cong Wang wrote: > >> > if (np->dev_name) > >> > - ndev = dev_get_by_name(&init_net, np->dev_name); > >> > + ndev = __dev_get_by_name(&init_net, np->dev_name); > >> > >> This change brings interesting bugs. > > > > Hmm, I didn't realize __dev_get_by_name() doesn't hold the device, so > > just call dev_hold() after this? > > Why not just... call dev_get_by_name()? It doesn't hurt to over-RCU > lock. > Just that taking RCU read lock while having rtnl lock is unnecessary, no other reason. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-01-17 at 12:18 +0800, Cong Wang wrote: > > Why not just... call dev_get_by_name()? It doesn't hurt to over-RCU > > lock. > > > > Just that taking RCU read lock while having rtnl lock is unnecessary, no > other reason. Calling the dev_get_by_name() would be just fine, and generate less code. Its not a fast path... Anyway David already applied your patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/netpoll.c b/net/core/netpoll.c index a5ad1c1..a9b1004 100644 --- a/net/core/netpoll.c +++ b/net/core/netpoll.c @@ -1056,6 +1056,7 @@ int netpoll_setup(struct netpoll *np) err = -ENODEV; goto unlock; } + dev_hold(ndev); if (netdev_master_upper_dev_get(ndev)) { np_err(np, "%s is a slave device, aborting\n",