[PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[Jan Beulich]

Otherwise 'modprobe -r' on a module having a dependency on bridge will
implicitly unload bridge, bringing down all connectivity that was using
bridges.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Jeff Mahoney <jeffm@suse.com>

---
 net/bridge/br_if.c |    9 +++++++++
 1 file changed, 9 insertions(+)

--- 2.6.39-rc5/net/bridge/br_if.c
+++ 2.6.39-rc5-bridge-module-get-put/net/bridge/br_if.c
@@ -290,6 +290,11 @@ int br_add_bridge(struct net *net, const
 	if (!dev)
 		return -ENOMEM;
 
+	if (!try_module_get(THIS_MODULE)) {
+		free_netdev(dev);
+		return -ENOENT;
+	}
+
 	rtnl_lock();
 	if (strchr(dev->name, '%')) {
 		ret = dev_alloc_name(dev, dev->name);
@@ -308,6 +313,8 @@ int br_add_bridge(struct net *net, const
 		unregister_netdevice(dev);
  out:
 	rtnl_unlock();
+	if (ret)
+		module_put(THIS_MODULE);
 	return ret;
 
 out_free:
@@ -339,6 +346,8 @@ int br_del_bridge(struct net *net, const
 		del_br(netdev_priv(dev), NULL);
 
 	rtnl_unlock();
+	if (ret == 0)
+		module_put(THIS_MODULE);
 	return ret;
 }
 

Re: [PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[David Miller]

From: "Jan Beulich" <JBeulich@novell.com>
Date: Fri, 29 Apr 2011 08:21:14 +0100

> Otherwise 'modprobe -r' on a module having a dependency on bridge will
> implicitly unload bridge, bringing down all connectivity that was using
> bridges.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Jeff Mahoney <jeffm@suse.com>

All network device drivers behave exactly the same way, when you rmmod
the thing we unconfigure all the routes, addresses, etc. going through
that device and let you unload it.

And this behavior is very much intentional.

Don't add an exception here.

Re: [PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[Jan Beulich]

>>> On 29.04.11 at 09:25, David Miller <davem@davemloft.net> wrote:
> From: "Jan Beulich" <JBeulich@novell.com>
> Date: Fri, 29 Apr 2011 08:21:14 +0100
> 
>> Otherwise 'modprobe -r' on a module having a dependency on bridge will
>> implicitly unload bridge, bringing down all connectivity that was using
>> bridges.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>> Cc: Jeff Mahoney <jeffm@suse.com>
> 
> All network device drivers behave exactly the same way, when you rmmod
> the thing we unconfigure all the routes, addresses, etc. going through
> that device and let you unload it.
> 
> And this behavior is very much intentional.
> 
> Don't add an exception here.

You talk of rmmod on the very module, but the issue is about
modprobe -r on a dependent module. I cannot believe you consider
it correct that *implicit* unloading of bridge.ko should happen when
bridges are configured.

If the solution proposed isn't satisfactory, can you suggest a better
alternative still serving the purpose?

Jan

Re: [PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[David Miller]

From: "Jan Beulich" <JBeulich@novell.com>
Date: Fri, 29 Apr 2011 08:41:10 +0100

> You talk of rmmod on the very module, but the issue is about
> modprobe -r on a dependent module. I cannot believe you consider
> it correct that *implicit* unloading of bridge.ko should happen when
> bridges are configured.

Which module in particular depends upon bridge and causes the
problem?

Re: [PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[Jan Beulich]

>>> On 29.04.11 at 10:10, David Miller <davem@davemloft.net> wrote:
> From: "Jan Beulich" <JBeulich@novell.com>
> Date: Fri, 29 Apr 2011 08:41:10 +0100
> 
>> You talk of rmmod on the very module, but the issue is about
>> modprobe -r on a dependent module. I cannot believe you consider
>> it correct that *implicit* unloading of bridge.ko should happen when
>> bridges are configured.
> 
> Which module in particular depends upon bridge and causes the
> problem?

The problem was observed (a long time ago) with ebtable_broute,
and I cannot see how this would have changed meanwhile.

Jan

Re: [PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[David Miller]

From: "Jan Beulich" <JBeulich@novell.com>
Date: Fri, 29 Apr 2011 09:31:27 +0100

>>>> On 29.04.11 at 10:10, David Miller <davem@davemloft.net> wrote:
>> From: "Jan Beulich" <JBeulich@novell.com>
>> Date: Fri, 29 Apr 2011 08:41:10 +0100
>> 
>>> You talk of rmmod on the very module, but the issue is about
>>> modprobe -r on a dependent module. I cannot believe you consider
>>> it correct that *implicit* unloading of bridge.ko should happen when
>>> bridges are configured.
>> 
>> Which module in particular depends upon bridge and causes the
>> problem?
> 
> The problem was observed (a long time ago) with ebtable_broute,
> and I cannot see how this would have changed meanwhile.

Well your change makes it so that someone who actually _wants_ to
unload the bridge module, regardless of configuration, cannot do so.

I think that's a worse problem than this ebtables thing.

Nothing on the system should be hitting modules with unload requests
unless the user explicitly asked for that specific module to be
unloaded.  At least not by default.

So the me the problem is perhaps that "modprobe -r" does this auto
dependency unloading thing by default.

When we first fixed network device drivers so that they now properly
always run with no module refcount at all, people complained because
there were some distributions that ran some daemon that periodically
looked for "unreferenced" modules and "helped" the user by
automatically unloaded them.

We killed that foolish daemon, and we can fix "modprobe -r" too.

Does "rmmod" have this behavior too?  If not, and it does the right
thing by only unloaded what the user asked for, then people should
use that.

I really don't in any way want to block people from being able to
cleanly unload the bridge module, regardless of configuration, if
that's what they want so your patch as written is not going to be
considered for inclusion.

Re: [PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[Jan Beulich]

>>> On 29.04.11 at 10:44, David Miller <davem@davemloft.net> wrote:
> From: "Jan Beulich" <JBeulich@novell.com>
> Date: Fri, 29 Apr 2011 09:31:27 +0100
> 
>>>>> On 29.04.11 at 10:10, David Miller <davem@davemloft.net> wrote:
>>> From: "Jan Beulich" <JBeulich@novell.com>
>>> Date: Fri, 29 Apr 2011 08:41:10 +0100
>>> 
>>>> You talk of rmmod on the very module, but the issue is about
>>>> modprobe -r on a dependent module. I cannot believe you consider
>>>> it correct that *implicit* unloading of bridge.ko should happen when
>>>> bridges are configured.
>>> 
>>> Which module in particular depends upon bridge and causes the
>>> problem?
>> 
>> The problem was observed (a long time ago) with ebtable_broute,
>> and I cannot see how this would have changed meanwhile.
> 
> Well your change makes it so that someone who actually _wants_ to
> unload the bridge module, regardless of configuration, cannot do so.
> 
> I think that's a worse problem than this ebtables thing.
> 
> Nothing on the system should be hitting modules with unload requests
> unless the user explicitly asked for that specific module to be
> unloaded.  At least not by default.
> 
> So the me the problem is perhaps that "modprobe -r" does this auto
> dependency unloading thing by default.
> 
> When we first fixed network device drivers so that they now properly
> always run with no module refcount at all, people complained because
> there were some distributions that ran some daemon that periodically
> looked for "unreferenced" modules and "helped" the user by
> automatically unloaded them.
> 
> We killed that foolish daemon, and we can fix "modprobe -r" too.

Michal - aren't you the modutils maintainer? What are your thoughts
here? (The original report we got is
https://bugzilla.novell.com/show_bug.cgi?id=267651.)

> Does "rmmod" have this behavior too?  If not, and it does the right
> thing by only unloaded what the user asked for, then people should
> use that.

No, it doesn't. Other than modprobe, rmmod deals only with the
module specified.

> I really don't in any way want to block people from being able to
> cleanly unload the bridge module, regardless of configuration, if
> that's what they want so your patch as written is not going to be
> considered for inclusion.

I understood that meanwhile, yet fail to see an alternative solution
(imo this auto-unloading is quite desirable in other cases).

Jan

Re: [PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[Michal Marek]

On 29.4.2011 11:09, Jan Beulich wrote:
>>>> On 29.04.11 at 10:44, David Miller<davem@davemloft.net>  wrote:
>> From: "Jan Beulich"<JBeulich@novell.com>
>> Date: Fri, 29 Apr 2011 09:31:27 +0100
>>
>>>>>> On 29.04.11 at 10:10, David Miller<davem@davemloft.net>  wrote:
>>>> From: "Jan Beulich"<JBeulich@novell.com>
>>>> Date: Fri, 29 Apr 2011 08:41:10 +0100
>>>>
>>>>> You talk of rmmod on the very module, but the issue is about
>>>>> modprobe -r on a dependent module. I cannot believe you consider
>>>>> it correct that *implicit* unloading of bridge.ko should happen when
>>>>> bridges are configured.
>>>>
>>>> Which module in particular depends upon bridge and causes the
>>>> problem?
>>>
>>> The problem was observed (a long time ago) with ebtable_broute,
>>> and I cannot see how this would have changed meanwhile.
>>
>> Well your change makes it so that someone who actually _wants_ to
>> unload the bridge module, regardless of configuration, cannot do so.
>>
>> I think that's a worse problem than this ebtables thing.
>>
>> Nothing on the system should be hitting modules with unload requests
>> unless the user explicitly asked for that specific module to be
>> unloaded.  At least not by default.
>>
>> So the me the problem is perhaps that "modprobe -r" does this auto
>> dependency unloading thing by default.
>>
>> When we first fixed network device drivers so that they now properly
>> always run with no module refcount at all, people complained because
>> there were some distributions that ran some daemon that periodically
>> looked for "unreferenced" modules and "helped" the user by
>> automatically unloaded them.
>>
>> We killed that foolish daemon, and we can fix "modprobe -r" too.
>
> Michal - aren't you the modutils maintainer?

That would be Jon (CC added).

> What are your thoughts
> here? (The original report we got is
> https://bugzilla.novell.com/show_bug.cgi?id=267651.)

I think that defaulting to not removing dependencies would be a good 
idea. But do not expect that it will help with those artificial tests, 
they will just proceed a few steps further until they hit the module 
with broken unloading ;-).

Michal

>
>> Does "rmmod" have this behavior too?  If not, and it does the right
>> thing by only unloaded what the user asked for, then people should
>> use that.
>
> No, it doesn't. Other than modprobe, rmmod deals only with the
> module specified.
>
>> I really don't in any way want to block people from being able to
>> cleanly unload the bridge module, regardless of configuration, if
>> that's what they want so your patch as written is not going to be
>> considered for inclusion.
>
> I understood that meanwhile, yet fail to see an alternative solution
> (imo this auto-unloading is quite desirable in other cases).
>
> Jan
>

Re: [PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[Jon Masters]

On Fri, 2011-04-29 at 13:08 +0200, Michal Marek wrote:
> On 29.4.2011 11:09, Jan Beulich wrote:
> >>>> On 29.04.11 at 10:44, David Miller<davem@davemloft.net>  wrote:

> >> Nothing on the system should be hitting modules with unload requests
> >> unless the user explicitly asked for that specific module to be
> >> unloaded.  At least not by default.
> >>
> >> So the me the problem is perhaps that "modprobe -r" does this auto
> >> dependency unloading thing by default.
> >>
> >> When we first fixed network device drivers so that they now properly
> >> always run with no module refcount at all, people complained because
> >> there were some distributions that ran some daemon that periodically
> >> looked for "unreferenced" modules and "helped" the user by
> >> automatically unloaded them.
> >>
> >> We killed that foolish daemon, and we can fix "modprobe -r" too.
> >
> > Michal - aren't you the modutils maintainer?
> 
> That would be Jon (CC added).

Thanks. So the specific feature you mention was added precisely because
some folks wanted to clean up ununsed modules by removing all of their
dependencies. Since I've not been on this thread until now, can you let
me know what precisely you need, and why? We can make the unloading of
unused modules configurable, but it sounds like you're saying even that
isn't good enough. What actually happens, what's the bug experience?

I realize there isn't a general fondness of module removing, and I for
one don't really mind having a few extra modules loaded in my kernel.

Jon.

Re: [PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[Jan Beulich]

>>> On 29.04.11 at 18:05, Jon Masters <jonathan@jonmasters.org> wrote:
> On Fri, 2011-04-29 at 13:08 +0200, Michal Marek wrote:
>> On 29.4.2011 11:09, Jan Beulich wrote:
>> >>>> On 29.04.11 at 10:44, David Miller<davem@davemloft.net>  wrote:
> 
>> >> Nothing on the system should be hitting modules with unload requests
>> >> unless the user explicitly asked for that specific module to be
>> >> unloaded.  At least not by default.
>> >>
>> >> So the me the problem is perhaps that "modprobe -r" does this auto
>> >> dependency unloading thing by default.
>> >>
>> >> When we first fixed network device drivers so that they now properly
>> >> always run with no module refcount at all, people complained because
>> >> there were some distributions that ran some daemon that periodically
>> >> looked for "unreferenced" modules and "helped" the user by
>> >> automatically unloaded them.
>> >>
>> >> We killed that foolish daemon, and we can fix "modprobe -r" too.
>> >
>> > Michal - aren't you the modutils maintainer?
>> 
>> That would be Jon (CC added).
> 
> Thanks. So the specific feature you mention was added precisely because
> some folks wanted to clean up ununsed modules by removing all of their
> dependencies. Since I've not been on this thread until now, can you let
> me know what precisely you need, and why? We can make the unloading of
> unused modules configurable, but it sounds like you're saying even that
> isn't good enough. What actually happens, what's the bug experience?

The problem observed was that unloading (via modprobe -r)
ebtable_broute.ko, bridge.ko was also unloaded, causing all
bridged networking to stop functioning on a machine.

Jan

Re: [PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[Jon Masters]

On Fri, 2011-04-29 at 17:07 +0100, Jan Beulich wrote:
> >>> On 29.04.11 at 18:05, Jon Masters <jonathan@jonmasters.org> wrote:
> > On Fri, 2011-04-29 at 13:08 +0200, Michal Marek wrote:
> >> On 29.4.2011 11:09, Jan Beulich wrote:
> >> >>>> On 29.04.11 at 10:44, David Miller<davem@davemloft.net>  wrote:
> > 
> >> >> Nothing on the system should be hitting modules with unload requests
> >> >> unless the user explicitly asked for that specific module to be
> >> >> unloaded.  At least not by default.
> >> >>
> >> >> So the me the problem is perhaps that "modprobe -r" does this auto
> >> >> dependency unloading thing by default.
> >> >>
> >> >> When we first fixed network device drivers so that they now properly
> >> >> always run with no module refcount at all, people complained because
> >> >> there were some distributions that ran some daemon that periodically
> >> >> looked for "unreferenced" modules and "helped" the user by
> >> >> automatically unloaded them.
> >> >>
> >> >> We killed that foolish daemon, and we can fix "modprobe -r" too.
> >> >
> >> > Michal - aren't you the modutils maintainer?
> >> 
> >> That would be Jon (CC added).
> > 
> > Thanks. So the specific feature you mention was added precisely because
> > some folks wanted to clean up ununsed modules by removing all of their
> > dependencies. Since I've not been on this thread until now, can you let
> > me know what precisely you need, and why? We can make the unloading of
> > unused modules configurable, but it sounds like you're saying even that
> > isn't good enough. What actually happens, what's the bug experience?
> 
> The problem observed was that unloading (via modprobe -r)
> ebtable_broute.ko, bridge.ko was also unloaded, causing all
> bridged networking to stop functioning on a machine.

Ah, right...ouch. That would be a "little" problem. Short of having an
exclusion list and all that nonsense, probably best to start with either
removing the unload logic or making it globally configurable. Thanks.

Jon.

Re: [PATCH] bridge: Module use count must be updated as bridges are created/destroyed

[Stephen Hemminger]

On Fri, 29 Apr 2011 00:25:30 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: "Jan Beulich" <JBeulich@novell.com>
> Date: Fri, 29 Apr 2011 08:21:14 +0100
> 
> > Otherwise 'modprobe -r' on a module having a dependency on bridge will
> > implicitly unload bridge, bringing down all connectivity that was using
> > bridges.
> > 
> > Signed-off-by: Jan Beulich <jbeulich@novell.com>
> > Cc: Jeff Mahoney <jeffm@suse.com>
> 
> All network device drivers behave exactly the same way, when you rmmod
> the thing we unconfigure all the routes, addresses, etc. going through
> that device and let you unload it.
> 
> And this behavior is very much intentional.
> 
> Don't add an exception here.

Agreed.