BUG: bonding module can only be loaded once

BUG: bonding module can only be loaded once

[Stephen Hemminger]

In order to create multiple bonding dynamically, it is common practice to
load the bonding module multiple times.  This got broken in recent kernels
2.6.29 and later.

Doing the following will OOPS:
  modprobe -o bond0 bonding
  modprobe -o bond1 bonding

2.6.29 actually OOPS on error handling, but that is fixed in 2.6.30.
But 2.6.30 still has the regression (caused by sysfs).

This regression was introduced by changes to sysfs and proc that
made duplicate insertion a problem.

[  134.012562] Ethernet Channel Bonding Driver: v3.5.0 (November 4, 2008)
[  134.012565] bonding: Warning: either miimon or arp_interval and arp_ip_target module parameters must be specified, otherwise bonding will not detect link failures! see bonding.txt for details.
[  134.012571] ------------[ cut here ]------------
[  134.012578] WARNING: at fs/proc/generic.c:590 proc_register+0x154/0x191()
[  134.012581] Hardware name: System Product Name
[  134.012583] proc_dir_entry 'net/bonding' already registered
[  134.012585] Modules linked in: bond1(+) bond0 binfmt_misc ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack ipt_REJECT xt_tcpudp iptable_filter ip_tables x_tables bridge stp llc kvm_intel kvm ipv6 video output pci_slot acpi_cpufreq cpufreq_powersave cpufreq_ondemand cpufreq_conservative cpufreq_userspace snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm_oss snd_mixer_oss snd_pcm snd_seq_dummy snd_seq_oss snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq snd_timer snd_seq_device psmouse snd pcspkr serio_raw soundcore snd_page_alloc wmi evdev rtc_cmos sg sr_mod cdrom hid_belkin usbhid mvsas pata_acpi pata_marvell libsas ata_piix scsi_transport_sas ahci sky2 igb dca floppy fuse
[  134.012639] Pid: 7370, comm: modprobe Not tainted 2.6.30-rc8 #134
[  134.012641] Call Trace:
[  134.012646]  [<ffffffff802edf10>] ? proc_register+0x154/0x191
[  134.012650]  [<ffffffff802edf10>] ? proc_register+0x154/0x191
[  134.012655]  [<ffffffff802326c8>] ? warn_slowpath_common+0x77/0xa3
[  134.012659]  [<ffffffff80232750>] ? warn_slowpath_fmt+0x51/0x59
[  134.012663]  [<ffffffff802edf10>] ? proc_register+0x154/0x191
[  134.012668]  [<ffffffff802ee111>] ? proc_mkdir_mode+0x3c/0x51
[  134.012677]  [<ffffffffa0078787>] ? bonding_init+0x787/0x825 [bond1]
[  134.012685]  [<ffffffffa0078000>] ? bonding_init+0x0/0x825 [bond1]
[  134.012690]  [<ffffffff80209052>] ? do_one_initcall+0x51/0x120
[  134.012695]  [<ffffffff80383870>] ? __up_read+0x13/0x8d
[  134.012701]  [<ffffffff80248999>] ? __blocking_notifier_call_chain+0x51/0x5f
[  134.012705]  [<ffffffff80256d2c>] ? sys_init_module+0xa0/0x1bf
[  134.012709]  [<ffffffff8020aeeb>] ? system_call_fastpath+0x16/0x1b
[  134.012712] ---[ end trace cade86cfa6baa544 ]---
[  134.014510] ------------[ cut here ]------------
[  134.014516] WARNING: at fs/sysfs/dir.c:487 sysfs_add_one+0xcc/0xe4()
[  134.014519] Hardware name: System Product Name
[  134.014521] sysfs: cannot create duplicate filename '/class/net/bonding_masters'
[  134.014524] Modules linked in: bond1(+) bond0 binfmt_misc ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack ipt_REJECT xt_tcpudp iptable_filter ip_tables x_tables bridge stp llc kvm_intel kvm ipv6 video output pci_slot acpi_cpufreq cpufreq_powersave cpufreq_ondemand cpufreq_conservative cpufreq_userspace snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm_oss snd_mixer_oss snd_pcm snd_seq_dummy snd_seq_oss snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq snd_timer snd_seq_device psmouse snd pcspkr serio_raw soundcore snd_page_alloc wmi evdev rtc_cmos sg sr_mod cdrom hid_belkin usbhid mvsas pata_acpi pata_marvell libsas ata_piix scsi_transport_sas ahci sky2 igb dca floppy fuse
[  134.014603] Pid: 7370, comm: modprobe Tainted: G        W  2.6.30-rc8 #134
[  134.014605] Call Trace:
[  134.014610]  [<ffffffff802f4171>] ? sysfs_add_one+0xcc/0xe4
[  134.014614]  [<ffffffff802f4171>] ? sysfs_add_one+0xcc/0xe4
[  134.014618]  [<ffffffff802326c8>] ? warn_slowpath_common+0x77/0xa3
[  134.014623]  [<ffffffff80232750>] ? warn_slowpath_fmt+0x51/0x59
[  134.014628]  [<ffffffff802f409d>] ? sysfs_pathname+0x35/0x3d
[  134.014632]  [<ffffffff802f409d>] ? sysfs_pathname+0x35/0x3d
[  134.014636]  [<ffffffff802f409d>] ? sysfs_pathname+0x35/0x3d
[  134.014640]  [<ffffffff802f4171>] ? sysfs_add_one+0xcc/0xe4
[  134.014644]  [<ffffffff802f3b47>] ? sysfs_add_file_mode+0x4e/0x7f
[  134.014655]  [<ffffffffa02d2ef9>] ? bond_create_sysfs+0xd/0x45 [bond1]
[  134.014664]  [<ffffffffa00787e7>] ? bonding_init+0x7e7/0x825 [bond1]
[  134.014673]  [<ffffffffa0078000>] ? bonding_init+0x0/0x825 [bond1]
[  134.014677]  [<ffffffff80209052>] ? do_one_initcall+0x51/0x120
[  134.014682]  [<ffffffff80383870>] ? __up_read+0x13/0x8d
[  134.014687]  [<ffffffff80248999>] ? __blocking_notifier_call_chain+0x51/0x5f
[  134.014691]  [<ffffffff80256d2c>] ? sys_init_module+0xa0/0x1bf
[  134.014695]  [<ffffffff8020aeeb>] ? system_call_fastpath+0x16/0x1b
[  134.014698] ---[ end trace cade86cfa6baa545 ]---

