[PATCH] route: Fix caught BUG_ON during rt_secret_rebuild_oneshot()

[PATCH] route: Fix caught BUG_ON during rt_secret_rebuild_oneshot()

[Vitaliy Gusev]

route: Fix caught BUG_ON during rt_secret_rebuild_oneshot()

Call rt_secret_rebuild can cause BUG_ON(timer_pending(&net->ipv4.rt_secret_timer)) in
add_timer as there is not any synchronization for call rt_secret_rebuild_oneshot()
for the same net namespace.

Also this issue affects to rt_secret_reschedule().

Thus use mod_timer enstead.

Signed-off-by: Vitaliy Gusev <vgusev@openvz.org>

---
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 4f11faa..8a77318 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -922,10 +922,8 @@ static void rt_secret_rebuild_oneshot(struct net *net)
 {
 	del_timer_sync(&net->ipv4.rt_secret_timer);
 	rt_cache_invalidate(net);
-	if (ip_rt_secret_interval) {
-		net->ipv4.rt_secret_timer.expires += ip_rt_secret_interval;
-		add_timer(&net->ipv4.rt_secret_timer);
-	}
+	if (ip_rt_secret_interval)
+		mod_timer(&net->ipv4.rt_secret_timer, jiffies + ip_rt_secret_interval);
 }
 
 static void rt_emergency_hash_rebuild(struct net *net)
@@ -3072,22 +3070,20 @@ static void rt_secret_reschedule(int old)
 	rtnl_lock();
 	for_each_net(net) {
 		int deleted = del_timer_sync(&net->ipv4.rt_secret_timer);
+		long time;
 
 		if (!new)
 			continue;
 
 		if (deleted) {
-			long time = net->ipv4.rt_secret_timer.expires - jiffies;
+			time = net->ipv4.rt_secret_timer.expires - jiffies;
 
 			if (time <= 0 || (time += diff) <= 0)
 				time = 0;
-
-			net->ipv4.rt_secret_timer.expires = time;
 		} else
-			net->ipv4.rt_secret_timer.expires = new;
+			time = new;
 
-		net->ipv4.rt_secret_timer.expires += jiffies;
-		add_timer(&net->ipv4.rt_secret_timer);
+		mod_timer(&net->ipv4.rt_secret_timer, jiffies + time);
 	}
 	rtnl_unlock();
 }

Re: [PATCH] route: Fix caught BUG_ON during rt_secret_rebuild_oneshot()

[Neil Horman]

On Tue, Mar 16, 2010 at 02:07:51PM +0300, Vitaliy Gusev wrote:
> route: Fix caught BUG_ON during rt_secret_rebuild_oneshot()
> 
> Call rt_secret_rebuild can cause BUG_ON(timer_pending(&net->ipv4.rt_secret_timer)) in
> add_timer as there is not any synchronization for call rt_secret_rebuild_oneshot()
> for the same net namespace.
> 
> Also this issue affects to rt_secret_reschedule().
> 
> Thus use mod_timer enstead.
> 
> Signed-off-by: Vitaliy Gusev <vgusev@openvz.org>
> 
> ---
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 4f11faa..8a77318 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -922,10 +922,8 @@ static void rt_secret_rebuild_oneshot(struct net *net)
>  {
>  	del_timer_sync(&net->ipv4.rt_secret_timer);
>  	rt_cache_invalidate(net);
> -	if (ip_rt_secret_interval) {
> -		net->ipv4.rt_secret_timer.expires += ip_rt_secret_interval;
> -		add_timer(&net->ipv4.rt_secret_timer);
> -	}
> +	if (ip_rt_secret_interval)
> +		mod_timer(&net->ipv4.rt_secret_timer, jiffies + ip_rt_secret_interval);
>  }
>  
>  static void rt_emergency_hash_rebuild(struct net *net)
> @@ -3072,22 +3070,20 @@ static void rt_secret_reschedule(int old)
>  	rtnl_lock();
>  	for_each_net(net) {
>  		int deleted = del_timer_sync(&net->ipv4.rt_secret_timer);
> +		long time;
>  
>  		if (!new)
>  			continue;
>  
>  		if (deleted) {
> -			long time = net->ipv4.rt_secret_timer.expires - jiffies;
> +			time = net->ipv4.rt_secret_timer.expires - jiffies;
>  
>  			if (time <= 0 || (time += diff) <= 0)
>  				time = 0;
> -
> -			net->ipv4.rt_secret_timer.expires = time;
>  		} else
> -			net->ipv4.rt_secret_timer.expires = new;
> +			time = new;
>  
> -		net->ipv4.rt_secret_timer.expires += jiffies;
> -		add_timer(&net->ipv4.rt_secret_timer);
> +		mod_timer(&net->ipv4.rt_secret_timer, jiffies + time);
>  	}
>  	rtnl_unlock();
>  }

Yeah, this seems reasonable.  Although this might be a good time to bring up the
subject of deprecating or removing the secret interval.  We've had our
statistical analysis bits in place to drive the rebuilding of the cache in the
event that any one chain gets too long, and in the year or so that we've had it,
it seems we've shaken out a few bugs with it (which suggests that people are
using it routinely).  As such, might it be time to just drop the rebuild timer
in its entirety?

Until that decision is made of course, the above seems like a sane fix for it.
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH] route: Fix caught BUG_ON during rt_secret_rebuild_oneshot()

[Vitaliy Gusev]

On Tuesday 16 March 2010 05:34:37 pm Neil Horman wrote:
...
> Yeah, this seems reasonable.  Although this might be a good time to bring up the
> subject of deprecating or removing the secret interval.  We've had our
> statistical analysis bits in place to drive the rebuilding of the cache in the
> event that any one chain gets too long, and in the year or so that we've had it,
> it seems we've shaken out a few bugs with it (which suggests that people are
> using it routinely).  As such, might it be time to just drop the rebuild timer
> in its entirety?

Rebuild timer was introduced by secure reason when there was no analysis for
chain lenght. Now it seems that rebuild timer can be removed.


> 
> Until that decision is made of course, the above seems like a sane fix for it.
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 



Thanks,
Vitaliy Gusev

Re: [PATCH] route: Fix caught BUG_ON during rt_secret_rebuild_oneshot()

[Neil Horman]

On Tue, Mar 16, 2010 at 06:27:08PM +0300, Vitaliy Gusev wrote:
> On Tuesday 16 March 2010 05:34:37 pm Neil Horman wrote:
> ...
> > Yeah, this seems reasonable.  Although this might be a good time to bring up the
> > subject of deprecating or removing the secret interval.  We've had our
> > statistical analysis bits in place to drive the rebuilding of the cache in the
> > event that any one chain gets too long, and in the year or so that we've had it,
> > it seems we've shaken out a few bugs with it (which suggests that people are
> > using it routinely).  As such, might it be time to just drop the rebuild timer
> > in its entirety?
> 
> Rebuild timer was introduced by secure reason when there was no analysis for
> chain lenght. Now it seems that rebuild timer can be removed.
> 
Dave, Eric, Herbert, what are your opinions on this?  Is the statistical analysis code
mature enough at this point to warrant the removal of the rebuild timer, or
should we still keep it as a fallback mechanism?
Neil

> 
> > 
> > Until that decision is made of course, the above seems like a sane fix for it.
> > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > 
> > 
> 
> 
> 
> Thanks,
> Vitaliy Gusev
> 

Re: [PATCH] route: Fix caught BUG_ON during rt_secret_rebuild_oneshot()

[Eric Dumazet]

Le mardi 16 mars 2010 à 13:35 -0400, Neil Horman a écrit :
> > 
> Dave, Eric, Herbert, what are your opinions on this?  Is the statistical analysis code
> mature enough at this point to warrant the removal of the rebuild timer, or
> should we still keep it as a fallback mechanism?
> Neil

I believe most heavy duty users dont use newest kernels yet.

(I had to make some changes last month because Paweł Staszewski had
problems to get a working route cache)

We should wait a bit (one year or two) before removing this timer.

Thanks

Re: [PATCH] route: Fix caught BUG_ON during rt_secret_rebuild_oneshot()

[Neil Horman]

On Tue, Mar 16, 2010 at 07:08:42PM +0100, Eric Dumazet wrote:
> Le mardi 16 mars 2010 à 13:35 -0400, Neil Horman a écrit :
> > > 
> > Dave, Eric, Herbert, what are your opinions on this?  Is the statistical analysis code
> > mature enough at this point to warrant the removal of the rebuild timer, or
> > should we still keep it as a fallback mechanism?
> > Neil
> 
> I believe most heavy duty users dont use newest kernels yet.
> 
> (I had to make some changes last month because Paweł Staszewski had
> problems to get a working route cache)
> 
> We should wait a bit (one year or two) before removing this timer.
> 
> Thanks
> 
> 
> 
Ok, I just figured it was time to pose the question, since we'd fixed several
bugs in this area over the last year.  Certainly doesn't hurt to keep the
rebuild timer around a bit longer.  I re-iterate my ack to Vitaliy's patch then.

Regards
Neil

Re: [PATCH] route: Fix caught BUG_ON during rt_secret_rebuild_oneshot()

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 16 Mar 2010 14:18:16 -0400

> Ok, I just figured it was time to pose the question, since we'd fixed several
> bugs in this area over the last year.  Certainly doesn't hurt to keep the
> rebuild timer around a bit longer.  I re-iterate my ack to Vitaliy's patch then.

Applied, thanks everyone.

I frankly would be OK with seeing the rebuild timer go away
as early as 2.6.35

Re: [PATCH] route: Fix caught BUG_ON during rt_secret_rebuild_oneshot()

[Neil Horman]

On Tue, Mar 16, 2010 at 02:30:28PM -0700, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Tue, 16 Mar 2010 14:18:16 -0400
> 
> > Ok, I just figured it was time to pose the question, since we'd fixed several
> > bugs in this area over the last year.  Certainly doesn't hurt to keep the
> > rebuild timer around a bit longer.  I re-iterate my ack to Vitaliy's patch then.
> 
> Applied, thanks everyone.
> 
> I frankly would be OK with seeing the rebuild timer go away
> as early as 2.6.35
> 
I'll have something together for it around that time frame
Neil