bondig - arp_interval with low value => Oops.

PATCH In review Nicolas 2P 4 participants 6 messages May 15, 2008 – Jun 10, 2008

Subsystems (3)

Networking Network drivers Kbuild

lore

Share & tools

lore

Nicolas 2P

author

2008-05-15 20:34 UTC #0 lore raw

PATCH 1/1 bondig - arp_interval with low value => Oops.

When setting arp_interval parameter to a very low value, delta_in_ticks for next arp might become 0, causing an infinite loop.

See http://bugzilla.kernel.org/show_bug.cgi?id=10680

Same problem for miimon parameter already fixed, but fix might be enhanced, by using msecs_to_jiffies() function.

Signed-off-by Nicolas de Pesloüan <[email protected]>

usr/src/linux/drivers/net/bonding/bond_main.c 2008-05-14 14:55:53.000000000 +0200 renamed from usr/src/linux/drivers/net/bonding/bond_main_orig.c 2008-05-13 02:00:01.000000000 +0200 +3 -3

@@ -2391,7 +2391,7 @@
                read_lock(&bond->lock);
        }
 
-       delay = ((bond->params.miimon * HZ) / 1000) ? : 1;
+       delay = msecs_to_jiffies(bond->params.miimon);
        read_unlock(&bond->lock);
        queue_delayed_work(bond->wq, &bond->mii_work, delay);
 }

@@ -2704,7 +2704,7 @@
 
        read_lock(&bond->lock);
 
-       delta_in_ticks = (bond->params.arp_interval * HZ) / 1000;
+       delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);
 
        if (bond->kill_timers) {
                goto out;

@@ -2837,7 +2837,7 @@
 
        read_lock(&bond->lock);
 
-       delta_in_ticks = (bond->params.arp_interval * HZ) / 1000;
+       delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval * HZ);
 
        if (bond->kill_timers) {
                goto out;

Joe Eykholt

2008-05-17 20:20 UTC #1 lore raw

Quoted 31 lines · Nicolas 2P wrote:

When setting arp_interval parameter to a very low value, delta_in_ticks for next arp might become 0, causing an infinite loop.
See http://bugzilla.kernel.org/show_bug.cgi?id=10680
Same problem for miimon parameter already fixed, but fix might be enhanced, by using msecs_to_jiffies() function.
Signed-off-by Nicolas de Pesloüan <[email protected]>
--- /usr/src/linux/drivers/net/bonding/bond_main_orig.c 2008-05-13 02:00:01.000000000 +0200
+++ /usr/src/linux/drivers/net/bonding/bond_main.c 2008-05-14
14:55:53.000000000 +0200
usr/src/linux/drivers/net/bonding/bond_main.c 2008-05-14 14:55:53.000000000 +0200 renamed from usr/src/linux/drivers/net/bonding/bond_main_orig.c 2008-05-13 02:00:01.000000000 +0200 +2 -2
@@ -2391,7 +2391,7 @@
                read_lock(&bond->lock);
        }
 
-       delay = ((bond->params.miimon * HZ) / 1000) ? : 1;
+       delay = msecs_to_jiffies(bond->params.miimon);
        read_unlock(&bond->lock);
        queue_delayed_work(bond->wq, &bond->mii_work, delay);
 }
@@ -2704,7 +2704,7 @@
 
        read_lock(&bond->lock);
 
-       delta_in_ticks = (bond->params.arp_interval * HZ) / 1000;
+       delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);
 
        if (bond->kill_timers) {
                goto out;

usr/src/linux/drivers/net/bonding/bond_main.c 2008-05-14 14:55:53.000000000 +0200 line 2840

Quoted 5 lines

usr/src/linux/drivers/net/bonding/bond_main.c 2008-05-14 14:55:53.000000000 +0200 renamed from usr/src/linux/drivers/net/bonding/bond_main_orig.c 2008-05-13 02:00:01.000000000 +0200 +1 -1
@@ -2837,7 +2837,7 @@
 
        read_lock(&bond->lock);
 
-       delta_in_ticks = (bond->params.arp_interval * HZ) / 1000;
+       delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval * HZ);

This seems like it should be just:

	delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);

Assuming the interval is in milliseconds, based on the previous usage.
Since it's multiplying by HZ (ticks per second) and dividing by 1000 (milliseconds per second), it was converting from milliseconds to ticks.

Same for the other changes?

Joe

Jay Vosburgh

2008-05-18 03:39 UTC #2 lore raw

Quoted 1 line · Joe Eykholt

Nicolas 2P wrote:

[...]

Quoted 10 lines