Re: BUG: bonding module can only be loaded once

[Arnd Bergmann]

On Tuesday 09 June 2009, Stephen Hemminger wrote:
> In order to create multiple bonding dynamically, it is common practice to
> load the bonding module multiple times.  This got broken in recent kernels
> 2.6.29 and later.
> 
> Doing the following will OOPS:
>   modprobe -o bond0 bonding
>   modprobe -o bond1 bonding
> 
> 2.6.29 actually OOPS on error handling, but that is fixed in 2.6.30.
> But 2.6.30 still has the regression (caused by sysfs).
> 
> This regression was introduced by changes to sysfs and proc that
> made duplicate insertion a problem.

Well, I guess it's more like the changes just made it obvious that
it's wrong to do this. Registering the same entries in procfs or sysfs 
means that the user will only be able to talk to one of the two
bonding drivers through these interfaces. 

The log messages you quoted are not actually Oops but rather WARNING,
which is (in this case) not fatal at all, just an indication that the
root user did something he should not have:

> [  134.012578] WARNING: at fs/proc/generic.c:590 proc_register+0x154/0x191()
> [  134.012583] proc_dir_entry 'net/bonding' already registered

> [  134.014516] WARNING: at fs/sysfs/dir.c:487 sysfs_add_one+0xcc/0xe4()
> [  134.014521] sysfs: cannot create duplicate filename '/class/net/bonding_masters'

The bonding driver could work around this by checking if the directories
already exist before registering them. One can also add rtnl_link_ops to
the driver for dynamically adding more interfaces.

If you combine the two, you can even print a helpful message like 'please
use "ip link add type bonding" instead of "modprobe -o bond0 bonding"'.

