| Message ID | 201003161407.51925.vgusev@openvz.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(); }
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> --- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html