[patch 05/22] : Fix hotplug race during device registration

[patch 05/22] : Fix hotplug race during device registration

[Greg KH]

[-- Attachment #1: fix-hotplug-race-during-device-registration.patch --]
[-- Type: text/plain, Size: 1289 bytes --]

From: Thomas de Grenier de Latour <degrenier@easyconnect.fr>

On Sun, 9 Apr 2006 21:56:59 +0400,
Sergey Vlasov <vsu@altlinux.ru> wrote:
> However, show_address() does not output anything unless
> dev->reg_state == NETREG_REGISTERED - and this state is set by
> netdev_run_todo() only after netdev_register_sysfs() returns, so in
> the meantime (while netdev_register_sysfs() is busy adding the
> "statistics" attribute group) some process may see an empty "address"
> attribute.

I've tried the attached patch, suggested by Sergey Vlasov on
hotplug-devel@, and as far as i can test it works just fine.

Signed-off-by: Alexander Patrakov <patrakov@ums.usu.ru>
Signed-off-by: David Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 net/core/dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6.16.9.orig/net/core/dev.c
+++ linux-2.6.16.9/net/core/dev.c
@@ -2932,11 +2932,11 @@ void netdev_run_todo(void)
 
 		switch(dev->reg_state) {
 		case NETREG_REGISTERING:
+			dev->reg_state = NETREG_REGISTERED;
 			err = netdev_register_sysfs(dev);
 			if (err)
 				printk(KERN_ERR "%s: failed sysfs registration (%d)\n",
 				       dev->name, err);
-			dev->reg_state = NETREG_REGISTERED;
 			break;
 
 		case NETREG_UNREGISTERING:

--

Re: [patch 05/22] : Fix hotplug race during device registration

[Chris Rankin]

With reference to this patch:
http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blobdiff;h=2731570eba5b35a21c311dd587057c39805082f1;hp=dfb62998866ae2e298139164a85ec0757b7f3fc7;hb=9469d458b90bfb9117cbb488cfa645d94c3921b1;f=net/core/dev.c

Doesn't this patch introduce another bug when registration fails, because reg_state is left as
NETREG_REGISTERED?

Cheers,
Chris


Send instant messages to your online friends http://uk.messenger.yahoo.com 

Re: [patch 05/22] : Fix hotplug race during device registration

[Alexander E. Patrakov]

Chris Rankin wrote:
> With reference to this patch:
> http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blobdiff;h=2731570eba5b35a21c311dd587057c39805082f1;hp=dfb62998866ae2e298139164a85ec0757b7f3fc7;hb=9469d458b90bfb9117cbb488cfa645d94c3921b1;f=net/core/dev.c
> 
> Doesn't this patch introduce another bug when registration fails, because reg_state is left as
> NETREG_REGISTERED?

This could be fixed up by saving the old value and restoring it in the "if 
(err)" statement, but I guess this has to be fixed in the mainline before 
allowing the modified "if (err)" into -stable.

-- 
Alexander E. Patrakov

Re: [patch 05/22] : Fix hotplug race during device registration

[Alexander E. Patrakov]

Chris Rankin wrote:
> With reference to this patch:
> http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blobdiff;h=2731570eba5b35a21c311dd587057c39805082f1;hp=dfb62998866ae2e298139164a85ec0757b7f3fc7;hb=9469d458b90bfb9117cbb488cfa645d94c3921b1;f=net/core/dev.c
> 
> Doesn't this patch introduce another bug when registration fails, because reg_state is left as
> NETREG_REGISTERED?

Look at the old code again. This is not a new bug. The old code fails 
registration, does a printk, and then sets dev->reg_state = NETREG_REGISTERED. 
So this doesn't revoke my signed-off-by line.

-- 
Alexander E. Patrakov

Re: [patch 05/22] : Fix hotplug race during device registration

[Chris Rankin]

--- "Alexander E. Patrakov" <patrakov@ums.usu.ru> wrote:
> Look at the old code again. This is not a new bug. The old code fails 
> registration, does a printk, and then sets dev->reg_state = NETREG_REGISTERED. 

OK, fair enough. But anyway, is it valid to leave reg_state as NETREG_REGISTERED when the
registration has failed?

Cheers,
Chris



		
___________________________________________________________ 
Yahoo! Photos – NEW, now offering a quality print service from just 7p a photo http://uk.photos.yahoo.com

Re: [patch 05/22] : Fix hotplug race during device registration

[Chris Rankin]

--- "Alexander E. Patrakov" <patrakov@ums.usu.ru> wrote:
> This could be fixed up by saving the old value and restoring it in the "if 
> (err)" statement, but I guess this has to be fixed in the mainline before 
> allowing the modified "if (err)" into -stable.

I'm not going to claim to know how this state machine works, but would restoring the state to the
original value prompt the kernel to try and reregister the device in an endless loop? I was
wondering if maybe it should be set to some "Failed" state instead.

Cheers,
Chris


		
___________________________________________________________ 
To help you stay safe and secure online, we've developed the all new Yahoo! Security Centre. http://uk.security.yahoo.com

Re: [patch 05/22] : Fix hotplug race during device registration

[Stephen Hemminger]

On Fri, 21 Apr 2006 14:52:19 +0100 (BST)
Chris Rankin <rankincj@yahoo.com> wrote:

> --- "Alexander E. Patrakov" <patrakov@ums.usu.ru> wrote:
> > Look at the old code again. This is not a new bug. The old code fails 
> > registration, does a printk, and then sets dev->reg_state = NETREG_REGISTERED. 
> 
> OK, fair enough. But anyway, is it valid to leave reg_state as NETREG_REGISTERED when the
> registration has failed?

Yes. the device is still half alive in that case. It is accessible via normal networking
calls, and can be unregistered. It just would not show up properly in sysfs.

Not sure how it would be possible (except maybe out of memory) to construct a case
where registration fails. Maybe races with name changes.