In the mean time, you could probably work around this by ignoring the error
condition (see below), but I would suspect that there may be more problems
with the concept of just loading the module again. The best advice to
users is probably to configure the maximum number of bonding devices they
might need with the max_bonds= module parameter (if I understand that
parameter correctly.

	Arnd <><

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5203,7 +5203,7 @@ static int __init bonding_init(void)
 
 	res = bond_create_sysfs();
 	if (res)
-		goto err;
+		pr_info("Loading bonding module without sysfs interface\n");
 
 	register_netdevice_notifier(&bond_netdev_notifier);
 	register_inetaddr_notifier(&bond_inetaddr_notifier);

Re: BUG: bonding module can only be loaded once

[Stephen Hemminger]

On Tue, 9 Jun 2009 14:06:44 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Tuesday 09 June 2009, Stephen Hemminger wrote:
> > In order to create multiple bonding dynamically, it is common practice to
> > load the bonding module multiple times.  This got broken in recent kernels
> > 2.6.29 and later.
> > 
> > Doing the following will OOPS:
> >   modprobe -o bond0 bonding
> >   modprobe -o bond1 bonding
> > 
> > 2.6.29 actually OOPS on error handling, but that is fixed in 2.6.30.
> > But 2.6.30 still has the regression (caused by sysfs).
> > 
> > This regression was introduced by changes to sysfs and proc that
> > made duplicate insertion a problem.
> 
> Well, I guess it's more like the changes just made it obvious that
> it's wrong to do this. Registering the same entries in procfs or sysfs 
> means that the user will only be able to talk to one of the two
> bonding drivers through these interfaces. 
> 
> The log messages you quoted are not actually Oops but rather WARNING,
> which is (in this case) not fatal at all, just an indication that the
> root user did something he should not have:
> 
> > [  134.012578] WARNING: at fs/proc/generic.c:590 proc_register+0x154/0x191()
> > [  134.012583] proc_dir_entry 'net/bonding' already registered
> 
> > [  134.014516] WARNING: at fs/sysfs/dir.c:487 sysfs_add_one+0xcc/0xe4()
> > [  134.014521] sysfs: cannot create duplicate filename '/class/net/bonding_masters'
> 
> The bonding driver could work around this by checking if the directories
> already exist before registering them. One can also add rtnl_link_ops to
> the driver for dynamically adding more interfaces.
> 
> If you combine the two, you can even print a helpful message like 'please
> use "ip link add type bonding" instead of "modprobe -o bond0 bonding"'.
> 
> In the mean time, you could probably work around this by ignoring the error
> condition (see below), but I would suspect that there may be more problems
> with the concept of just loading the module again. The best advice to
> users is probably to configure the maximum number of bonding devices they
> might need with the max_bonds= module parameter (if I understand that
> parameter correctly.
> 
> 	Arnd <><
> 
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5203,7 +5203,7 @@ static int __init bonding_init(void)
>  
>  	res = bond_create_sysfs();
>  	if (res)
> -		goto err;
> +		pr_info("Loading bonding module without sysfs interface\n");
>  
>  	register_netdevice_notifier(&bond_netdev_notifier);
>  	register_inetaddr_notifier(&bond_inetaddr_notifier);

That only makes it limp along, and there still are warnings.
The point is that who ever added the WARN() in proc and sysfs, effectively
broke a bonding usage model.

-- 

Re: BUG: bonding module can only be loaded once

[Patrick McHardy]

Stephen Hemminger wrote:
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -5203,7 +5203,7 @@ static int __init bonding_init(void)
>>  
>>  	res = bond_create_sysfs();
>>  	if (res)
>> -		goto err;
>> +		pr_info("Loading bonding module without sysfs interface\n");
>>  
>>  	register_netdevice_notifier(&bond_netdev_notifier);
>>  	register_inetaddr_notifier(&bond_inetaddr_notifier);
> 
> That only makes it limp along, and there still are warnings.
> The point is that who ever added the WARN() in proc and sysfs, effectively
> broke a bonding usage model.

It did already dump the stack before that change, didn't it?

In any case, this is not the first time this has been broken and the
fundamental reason is in my opinion that the bonding interface is
broken to begin with. The module aliasing thing is complete crap
and should have been phased out long ago. At this point its probably
not worth anymore to migrate people to the sysfs interface though,
the best thing would be to add an rtnl_link interface and phase out
both.

Re: BUG: bonding module can only be loaded once

[Jay Vosburgh]

Patrick McHardy <kaber@trash.net> wrote:

>Stephen Hemminger wrote:
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -5203,7 +5203,7 @@ static int __init bonding_init(void)
>>>   	res = bond_create_sysfs();
>>>  	if (res)
>>> -		goto err;
>>> +		pr_info("Loading bonding module without sysfs interface\n");
>>>   	register_netdevice_notifier(&bond_netdev_notifier);
>>>  	register_inetaddr_notifier(&bond_inetaddr_notifier);
>>
>> That only makes it limp along, and there still are warnings.
>> The point is that who ever added the WARN() in proc and sysfs, effectively
>> broke a bonding usage model.
>
>It did already dump the stack before that change, didn't it?
>
>In any case, this is not the first time this has been broken and the
>fundamental reason is in my opinion that the bonding interface is
>broken to begin with. The module aliasing thing is complete crap
>and should have been phased out long ago. At this point its probably
>not worth anymore to migrate people to the sysfs interface though,
>the best thing would be to add an rtnl_link interface and phase out
>both.

	The "load bonding multiple times" stuff is only there now for
backwards compatibility with old distro initscripts / sysconfig packages
that don't configure bonding through sysfs (a sysfs API was added to
bonding three or four years ago).

	All of the current distro releases I'm aware of use sysfs to
configure bonding, and have done so for at least a year or two.  I
haven't done an exhaustive survey, but it seems unlikely that users are
running a current up to date kernel with a two or three year old
initscripts / sysconfig package.  Anybody have information to the
contrary?

	If nobody has any heartburn at dropping support for multiple
bonding instances on old distros, I'm as happy as anybody to remove all
of the multiple load logic from bonding.  There's been plenty of time
for transitioning from "multiple load" to sysfs.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: BUG: bonding module can only be loaded once

[Patrick McHardy]

Jay Vosburgh wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> 
>> In any case, this is not the first time this has been broken and the
>> fundamental reason is in my opinion that the bonding interface is
>> broken to begin with. The module aliasing thing is complete crap
>> and should have been phased out long ago. At this point its probably
>> not worth anymore to migrate people to the sysfs interface though,
>> the best thing would be to add an rtnl_link interface and phase out
>> both.
> 
> 	The "load bonding multiple times" stuff is only there now for
> backwards compatibility with old distro initscripts / sysconfig packages
> that don't configure bonding through sysfs (a sysfs API was added to
> bonding three or four years ago).
> 
> 	All of the current distro releases I'm aware of use sysfs to
> configure bonding, and have done so for at least a year or two.  I
> haven't done an exhaustive survey, but it seems unlikely that users are
> running a current up to date kernel with a two or three year old
> initscripts / sysconfig package.  Anybody have information to the
> contrary?

