[PATCH] core: linkwatch should use jiffies64

[PATCH] core: linkwatch should use jiffies64

[Stefan Rompf]

Hi,

the linkwatch code can overflow on a jiffies wrap, scheduling
work with a too large delay. If the delay is >0x80000000,
internal_add_timer() seems to overflow too, hiding the bug, so
this isn't triggered too easily.

Best solution is to use jiffies64 for calculation as these
events happen with any possible delay in between.

This should be 2.6.17 stuff.

Signed-off-by: Stefan Rompf <stefan@loplof.de>

--- linux-2.6.17-rc3/net/core/link_watch.c.orig	2006-04-27 20:37:09.000000000 +0200
+++ linux-2.6.17-rc3/net/core/link_watch.c	2006-04-27 21:49:00.000000000 +0200
@@ -32,8 +32,8 @@
 	LW_SE_USED
 };
 
+static u64 linkwatch_nextevent;
 static unsigned long linkwatch_flags;
-static unsigned long linkwatch_nextevent;
 
 static void linkwatch_event(void *dummy);
 static DECLARE_WORK(linkwatch_work, linkwatch_event, NULL);
@@ -136,7 +136,7 @@
 	 * cause a storm of messages on the netlink
 	 * socket
 	 */	
-	linkwatch_nextevent = jiffies + HZ;
+	linkwatch_nextevent = get_jiffies_64() + HZ;
 	clear_bit(LW_RUNNING, &linkwatch_flags);
 
 	rtnl_lock();
@@ -170,7 +170,7 @@
 		spin_unlock_irqrestore(&lweventlist_lock, flags);
 
 		if (!test_and_set_bit(LW_RUNNING, &linkwatch_flags)) {
-			unsigned long thisevent = jiffies;
+			u64 thisevent = get_jiffies_64();
 
 			if (thisevent >= linkwatch_nextevent) {
 				schedule_work(&linkwatch_work);

Re: [PATCH] core: linkwatch should use jiffies64

[David S. Miller]

From: Stefan Rompf <stefan@loplof.de>
Date: Sun, 7 May 2006 12:13:56 +0200

> the linkwatch code can overflow on a jiffies wrap, scheduling
> work with a too large delay. If the delay is >0x80000000,
> internal_add_timer() seems to overflow too, hiding the bug, so
> this isn't triggered too easily.

What is so special about what linkwatch is doing such
that it needs this kind of treatment and other similar
pieces or code do not?

We have all sorts of interfaces such as time_after() et el.
in order to deal with wrapping issues.  And furthermore
using 64-bit jiffies here might not be appropriate because
they are not guarenteed to be accessed atomically, nothing
here prevents the upper 32-bits from being updated while
we sample the lower 32-bits on 32-bit systems that will do
the 64-bit jiffied load using two 32-bit loads.

Re: [PATCH] core: linkwatch should use jiffies64

[Stefan Rompf]

Am Montag 08 Mai 2006 08:07 schrieb David S. Miller:

> What is so special about what linkwatch is doing such
> that it needs this kind of treatment and other similar
> pieces or code do not?
>
> We have all sorts of interfaces such as time_after() et el.
> in order to deal with wrapping issues.

time_after() and friends can handle jiffies wrapping, however they require the 
difference between compared times to be less than 0x80000000 jiffies (about 
24 days on HZ=1000) to work reliably on 32bit architectures. So if the 
network is stable for 24 days, events generated within days 25-49 will suffer 
a *huge* false delay.

> And furthermore 
> using 64-bit jiffies here might not be appropriate because
> they are not guarenteed to be accessed atomically,

get_jiffies_64() handles this transparently.

Stefan

[NET] linkwatch: Handle jiffies wrap-around

[Herbert Xu]

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

On Mon, May 08, 2006 at 06:28:56PM +0000, Stefan Rompf wrote:
> 
> time_after() and friends can handle jiffies wrapping, however they require the 
> difference between compared times to be less than 0x80000000 jiffies (about 
> 24 days on HZ=1000) to work reliably on 32bit architectures. So if the 
> network is stable for 24 days, events generated within days 25-49 will suffer 
> a *huge* false delay.

How about something like this?

[NET] linkwatch: Handle jiffies wrap-around

The test used in the linkwatch does not handle wrap-arounds correctly.
Since the intention of the code is to eliminate bursts of messages we
can afford to delay things up to a second.  Using that fact we can
easily handle wrap-arounds by making sure that we don't delay things
by more than one second.

This is based on diagnosis and a patch by Stefan Rompf.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: linkwatch-wrap.patch --]
[-- Type: text/plain, Size: 717 bytes --]

diff --git a/net/core/link_watch.c b/net/core/link_watch.c
index 341de44..646937c 100644
--- a/net/core/link_watch.c
+++ b/net/core/link_watch.c
@@ -170,13 +170,13 @@
 		spin_unlock_irqrestore(&lweventlist_lock, flags);
 
 		if (!test_and_set_bit(LW_RUNNING, &linkwatch_flags)) {
-			unsigned long thisevent = jiffies;
+			unsigned long delay = linkwatch_nextevent - jiffies;
 
-			if (thisevent >= linkwatch_nextevent) {
+			/* If we wrap around we'll delay it by at most HZ. */
+			if (!delay || delay > HZ)
 				schedule_work(&linkwatch_work);
-			} else {
-				schedule_delayed_work(&linkwatch_work, linkwatch_nextevent - thisevent);
-			}
+			else
+				schedule_delayed_work(&linkwatch_work, delay);
 		}
 	}
 }

Re: [NET] linkwatch: Handle jiffies wrap-around

[Stefan Rompf]

Am Dienstag 09 Mai 2006 13:26 schrieb Herbert Xu:

> The test used in the linkwatch does not handle wrap-arounds correctly.
> Since the intention of the code is to eliminate bursts of messages we
> can afford to delay things up to a second.

looks good, the code generates only up to 1 second delay for one second every 
49 days, avoiding 64bit operations. David, please prefer this patch over mine.

> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: Stefan Rompf <stefan@loplof.de>

Re: [NET] linkwatch: Handle jiffies wrap-around

[David S. Miller]

From: Stefan Rompf <stefan@loplof.de>
Date: Tue, 9 May 2006 18:51:49 +0200

> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Acked-by: Stefan Rompf <stefan@loplof.de>

Applied, thanks everyone.