:Ethernet Bonding Driver-2.4.17

Hi all,

The patch is WRT a problem with the Ethernet Bonding Driver when compiled as a module. Tested on 2.4.17
and the patch is also against 2.4.17.

# insmod bonding max_bonds=2
Using /lib/modules/2.4.17/kernel/drivers/net/bonding.o
Warning: /lib/modules/2.4.17/kernel/drivers/net/bonding.o parameter
max_bonds
has max < min!
/lib/modules/2.4.17/kernel/drivers/net/bonding.o: unknown parameter type
'(' for
max_bonds

and the module fails to load.

The problem seems to be with the way MODULE_PARM was written for max_bonds.

MODULE_PARM(max_bonds,"1-" __MODULE_STRING(INT_MAX) "i");

INT_MAX is defined to be ((int)(~0U>>1)) and 'insmod' gets the string "1-((int)(~0U>>1))i" which it is failing to understand.

As max_bonds is an integer and not an array, i feel omitting the min-max range would be a better option.

And if a negative or zero value is supplied to max_bonds while loading, it can be taken care of in bonding_init() by setting it back to MAX_BONDS.

Thx,
Rajasekhar Inguva

@@ -226,7 +226,7 @@
 static struct bonding *these_bonds =  NULL;
 static struct net_device *dev_bonds = NULL;
 
-MODULE_PARM(max_bonds, "1-" __MODULE_STRING(INT_MAX) "i");
+MODULE_PARM(max_bonds,"i");
 MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");
 MODULE_PARM(miimon, "i");
 MODULE_PARM_DESC(miimon, "Link check interval in milliseconds");

@@ -1981,6 +1981,10 @@
 
 	/* Find a name for this unit */
 	static struct net_device *dev_bond = NULL;
+
+	/* If max_bonds <=0, set it to MAX_BONDS */
+	if(max_bonds <=0)
+		max_bonds = MAX_BONDS;
 
 	dev_bond = dev_bonds = kmalloc(max_bonds*sizeof(struct net_device), 
 					GFP_KERNEL);

@@ -226,7 +226,7 @@
 static struct bonding *these_bonds =  NULL;
 static struct net_device *dev_bonds = NULL;
 
-MODULE_PARM(max_bonds, "1-" __MODULE_STRING(INT_MAX) "i");
+MODULE_PARM(max_bonds,"i");
 MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");
 MODULE_PARM(miimon, "i");
 MODULE_PARM_DESC(miimon, "Link check interval in milliseconds");

@@ -1981,6 +1981,10 @@
 
 	/* Find a name for this unit */
 	static struct net_device *dev_bond = NULL;
+
+	/* If max_bonds <=0, set it to MAX_BONDS */
+	if(max_bonds <=0)
+		max_bonds = MAX_BONDS;
 
 	dev_bond = dev_bonds = kmalloc(max_bonds*sizeof(struct net_device), 
 					GFP_KERNEL);

Thanks for the fix, Rajasekhar.

I once encountered the same problem which may not be unique to bonding though, and fixed it by simply replacing INT_MAX with 2147483647. Both this method and yours allows far more interfaces than anyone would need, but at least your fix is cleaner in that it guarantees that the parameters will really be checked and not only rely on insmod.

Chad, I don't remember if I sent you my fix, but
I'd prefer this new one. Could you apply it
and/or send it to davem ?

Regards,
Willy

___________________________________________________________
Do You Yahoo!? -- Une adresse @yahoo.fr gratuite et en français ! Yahoo! Mail : http://fr.mail.yahoo.com

What about something like this? MAX_BONDS appears to be the default max_bonds value, so max_bonds>MAX_BONDS is fine as long as max_bonds<=INT_MAX, AFAICT.

I'm thinking a bogus value should reset to the default rather than <=0 --> 1 or >INT_MAX --> INT_MAX. Compiled but not tested.

Ken.
[email protected]

--- linux/drivers/net/bonding.c.orig	Fri Feb 22 00:49:57 2002
+++ linux/drivers/net/bonding.c	Fri Feb 22 01:19:36 2002
@@ -234,7 +234,7 @@
 static struct bonding *these_bonds =  NULL;
 static struct net_device *dev_bonds = NULL;
 
-MODULE_PARM(max_bonds, "1-" __MODULE_STRING(INT_MAX) "i");
+MODULE_PARM(max_bonds, "i");
 MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");
 MODULE_PARM(miimon, "i");
 MODULE_PARM_DESC(miimon, "Link check interval in milliseconds");
@@ -2037,6 +2037,14 @@
 
 	/* Find a name for this unit */
 	static struct net_device *dev_bond = NULL;
+
+	if ( max_bonds < 1 || max_bonds > INT_MAX ) {
+		printk( KERN_WARNING
+			"binding_init(): max_bonds (%d) not in range %d-%d, "
+			"was reset to MAX_BONDS (%d)",
+			max_bonds, 1, INT_MAX, MAX_BONDS );
+		max_bonds = MAX_BONDS ;
+	}
 
 	dev_bond = dev_bonds = kmalloc(max_bonds*sizeof(struct net_device), 
 					GFP_KERNEL);

On Sat, Feb 16, 2002 at 01:28:03PM +0100, Pozsar Balazs wrote:

| 
| On Sat, 16 Feb 2002, Rajasekhar Inguva wrote:
| 
| > Hi all,
| >
| > The patch is WRT a problem with the Ethernet Bonding Driver when
| > compiled as a module. Tested on 2.4.17
| > and the patch is also against 2.4.17.
| >
| > # insmod bonding max_bonds=2
| > Using /lib/modules/2.4.17/kernel/drivers/net/bonding.o
| > Warning: /lib/modules/2.4.17/kernel/drivers/net/bonding.o parameter
| > max_bonds
| > has max < min!
| > /lib/modules/2.4.17/kernel/drivers/net/bonding.o: unknown parameter type
| > '(' for
| > max_bonds
| >
| > and the module fails to load.
| >
| > The problem seems to be with the way MODULE_PARM was written for
| > max_bonds.
| >
| > MODULE_PARM(max_bonds,"1-" __MODULE_STRING(INT_MAX) "i");
| >
| > INT_MAX is defined to be ((int)(~0U>>1)) and 'insmod' gets the string
| > "1-((int)(~0U>>1))i" which it is failing to understand.
| >
| > As max_bonds is an integer and not an array, i feel omitting the min-max
| > range would be a better option.
| >
| > And if a negative or zero value is supplied to max_bonds while loading,
| > it can be taken care of in bonding_init() by setting it back to
| > MAX_BONDS.
| >
| > Thx,
| > Rajasekhar Inguva
| >
| >
| > --- /usr/src/linux/drivers/net/bonding.c	Fri Dec 21 23:11:54 2001
| > +++ /usr/src/linux/drivers/fixed/bonding.c	Sat Feb 16 17:03:48 2002
| > @@ -226,7 +226,7 @@
| >  static struct bonding *these_bonds =  NULL;
| >  static struct net_device *dev_bonds = NULL;
| >
| > -MODULE_PARM(max_bonds, "1-" __MODULE_STRING(INT_MAX) "i");
| > +MODULE_PARM(max_bonds,"i");
| >  MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");
| >  MODULE_PARM(miimon, "i");
| >  MODULE_PARM_DESC(miimon, "Link check interval in milliseconds");
| > @@ -1981,6 +1981,10 @@
| >
| >  	/* Find a name for this unit */
| >  	static struct net_device *dev_bond = NULL;
| > +
| > +	/* If max_bonds <=0, set it to MAX_BONDS */
| > +	if(max_bonds <=0)
| > +		max_bonds = MAX_BONDS;
| 
| 
| Imho you would need a check for the upper limit too. like:
| 	if(max_bonds > MAX_BONDS)
| 		max_bonds = MAX_BONDS;
| 
| 
| >
| >  	dev_bond = dev_bonds = kmalloc(max_bonds*sizeof(struct net_device),
| >  					GFP_KERNEL);
| 
| -- 
| pozsy