I'd expect its not the distros, but rather the applicances which might
still be using this. I know a vendor I used to work for a couple of
years ago just recently made the switch from 2.6.16 to a current kernel,
and I'd expect that they are still using this (I can find out tommorrow
if you want to know for sure). Vyatta likewise, I guess.

> 	If nobody has any heartburn at dropping support for multiple
> bonding instances on old distros, I'm as happy as anybody to remove all
> of the multiple load logic from bonding.  There's been plenty of time
> for transitioning from "multiple load" to sysfs.

In my opinion it would need a feature-removal-schedule announcement.

[PATCH] bonding: fix multiple module load problem

[Stephen Hemminger]

Some users still load bond module multiple times to create bonding
devices.  This accidentally was broken when sysfs interface was added.

Note: sysfs and procfs still produce WARN() messages when this is done
so the sysfs method is the recommended API.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Patch against 2.6.30-rc8

--- a/drivers/net/bonding/bond_sysfs.c	2009-06-09 10:07:29.618979856 -0700
+++ b/drivers/net/bonding/bond_sysfs.c	2009-06-09 10:18:04.039973238 -0700
@@ -1538,6 +1538,7 @@ int bond_create_sysfs(void)
 			printk(KERN_ERR
 			       "network device named %s already exists in sysfs",
 			       class_attr_bonding_masters.attr.name);
+		ret = 0;
 	}
 
 	return ret;

Re: [Bonding-devel] [PATCH] bonding: fix multiple module load problem

[Jay Vosburgh]

>Some users still load bond module multiple times to create bonding
>devices.  This accidentally was broken when sysfs interface was added.

	After looking at it, I think the multiple load stuff was broken
by the following:

commit b8a9787eddb0e4665f31dd1d64584732b2b5d051
Author: Jay Vosburgh <fubar@us.ibm.com>
Date:   Fri Jun 13 18:12:04 2008 -0700

    bonding: Allow setting max_bonds to zero
    
        Permit bonding to function rationally if max_bonds is set to
    zero.  This will load the module, but create no master devices (which can
    be created via sysfs).
    
        Requires some change to bond_create_sysfs; currently, the
    netdev sysfs directory is determined from the first bonding device created,
    but this is no longer possible.  Instead, an interface from net/core is
    created to create and destroy files in net_class.
    
        Based on a patch submitted by Phil Oester <kernel@linuxaces.com>.
    Modified by Jay Vosburgh to fix the sysfs issue mentioned above and to
    update the documentation.
    
    Signed-off-by: Phil Oester <kernel@linuxace.com>
    Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
    Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

	So, the patch below should probably go to -stable, too.

>Note: sysfs and procfs still produce WARN() messages when this is done
>so the sysfs method is the recommended API.
>
>Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>


