From: Benjamin LaHaise <bcrl@kvack.org> Date: 2012-10-19 19:13:49
This is v2 of an attempt to pull in the relevant fixes for a problem in
v2.6.32 kernels where invalid cached routes are retained even after changes
to the routing table have been made. A simple test case can be found at
http://marc.info/?l=linux-netdev&m=135015076708950&w=2 . Based on feedback
from David Miller, additional changes have been pulled in, including fixes
for the same issue in IPv6. Most of the patches required some rework owing
to the large differences in the networking stack between 2.6.32 and 3.6.
I have performed basic tests to confirm that the cases I was hitting are
now fixed, including a couple of tests with IPv4 and IPv6. Comments?
Thanks again to David for the pointers to the additional fixes required in
this area.
Benjamin LaHaise (6):
ipv4: check_rt_genid in dst_check
net: Document dst->obsolete better.
ipv6: use DST_* macro to set obselete field
netns: move net->ipv4.rt_genid to net->rt_genid
ipv6: use net->rt_genid to check dst validity
xfrm: invalidate dst on policy insertion/deletion
include/net/dst.h | 14 +++++++++++++-
include/net/ip6_fib.h | 4 +---
include/net/net_namespace.h | 12 ++++++++++++
include/net/netns/ipv4.h | 1 -
net/core/dst.c | 4 ++--
net/ipv4/route.c | 28 +++++++++++++++-------------
net/ipv6/inet6_connection_sock.c | 24 +-----------------------
net/ipv6/route.c | 35 ++++++++++++++++++++++++-----------
net/sctp/output.c | 2 +-
net/xfrm/xfrm_policy.c | 22 ++++++++++++----------
security/selinux/include/xfrm.h | 1 +
11 files changed, 82 insertions(+), 65 deletions(-)
--
"Thought is the essence of where you are now."
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2012-10-19 19:21:03
commit d11a4dc18bf41719c9f0d7ed494d295dd2973b92
Author: Timo Teräs [off-list ref]
Date: Thu Mar 18 23:20:20 2010 +0000
ipv4: check rt_genid in dst_check
Xfrm_dst keeps a reference to ipv4 rtable entries on each
cached bundle. The only way to renew xfrm_dst when the underlying
route has changed, is to implement dst_check for this. This is
what ipv6 side does too.
The problems started after 87c1e12b5eeb7b30b4b41291bef8e0b41fc3dde9
("ipsec: Fix bogus bundle flowi") which fixed a bug causing xfrm_dst
to not get reused, until that all lookups always generated new
xfrm_dst with new route reference and path mtu worked. But after the
fix, the old routes started to get reused even after they were expired
causing pmtu to break (well it would occationally work if the rtable
gc had run recently and marked the route obsolete causing dst_check to
get called).
Signed-off-by: Timo Teras [off-list ref]
Acked-by: Herbert Xu [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
This commit is based on the above, with the addition of verifying blackhole
routes in the same manner.
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
net/ipv4/route.c | 17 ++++++++++++-----
1 files changed, 12 insertions(+), 5 deletions(-)
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2012-10-19 19:21:22
commit f5b0a8743601a4477419171f5046bd07d1c080a0
Author: David S. Miller [off-list ref]
Date: Thu Jul 19 12:31:33 2012 -0700
net: Document dst->obsolete better.
Add a big comment explaining how the field works, and use defines
instead of magic constants for the values assigned to it.
Suggested by Joe Perches.
Signed-off-by: David S. Miller [off-list ref]
This is a slightly modified backport of the above commit to cover additional
locationsi of dst.u.obsolete = -1.
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
include/net/dst.h | 14 +++++++++++++-
net/core/dst.c | 4 ++--
net/ipv4/route.c | 12 ++++++------
net/sctp/output.c | 2 +-
net/xfrm/xfrm_policy.c | 21 +++++++++++----------
5 files changed, 33 insertions(+), 20 deletions(-)
@@ -99,7 +99,7 @@ loop:*Butwedonothavestate"obsoleted, but*referencedbyparent", so it is right.*/-if(dst->obsolete>1)+if(dst->obsolete>0)continue;___dst_free(dst);
@@ -375,7 +375,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)skb_set_owner_w(nskb,sk);/* The 'obsolete' field of dst is set to 2 when a dst is freed. */-if(!dst||(dst->obsolete>1)){+if(!dst||dst->obsolete){dst_release(dst);sctp_transport_route(tp,NULL,sctp_sk(sk));if(asoc&&(asoc->param_flags&SPP_PMTUD_ENABLE)){
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2012-10-19 19:21:46
commit 2c20cbd7e3aa6e9dddc07975d3f3a89fe1f69c00
Author: Nicolas Dichtel [off-list ref]
Date: Mon Sep 10 22:09:47 2012 +0000
ipv6: use DST_* macro to set obselete field
Signed-off-by: Nicolas Dichtel [off-list ref]
Acked-by: Eric Dumazet [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
This is a slightly modified backport of the above commit that covers
additional locations setting dst->u.obsolete = -1.
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
net/ipv6/route.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2012-10-19 19:22:00
commit b42664f898c976247f7f609b8bb9c94d7475ca10
Author: Nicolas Dichtel [off-list ref]
Date: Mon Sep 10 22:09:44 2012 +0000
netns: move net->ipv4.rt_genid to net->rt_genid
This commit prepares the use of rt_genid by both IPv4 and IPv6.
Initialization is left in IPv4 part.
Signed-off-by: Nicolas Dichtel [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
Since 2.6.32 still has the routing cache, this patch has been modified to
retain the IPv4 method for updating rt_genid in rt_cache_invalidate() to
perturb the route cache's hash function.
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
include/net/net_namespace.h | 12 ++++++++++++
include/net/netns/ipv4.h | 1 -
net/ipv4/route.c | 11 +++--------
3 files changed, 15 insertions(+), 9 deletions(-)
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2012-10-19 19:22:16
commit 6f3118b571b8a4c06c7985dc3172c3526cb86253
Author: Nicolas Dichtel [off-list ref]
Date: Mon Sep 10 22:09:46 2012 +0000
ipv6: use net->rt_genid to check dst validity
IPv6 dst should take care of rt_genid too. When a xfrm policy is inserted or
deleted, all dst should be invalidated.
To force the validation, dst entries should be created with ->obsolete set t
o
DST_OBSOLETE_FORCE_CHK. This was already the case for all functions calling
ip6_dst_alloc(), except for ip6_rt_copy().
As a consequence, we can remove the specific code in inet6_connection_sock.
Signed-off-by: Nicolas Dichtel [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
This version of the above commit is slightly modified to compensate for
differences in ip6_dst_alloc().
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
include/net/ip6_fib.h | 4 +---
net/ipv6/inet6_connection_sock.c | 24 +-----------------------
net/ipv6/route.c | 29 +++++++++++++++++++++--------
3 files changed, 23 insertions(+), 34 deletions(-)
@@ -886,6 +894,13 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)rt=(structrt6_info*)dst;+/* All IPV6 dsts are created with ->obsolete set to the value+*DST_OBSOLETE_FORCE_CHKwhichforcesvalidationcallsdown+*intothisfunctionalways.+*/+if(rt->rt6i_genid!=rt_genid(dev_net(rt->u.dst.dev)))+returnNULL;+if(rt&&rt->rt6i_node&&(rt->rt6i_node->fn_sernum==cookie))returndst;
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2012-10-19 19:22:29
commit ee8372dd1989287c5eedb69d44bac43f69e496f1
Author: Nicolas Dichtel [off-list ref]
Date: Mon Sep 10 22:09:45 2012 +0000
xfrm: invalidate dst on policy insertion/deletion
When a policy is inserted or deleted, all dst should be recalculated.
Signed-off-by: Nicolas Dichtel [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
net/xfrm/xfrm_policy.c | 1 +
security/selinux/include/xfrm.h | 1 +
2 files changed, 2 insertions(+), 0 deletions(-)
Hi Ben,
On Fri, Oct 19, 2012 at 03:13:48PM -0400, Benjamin LaHaise wrote:
This is v2 of an attempt to pull in the relevant fixes for a problem in
v2.6.32 kernels where invalid cached routes are retained even after changes
to the routing table have been made. A simple test case can be found at
http://marc.info/?l=linux-netdev&m=135015076708950&w=2 . Based on feedback
from David Miller, additional changes have been pulled in, including fixes
for the same issue in IPv6. Most of the patches required some rework owing
to the large differences in the networking stack between 2.6.32 and 3.6.
I have performed basic tests to confirm that the cases I was hitting are
now fixed, including a couple of tests with IPv4 and IPv6. Comments?
Thanks again to David for the pointers to the additional fixes required in
this area.
Looks like you've done an amazing work. I'm not the best person to judge
if these changes are valid or not, so I'll trust you and David on this.
I just have two questions: since you had to backport them from 3.6 to 2.6.32,
I'm assuming that the equivalent was not ready in closer LTS kernels (2.6.34,
3.0, 3.2, 3.4). Does this mean they're affected by the same issue too or are
they immune ? And if they're affected, are you going to proceed with forward
porting of your work for the closest ones (.34 and 3.0 come to mind) ? And if
they aren't affected, do you think that picking some changes there would have
been easier ?
As you probably know, we want to ensure that people who upgrade from an old
LTS to a newer one won't experience regressions due to problems which were
fixed only in the older version.
Thanks !
Willy
On Fri, Oct 19, 2012 at 03:49:30PM -0400, David Miller wrote:
quoted
How about checking if these changes are already in 3.0/3.2/etc. or not
before asking such questions?
Because I didn't find the patches in 3.0 and Ben said he backported them
from 3.6, I think these are two valid reasons to ask, no ?
Well, the thing is, I personally don't consider them appropriate for
3.x.y -stable backports, and that's why I haven't submitted them.
OK. Is is because the issue is less important there or because the fix are
more risky than the issues they fix (or any other reason) ?
I have a different opinion about the risk/benefit ratio than Ben does.
I do not think these cases are important enough to enough people to
justify -stable inclusion at all.
On Fri, Oct 19, 2012 at 03:49:30PM -0400, David Miller wrote:
quoted
How about checking if these changes are already in 3.0/3.2/etc. or not
before asking such questions?
Because I didn't find the patches in 3.0 and Ben said he backported them
from 3.6, I think these are two valid reasons to ask, no ?
Well, the thing is, I personally don't consider them appropriate for
3.x.y -stable backports, and that's why I haven't submitted them.
OK. Is is because the issue is less important there or because the fix are
more risky than the issues they fix (or any other reason) ?
I have a different opinion about the risk/benefit ratio than Ben does.
I do not think these cases are important enough to enough people to
justify -stable inclusion at all.
OK, thanks for the precision.
So maybe in the end we should just merge d11a4dc18 that Ben found to be
the least invasive one fixing the issues, and we'd be in sync with the
rest of the stable branches, even if, as you noted a few days ago, it's
only a partial fix for the issue.
Ben, what's your opinion on this ? I know it's never fun to do backports
and not merge them later, but I trust David more than anyone else on the
network part, so if he decided that while incomplete, the patch above
was all that was needed for other stable branches, maybe we should just
stay on the safe side and do the same ?
Willy
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2012-10-19 20:18:05
On Fri, Oct 19, 2012 at 10:03:18PM +0200, Willy Tarreau wrote:
OK. Is is because the issue is less important there or because the fix are
more risky than the issues they fix (or any other reason) ?
The behaviour of 2.6.32.x is certainly a regression from previous stable
kernels used by various distributions (ie 2.6.18), and is quite serious
for those of us running routers on top of these releases.
-ben
--
"Thought is the essence of where you are now."
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2012-10-19 20:22:44
On Fri, Oct 19, 2012 at 10:14:31PM +0200, Willy Tarreau wrote:
...
So maybe in the end we should just merge d11a4dc18 that Ben found to be
the least invasive one fixing the issues, and we'd be in sync with the
rest of the stable branches, even if, as you noted a few days ago, it's
only a partial fix for the issue.
Ben, what's your opinion on this ? I know it's never fun to do backports
and not merge them later, but I trust David more than anyone else on the
network part, so if he decided that while incomplete, the patch above
was all that was needed for other stable branches, maybe we should just
stay on the safe side and do the same ?
There is a caveat to the minimally invasive fix: doing so will result in
cached routes always being lookup up when the check occurs. This could
potentially result in a performance regression from some users. The
tradeoffs here are really murky.
-ben
--
"Thought is the essence of where you are now."
On Fri, Oct 19, 2012 at 04:22:44PM -0400, Benjamin LaHaise wrote:
On Fri, Oct 19, 2012 at 10:14:31PM +0200, Willy Tarreau wrote:
...
quoted
So maybe in the end we should just merge d11a4dc18 that Ben found to be
the least invasive one fixing the issues, and we'd be in sync with the
rest of the stable branches, even if, as you noted a few days ago, it's
only a partial fix for the issue.
Ben, what's your opinion on this ? I know it's never fun to do backports
and not merge them later, but I trust David more than anyone else on the
network part, so if he decided that while incomplete, the patch above
was all that was needed for other stable branches, maybe we should just
stay on the safe side and do the same ?
There is a caveat to the minimally invasive fix: doing so will result in
cached routes always being lookup up when the check occurs. This could
potentially result in a performance regression from some users. The
tradeoffs here are really murky.
I see. Then users will have the same issue when upgrading from 2.6.32 to 3.0.
OK let's bisect the situation :
- current 2.6.32 users are facing a routing bug that needs be fixed.
- 2.6.34 and onwards do not have this bug but are probably affected by
lower performance due to the minimal fix.
- if we apply the minimal fix alone, some 2.6.32 users will suddenly
experience a performance regression (what order of magnitude would
be nice to estimate).
- if we apply all the fixes, 2.6.32 will experience the performance
regression only when the upgrade to newer kernels which don't have
these fixes.
- the fixes come with a risk of regression (as usual).
Note that everything is tied to the level of performance regression to
expect from the fix when jumping from current 2.6.32 to next LTS. For
example, if some other improvements in 3.0 compensate for the performance
loss caused by the fix, it might be OK to have the other fixes only in
2.6.32. But if it is an important enough performance drop, would the
following plan be acceptable to you and David ?
- we merge the minimal fix now in order to get rid of this nasty issue
and to be in sync with other stable kernels ; a performance regression
will happen, but users will face it anyway when upgrading.
- some of us confident enough in the other fixes use them in their
environment for some time to ensure they don't cause unexpected
regressions.
- if past some delay (weeks, months ?) no issue is uncovered, patches
are proposed for inclusion in more recent branches and we merge them
into older branches.
An alternative solution could be the following one if the performance
regression is supposed not to be that important :
- we only merge the minimal patch now to fix the issue and cross fingers
hoping nobody complains ;
- if someone complains, we submit him the other patches then merge them
if they address the regression and do not cause a new one ;
- depending on the importance of the observed performance regression,
the patches may or may not be adopted by more recent kernels.
BTW, do you have an idea of how to test the performance drop due to the
patch on a test platform ? I mean, I can inject packets and change routes,
but doing so randomly can prove nothing.
Thanks,
Willy
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2012-10-19 21:03:34
On Fri, Oct 19, 2012 at 10:53:00PM +0200, Willy Tarreau wrote:
I see. Then users will have the same issue when upgrading from 2.6.32 to 3.0.
OK let's bisect the situation :
- current 2.6.32 users are facing a routing bug that needs be fixed.
- 2.6.34 and onwards do not have this bug but are probably affected by
lower performance due to the minimal fix.
Argh, sorry (too many patches in flight), it looks like just cherry picking
d11a4dc18bf41719c9f0d7ed494d295dd2973b92 is okay. It does have the
rt_is_expired() changes, so it should not have much of a performance
regression. So going with only this change is probably the way to go
for now.
-ben
On Fri, Oct 19, 2012 at 05:03:33PM -0400, Benjamin LaHaise wrote:
On Fri, Oct 19, 2012 at 10:53:00PM +0200, Willy Tarreau wrote:
quoted
I see. Then users will have the same issue when upgrading from 2.6.32 to 3.0.
OK let's bisect the situation :
- current 2.6.32 users are facing a routing bug that needs be fixed.
- 2.6.34 and onwards do not have this bug but are probably affected by
lower performance due to the minimal fix.
Argh, sorry (too many patches in flight), it looks like just cherry picking
d11a4dc18bf41719c9f0d7ed494d295dd2973b92 is okay. It does have the
rt_is_expired() changes, so it should not have much of a performance
regression. So going with only this change is probably the way to go
for now.
Perfect then! I'm queuing it.
Thanks very much for your work !
Willy