* [PATCH] bond_main.c: fix device deregistration in init exception path
@ 2005-09-18 6:15 Florin Malita
2005-09-18 6:32 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Florin Malita @ 2005-09-18 6:15 UTC (permalink / raw)
To: linux-kernel; +Cc: akpm, ctindel, fubar
bond_init() is not releasing rtnl_sem after register_netdevice() and
before calling unregister_netdevice() (from bond_free_all()) in the
exception path. As the device registration is not completed
(dev->reg_state == NETREG_REGISTERING), the call to
unregister_netdevice() triggers BUG_ON(dev->reg_state != NETREG_REGISTERED).
Signed-off-by: Florin Malita <fmalita@gmail.com>
----
diff --git a/drivers/net/bonding/bond_main.c
b/drivers/net/bonding/bond_main.c
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5039,6 +5039,10 @@ static int __init bonding_init(void)
return 0;
out_err:
+ /* give register_netdevice() a chance to complete */
+ rtnl_unlock();
+ rtnl_lock();
+
/* free and unregister all bonds that were successfully added */
bond_free_all();
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] bond_main.c: fix device deregistration in init exception path
2005-09-18 6:15 [PATCH] bond_main.c: fix device deregistration in init exception path Florin Malita
@ 2005-09-18 6:32 ` Andrew Morton
2005-09-18 7:25 ` David S. Miller
2005-09-22 2:38 ` Jeff Garzik
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Morton @ 2005-09-18 6:32 UTC (permalink / raw)
To: Florin Malita; +Cc: linux-kernel, ctindel, fubar, David S. Miller, netdev
Florin Malita <fmalita@gmail.com> wrote:
>
> User-Agent: Mozilla Thunderbird 1.0.6-1.1.fc4 (X11/20050720)
Your MUA is converting tabs to spaces.
> bond_init() is not releasing rtnl_sem after register_netdevice() and
> before calling unregister_netdevice() (from bond_free_all()) in the
> exception path. As the device registration is not completed
> (dev->reg_state == NETREG_REGISTERING), the call to
> unregister_netdevice() triggers BUG_ON(dev->reg_state != NETREG_REGISTERED).
>
> Signed-off-by: Florin Malita <fmalita@gmail.com>
> ----
> diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5039,6 +5039,10 @@ static int __init bonding_init(void)
> return 0;
>
> out_err:
> + /* give register_netdevice() a chance to complete */
> + rtnl_unlock();
> + rtnl_lock();
> +
> /* free and unregister all bonds that were successfully added */
> bond_free_all();
OK, so the bonding devices which were registered are now in state
NETREG_REGISTERING and we need to run netdev_run_todo() to flip them into
state NETREG_REGISTERED. If we don't do that,
bond_free_all()->unregister_netdevice() will go BUG.
The fix is solid enough, although a better comment is needed. Let's let
Dave decide whether that was a sane thing to go BUG over..
From: Florin Malita <fmalita@gmail.com>
bond_init() is not releasing rtnl_sem after register_netdevice() and before
calling unregister_netdevice() (from bond_free_all()) in the exception
path. As the device registration is not completed (dev->reg_state ==
NETREG_REGISTERING), the call to unregister_netdevice() triggers
BUG_ON(dev->reg_state != NETREG_REGISTERED).
Signed-off-by: Florin Malita <fmalita@gmail.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---
drivers/net/bonding/bond_main.c | 8 ++++++++
1 files changed, 8 insertions(+)
diff -puN drivers/net/bonding/bond_main.c~bond_mainc-fix-device-deregistration-in-init-exception drivers/net/bonding/bond_main.c
--- devel/drivers/net/bonding/bond_main.c~bond_mainc-fix-device-deregistration-in-init-exception 2005-09-17 23:18:38.000000000 -0700
+++ devel-akpm/drivers/net/bonding/bond_main.c 2005-09-17 23:31:02.000000000 -0700
@@ -5039,6 +5039,14 @@ static int __init bonding_init(void)
return 0;
out_err:
+ /*
+ * rtnl_unlock() will run netdev_run_todo(), putting the
+ * thus-far-registered bonding devices into a state which
+ * unregigister_netdevice() will accept
+ */
+ rtnl_unlock();
+ rtnl_lock();
+
/* free and unregister all bonds that were successfully added */
bond_free_all();
_
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bond_main.c: fix device deregistration in init exception path
2005-09-18 6:32 ` Andrew Morton
@ 2005-09-18 7:25 ` David S. Miller
2005-09-22 2:38 ` Jeff Garzik
1 sibling, 0 replies; 6+ messages in thread
From: David S. Miller @ 2005-09-18 7:25 UTC (permalink / raw)
To: akpm; +Cc: fmalita, linux-kernel, ctindel, fubar, netdev
From: Andrew Morton <akpm@osdl.org>
Date: Sat, 17 Sep 2005 23:32:24 -0700
> The fix is solid enough, although a better comment is needed. Let's
> let Dave decide whether that was a sane thing to go BUG over..
The fix is fine and so is the BUG() check.
Usually, you're supposed to make sure that the very last thing
you do is register_netdev(), and that all possible errors are
possible only before that call.
And that's what BOND basically does, except that it must register
multiple devices in a loop and therefore cannot follow that rule
precisely.
So I've added the patch to my tree, it's fine to do this for a
special case usage like this one.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bond_main.c: fix device deregistration in init exception path
2005-09-18 6:32 ` Andrew Morton
2005-09-18 7:25 ` David S. Miller
@ 2005-09-22 2:38 ` Jeff Garzik
2005-09-22 2:42 ` Andrew Morton
2005-09-22 2:43 ` Al Viro
1 sibling, 2 replies; 6+ messages in thread
From: Jeff Garzik @ 2005-09-22 2:38 UTC (permalink / raw)
To: Andrew Morton, Florin Malita
Cc: linux-kernel, ctindel, fubar, David S. Miller, netdev
Andrew Morton wrote:
> diff -puN drivers/net/bonding/bond_main.c~bond_mainc-fix-device-deregistration-in-init-exception drivers/net/bonding/bond_main.c
> --- devel/drivers/net/bonding/bond_main.c~bond_mainc-fix-device-deregistration-in-init-exception 2005-09-17 23:18:38.000000000 -0700
> +++ devel-akpm/drivers/net/bonding/bond_main.c 2005-09-17 23:31:02.000000000 -0700
> @@ -5039,6 +5039,14 @@ static int __init bonding_init(void)
> return 0;
>
> out_err:
> + /*
> + * rtnl_unlock() will run netdev_run_todo(), putting the
> + * thus-far-registered bonding devices into a state which
> + * unregigister_netdevice() will accept
> + */
> + rtnl_unlock();
> + rtnl_lock();
> +
Don't we want a schedule() or schedule_timeout(1) in between?
Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bond_main.c: fix device deregistration in init exception path
2005-09-22 2:38 ` Jeff Garzik
@ 2005-09-22 2:42 ` Andrew Morton
2005-09-22 2:43 ` Al Viro
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2005-09-22 2:42 UTC (permalink / raw)
To: Jeff Garzik; +Cc: fmalita, linux-kernel, ctindel, fubar, davem, netdev
Jeff Garzik <jgarzik@pobox.com> wrote:
>
> Andrew Morton wrote:
> > diff -puN drivers/net/bonding/bond_main.c~bond_mainc-fix-device-deregistration-in-init-exception drivers/net/bonding/bond_main.c
> > --- devel/drivers/net/bonding/bond_main.c~bond_mainc-fix-device-deregistration-in-init-exception 2005-09-17 23:18:38.000000000 -0700
> > +++ devel-akpm/drivers/net/bonding/bond_main.c 2005-09-17 23:31:02.000000000 -0700
> > @@ -5039,6 +5039,14 @@ static int __init bonding_init(void)
> > return 0;
> >
> > out_err:
> > + /*
> > + * rtnl_unlock() will run netdev_run_todo(), putting the
> > + * thus-far-registered bonding devices into a state which
> > + * unregigister_netdevice() will accept
> > + */
> > + rtnl_unlock();
> > + rtnl_lock();
> > +
>
>
> Don't we want a schedule() or schedule_timeout(1) in between?
>
No, it's all synchronous. See the nice comment ;)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bond_main.c: fix device deregistration in init exception path
2005-09-22 2:38 ` Jeff Garzik
2005-09-22 2:42 ` Andrew Morton
@ 2005-09-22 2:43 ` Al Viro
1 sibling, 0 replies; 6+ messages in thread
From: Al Viro @ 2005-09-22 2:43 UTC (permalink / raw)
To: Jeff Garzik
Cc: Andrew Morton, Florin Malita, linux-kernel, ctindel, fubar,
David S. Miller, netdev
On Wed, Sep 21, 2005 at 10:38:26PM -0400, Jeff Garzik wrote:
> >+ rtnl_unlock();
> >+ rtnl_lock();
> >+
>
>
> Don't we want a schedule() or schedule_timeout(1) in between?
No. rtnl_unlock() does the needed calls directly.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2005-09-22 2:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-09-18 6:15 [PATCH] bond_main.c: fix device deregistration in init exception path Florin Malita
2005-09-18 6:32 ` Andrew Morton
2005-09-18 7:25 ` David S. Miller
2005-09-22 2:38 ` Jeff Garzik
2005-09-22 2:42 ` Andrew Morton
2005-09-22 2:43 ` Al Viro
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox