[PATCH]:Ethernet Bonding Driver-2.4.17

[PATCH]:Ethernet Bonding Driver-2.4.17

[Rajasekhar Inguva]

[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]

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;
 
 	dev_bond = dev_bonds = kmalloc(max_bonds*sizeof(struct net_device), 
 					GFP_KERNEL);

[-- Attachment #2: bonding.patch --]
[-- Type: text/plain, Size: 780 bytes --]

--- /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;
 
 	dev_bond = dev_bonds = kmalloc(max_bonds*sizeof(struct net_device), 
 					GFP_KERNEL);

Re: [PATCH]:Ethernet Bonding Driver-2.4.17

[Pozsar Balazs]

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

Re: [PATCH]:Ethernet Bonding Driver-2.4.17

[Ken Brownfield]

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.
brownfld@irridia.com

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

Re: [PATCH]:Ethernet Bonding Driver-2.4.17

[willy tarreau]

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