>---
>Patch against 2.6.30-rc8
>
>--- a/drivers/net/bonding/bond_sysfs.c	2009-06-09 10:07:29.618979856 -0700
>+++ b/drivers/net/bonding/bond_sysfs.c	2009-06-09 10:18:04.039973238 -0700
>@@ -1538,6 +1538,7 @@ int bond_create_sysfs(void)
> 			printk(KERN_ERR
> 			       "network device named %s already exists in sysfs",
> 			       class_attr_bonding_masters.attr.name);
>+		ret = 0;
> 	}
>
> 	return ret;
>

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [Bonding-devel] BUG: bonding module can only be loaded once

[Jay Vosburgh]

Patrick McHardy <kaber@trash.net> wrote:

>Jay Vosburgh wrote:
>> Patrick McHardy <kaber@trash.net> wrote:
>> 
>>> In any case, this is not the first time this has been broken and the
>>> fundamental reason is in my opinion that the bonding interface is
>>> broken to begin with. The module aliasing thing is complete crap
>>> and should have been phased out long ago. At this point its probably
>>> not worth anymore to migrate people to the sysfs interface though,
>>> the best thing would be to add an rtnl_link interface and phase out
>>> both.
>> 
>> 	The "load bonding multiple times" stuff is only there now for
>> backwards compatibility with old distro initscripts / sysconfig packages
>> that don't configure bonding through sysfs (a sysfs API was added to
>> bonding three or four years ago).
>> 
>> 	All of the current distro releases I'm aware of use sysfs to
>> configure bonding, and have done so for at least a year or two.  I
>> haven't done an exhaustive survey, but it seems unlikely that users are
>> running a current up to date kernel with a two or three year old
>> initscripts / sysconfig package.  Anybody have information to the
>> contrary?
>
>I'd expect its not the distros, but rather the applicances which might
>still be using this. I know a vendor I used to work for a couple of
>years ago just recently made the switch from 2.6.16 to a current kernel,
>and I'd expect that they are still using this (I can find out tommorrow
>if you want to know for sure). Vyatta likewise, I guess.

	Yes, I'd like to know for sure; thanks.

>> 	If nobody has any heartburn at dropping support for multiple
>> bonding instances on old distros, I'm as happy as anybody to remove all
>> of the multiple load logic from bonding.  There's been plenty of time
>> for transitioning from "multiple load" to sysfs.
>
>In my opinion it would need a feature-removal-schedule announcement.

	Yah, probably.  The multiple load stuff was working fine as of,
oh, a year or two ago, so I wasn't worried so much about getting rid of
it.  If it's causing problems, though, it's time for it to go (or be
scheduled to go in the not too distant future).  Can't keep driving that
Ford Pinto forever.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [Bonding-devel] BUG: bonding module can only be loaded once

[Patrick McHardy]

Jay Vosburgh wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> 
>> I'd expect its not the distros, but rather the applicances which might
>> still be using this. I know a vendor I used to work for a couple of
>> years ago just recently made the switch from 2.6.16 to a current kernel,
>> and I'd expect that they are still using this (I can find out tommorrow
>> if you want to know for sure). Vyatta likewise, I guess.
> 
> 	Yes, I'd like to know for sure; thanks.

I'll let you know.

>>> 	If nobody has any heartburn at dropping support for multiple
>>> bonding instances on old distros, I'm as happy as anybody to remove all
>>> of the multiple load logic from bonding.  There's been plenty of time
>>> for transitioning from "multiple load" to sysfs.
>> In my opinion it would need a feature-removal-schedule announcement.
> 
> 	Yah, probably.  The multiple load stuff was working fine as of,
> oh, a year or two ago, so I wasn't worried so much about getting rid of
> it.  If it's causing problems, though, it's time for it to go (or be
> scheduled to go in the not too distant future).  Can't keep driving that
> Ford Pinto forever.

Well, if it has been broken for long enough, maybe we don't need
a feature-removal-schedule warning.

Re: [Bonding-devel] BUG: bonding module can only be loaded once

[Patrick McHardy]

Patrick McHardy wrote:
> Jay Vosburgh wrote:
>> Patrick McHardy <kaber@trash.net> wrote:
>>
>>> I'd expect its not the distros, but rather the applicances which might
>>> still be using this. I know a vendor I used to work for a couple of
>>> years ago just recently made the switch from 2.6.16 to a current kernel,
>>> and I'd expect that they are still using this (I can find out tommorrow
>>> if you want to know for sure). Vyatta likewise, I guess.
>>
>>     Yes, I'd like to know for sure; thanks.
> 
> I'll let you know.

They are still using the module alias method, but on a kernel slightly
older than the patch which broke this (2.6.25).