Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761954AbXENBfE (ORCPT ); Sun, 13 May 2007 21:35:04 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1758435AbXENBex (ORCPT ); Sun, 13 May 2007 21:34:53 -0400 Received: from fgwmail6.fujitsu.co.jp ([192.51.44.36]:46406 "EHLO fgwmail6.fujitsu.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755240AbXENBew (ORCPT ); Sun, 13 May 2007 21:34:52 -0400 Subject: Re: [BUG][PATCH] Fix race condition about network device name allocation From: Kenji Kaneshige To: Stephen Hemminger Cc: netdev@vger.kernel.org, linux-kernel , Andrew Morton In-Reply-To: <20070511092519.1f34ab34@freepuppy> References: <1178862045.3979.33.camel@kane-linux> <20070511092519.1f34ab34@freepuppy> Content-Type: text/plain; charset=ISO-2022-JP Date: Mon, 14 May 2007 10:33:01 +0900 Message-Id: <1179106381.3881.16.camel@kane-linux> Mime-Version: 1.0 X-Mailer: Evolution 2.8.0 (2.8.0-7.fc6) Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8380 Lines: 191 Hi Stephen, Thank you for comments. I'll try your patch. I have one concern about your patch, though I don't know very much about netdev codes. @@ -3411,6 +3410,9 @@ void unregister_netdevice(struct net_dev > /* Notifier chain MUST detach us from master device. */ > BUG_TRAP(!dev->master); > > + /* Remove entries from sysfs */ > + netdev_unregister_sysfs(dev); > + > /* Finish processing unregister after unlock */ > net_set_todo(dev); > With your patch, the netdev_unregister_sysfs() will be called earlier than now. Does it have no side effect? I'm worrying about if there are somebody that depend on sysfs entry until net_run_todo() is called. Anyway, I'll try your patch. Thanks, Kenji Kaneshige 2007-05-11 (金) の 09:25 -0700 に Stephen Hemminger さんは書きました: > On Fri, 11 May 2007 14:40:45 +0900 > Kenji Kaneshige wrote: > > > Hi, > > > > I encountered the following error when I was hot-plugging network card > > using pci hotplug driver. > > > > kobject_add failed for eth8 with -EEXIST, don't try to register things with the same name in the same directory. > > > > Call Trace: > > [] show_stack+0x40/0xa0 > > sp=e0000000652cfbf0 bsp=e0000000652c9450 > > [] dump_stack+0x30/0x60 > > sp=e0000000652cfdc0 bsp=e0000000652c9438 > > [] kobject_shadow_add+0x3b0/0x440 > > sp=e0000000652cfdc0 bsp=e0000000652c93f0 > > [] kobject_add+0x30/0x60 > > sp=e0000000652cfdc0 bsp=e0000000652c93d0 > > [] device_add+0x160/0xf40 > > sp=e0000000652cfdc0 bsp=e0000000652c9358 > > [] netdev_register_sysfs+0xc0/0xe0 > > sp=e0000000652cfdc0 bsp=e0000000652c9328 > > [] register_netdevice+0x600/0x7e0 > > sp=e0000000652cfdc0 bsp=e0000000652c92e8 > > [] register_netdev+0x70/0xc0 > > sp=e0000000652cfdd0 bsp=e0000000652c92c0 > > [] e1000_probe+0x1710/0x1a00 [e1000] > > sp=e0000000652cfdd0 bsp=e0000000652c9258 > > [] pci_device_probe+0xc0/0x120 > > sp=e0000000652cfde0 bsp=e0000000652c9218 > > [] really_probe+0x1c0/0x300 > > sp=e0000000652cfde0 bsp=e0000000652c91c8 > > [] driver_probe_device+0x1a0/0x1c0 > > sp=e0000000652cfde0 bsp=e0000000652c9198 > > [] __device_attach+0x30/0x60 > > sp=e0000000652cfde0 bsp=e0000000652c9170 > > [] bus_for_each_drv+0x80/0x120 > > sp=e0000000652cfde0 bsp=e0000000652c9138 > > [] device_attach+0xa0/0x100 > > sp=e0000000652cfe00 bsp=e0000000652c9110 > > [] bus_attach_device+0x60/0xe0 > > sp=e0000000652cfe00 bsp=e0000000652c90e0 > > [] device_add+0x950/0xf40 > > sp=e0000000652cfe00 bsp=e0000000652c9068 > > [] pci_bus_add_device+0x20/0x100 > > sp=e0000000652cfe00 bsp=e0000000652c9040 > > [] pci_bus_add_devices+0x50/0x320 > > sp=e0000000652cfe00 bsp=e0000000652c9010 > > [] shpchp_configure_device+0x460/0x4a0 [shpchp] > > sp=e0000000652cfe00 bsp=e0000000652c8fc0 > > [] board_added+0x720/0x8c0 [shpchp] > > sp=e0000000652cfe00 bsp=e0000000652c8f80 > > [] shpchp_enable_slot+0x920/0xa40 [shpchp] > > sp=e0000000652cfe10 bsp=e0000000652c8f30 > > [] shpchp_sysfs_enable_slot+0x120/0x220 [shpchp] > > sp=e0000000652cfe20 bsp=e0000000652c8ef8 > > [] enable_slot+0x90/0xc0 [shpchp] > > sp=e0000000652cfe20 bsp=e0000000652c8ed0 > > [] power_write_file+0x1e0/0x2a0 [pci_hotplug] > > sp=e0000000652cfe20 bsp=e0000000652c8ea0 > > [] hotplug_slot_attr_store+0x60/0xa0 [pci_hotplug] > > sp=e0000000652cfe20 bsp=e0000000652c8e68 > > [] sysfs_write_file+0x270/0x300 > > sp=e0000000652cfe20 bsp=e0000000652c8e18 > > [] vfs_write+0x1b0/0x300 > > sp=e0000000652cfe20 bsp=e0000000652c8dc0 > > [] sys_write+0x70/0xe0 > > sp=e0000000652cfe20 bsp=e0000000652c8d48 > > [] ia64_ret_from_syscall+0x0/0x20 > > sp=e0000000652cfe30 bsp=e0000000652c8d48 > > [] __kernel_syscall_via_break+0x0/0x20 > > sp=e0000000652d0000 bsp=e0000000652c8d48 > > > > This error is caused by the race condition between dev_alloc_name() > > and unregistering net device. The dev_alloc_name() function checks > > dev_base list to find a suitable id. The net device is removed from > > the dev_base list when net device is unregistered. Those are done with > > rtnl lock held, so there is no problem. However, removing sysfs entry > > is delayed until netdev_run_todo() which is outside rtnl lock. Because > > of this, dev_alloc_name() can create a device name which is duplicated > > with the existing sysfs entry. > > > > The following patch fixes this by changing dev_alloc_name() function > > to check not only dev_base list but also todo list and snapshot list > > to find a suitable id. If some devices exists on the todo list or > > snapshot list, sysfs entries corresponding to them are not deleted > > yet. > > > > Thanks, > > Kenji Kaneshige > > How about the following (untested), instead. It hold a removes the sysfs > entries earlier (on unregister_netdevice), but holds a kobject reference. > Then when todo runs the actual last put free happens. > > --- > net/core/dev.c | 10 ++++++---- > net/core/net-sysfs.c | 8 +++++++- > 2 files changed, 13 insertions(+), 5 deletions(-) > > --- net-2.6.orig/net/core/dev.c 2007-05-11 09:10:10.000000000 -0700 > +++ net-2.6/net/core/dev.c 2007-05-11 09:21:01.000000000 -0700 > @@ -3245,7 +3245,6 @@ void netdev_run_todo(void) > continue; > } > > - netdev_unregister_sysfs(dev); > dev->reg_state = NETREG_UNREGISTERED; > > netdev_wait_allrefs(dev); > @@ -3256,11 +3255,11 @@ void netdev_run_todo(void) > BUG_TRAP(!dev->ip6_ptr); > BUG_TRAP(!dev->dn_ptr); > > - /* It must be the very last action, > - * after this 'dev' may point to freed up memory. > - */ > if (dev->destructor) > dev->destructor(dev); > + > + /* Free network device */ > + kobject_put(&dev->dev.kobj); > } > > out: > @@ -3411,6 +3410,9 @@ void unregister_netdevice(struct net_dev > /* Notifier chain MUST detach us from master device. */ > BUG_TRAP(!dev->master); > > + /* Remove entries from sysfs */ > + netdev_unregister_sysfs(dev); > + > /* Finish processing unregister after unlock */ > net_set_todo(dev); > > --- net-2.6.orig/net/core/net-sysfs.c 2007-05-11 09:10:14.000000000 -0700 > +++ net-2.6/net/core/net-sysfs.c 2007-05-11 09:14:45.000000000 -0700 > @@ -456,9 +456,15 @@ static struct class net_class = { > #endif > }; > > +/* Delete sysfs entries but hold kobject reference until after all > + * netdev references are gone. > + */ > void netdev_unregister_sysfs(struct net_device * net) > { > - device_del(&(net->dev)); > + struct device *dev = &(net->dev); > + > + kobject_get(&dev->kobj); > + device_del(dev); > } > > /* Create sysfs entries for network device. */ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/