usr/src/linux/drivers/net/bonding/bond_main.c 2008-05-14 14:55:53.000000000 +0200 line 2840
Quoted 6 lines
usr/src/linux/drivers/net/bonding/bond_main.c 2008-05-14 14:55:53.000000000 +0200 renamed from usr/src/linux/drivers/net/bonding/bond_main_orig.c 2008-05-13 02:00:01.000000000 +0200 +1 -1
@@ -2837,7 +2837,7 @@
 
        read_lock(&bond->lock);
 
-       delta_in_ticks = (bond->params.arp_interval * HZ) / 1000;
+       delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval * HZ);
This seems like it should be just:
	delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);

Yes. I've corrected this already in my copy (which I'll be
posting momentarily).

Quoted 1 line

Same for the other changes?

The other two instances were correct, only the last change had
the extra "* HZ".

-J

---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]

Jeff Garzik

2008-06-10 22:19 UTC #3 lore raw

In reply to Nicolas 2P

Quoted 7 lines · Nicolas 2P wrote:

When setting arp_interval parameter to a very low value, delta_in_ticks for next arp might become 0, causing an infinite loop.
See http://bugzilla.kernel.org/show_bug.cgi?id=10680
Same problem for miimon parameter already fixed, but fix might be enhanced, by using msecs_to_jiffies() function.
Signed-off-by Nicolas de Pesloüan <[email protected]>

I don't see an ACK from Jay in my mbox?

Certainly looks nicer than adding "? : 1" to each of the remaining two cases.

Jay Vosburgh

2008-06-10 22:30 UTC #4 lore raw

Quoted 9 lines · Jeff Garzik

Quoted 8 lines · Nicolas 2P wrote:
When setting arp_interval parameter to a very low value, delta_in_ticks for next arp might become 0, causing an infinite loop.
See http://bugzilla.kernel.org/show_bug.cgi?id=10680
Same problem for miimon parameter already fixed, but fix might be enhanced, by using msecs_to_jiffies() function.

Quoted 6 lines

Quoted 1 line
Signed-off-by Nicolas de Pesloüan <[email protected]>
I don't see an ACK from Jay in my mbox?
Certainly looks nicer than adding "? : 1" to each of the remaining two cases.

This was accepted a couple of weeks ago; I had to fix a bug in
the patch, so I'd guess you didn't connect them:

commit 5ce0da8f0386b62345312ec8fed31303732f4220

Author: Jay Vosburgh <[email protected]>
Date:   Sat May 17 21:10:07 2008 -0700

bonding: Use msecs_to_jiffies, eliminate panic

Convert bonding to use msecs_to_jiffies instead of doing the
math. For the ARP monitor, there was an underflow problem that could result in an infinite loop. The miimon already had that worked around, but this is cleaner.

Originally by Nicolas de Pesloüan <[email protected]>
Jay Vosburgh corrected a math error in the original; Nicolas' original commit message is:

When setting arp_interval parameter to a very low value, delta_in_ticks for next arp might become 0, causing an infinite loop.

See http://bugzilla.kernel.org/show_bug.cgi?id=10680

Same problem for miimon parameter already fixed, but fix might be enhanced, by using msecs_to_jiffies() function.

Signed-off-by Nicolas de Pesloüan <[email protected]>

Signed-off-by Jay Vosburgh <[email protected]>

Signed-off-by Jeff Garzik <[email protected]>

-J

---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]

Jeff Garzik

2008-06-10 22:39 UTC #5 lore raw

Quoted 17 lines · Jay Vosburgh

Quoted 6 lines · Jeff Garzik
Quoted 5 lines · Nicolas 2P wrote:
When setting arp_interval parameter to a very low value, delta_in_ticks for next arp might become 0, causing an infinite loop.
See http://bugzilla.kernel.org/show_bug.cgi?id=10680
Quoted 8 lines
Quoted 4 lines
Same problem for miimon parameter already fixed, but fix might be enhanced, by using msecs_to_jiffies() function.
Signed-off-by Nicolas de Pesloüan <[email protected]>
I don't see an ACK from Jay in my mbox?
Certainly looks nicer than adding "? : 1" to each of the remaining two cases.

Quoted 8 lines

This was accepted a couple of weeks ago; I had to fix a bug in
the patch, so I'd guess you didn't connect them:
commit 5ce0da8f0386b62345312ec8fed31303732f4220
Author: Jay Vosburgh <[email protected]>
Date:   Sat May 17 21:10:07 2008 -0700
bonding: Use msecs_to_jiffies, eliminate panic

Great, thanks!