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

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

[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

[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

[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.

[patch 00/22] 2.6.16-stable review cycle

[Greg KH]

This is the start of the stable review cycle for the 2.6.16.10 release.
There are 22 patches in this series, all will be posted as a response to
this one.  If anyone has any issues with these being applied, please let
us know.  If anyone is a maintainer of the proper subsystem, and wants
to add a signed-off-by: line to the patch, please respond with it.

These patches are sent out with a number of different people on the Cc:
line.  If you wish to be a reviewer, please email stable@kernel.org to
add your name to the list.  If you want to be off the reviewer list,
also email us.

Responses should be made by Sunday, April 23, 03:00:00 UTC.  Anything
received after that time, might be too late.

thanks,

the -stable release team

[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:

--