[RFC PATCH]xfrm: fix perpetual bundles

[RFC PATCH]xfrm: fix perpetual bundles

[jamal]

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


Apologies for the shotgun email but this has been perplexing me for
sometime and i am worried the the patch i have is a band-aid (although
it fixes the problem), so here's a long-winded description..

Essentially, it is possible to create a scenario where the
xfrm policy->bundles grows indefinitely. This can be observed
from grep xfrm_dst_cache /proc/slabinfo
You can see the number of objects growing correlated to the number
of pings i send.... So if i sent 1K pings this way then stopped, 
there would be 1K objects in xfrm_dst_cache. If i start and send
another ping, slab grows to 2K.. etc

To recreate this, make sure you have a proper matching SP and SA and
any recent kernel (I can reproduce this in net-next 2.6 before and after
the xfrm mark merge). Use an app like iputils::ping. Ping uses raw
socket, does a connect() once and then sendmsg() for each packet.
You also need the receiver of ping to setup proper ipsec rules
so you get a response.

1)In the connect() stage, in the slow path a route cache is
created with the rth->fl.fl4_src of 0.0.0.0...
==> policy->bundles is empty, so we do a lookup, fail, create
one.. (remember rth->fl.fl4_src of 0.0.0.0 at this stage and
thats what we end storing in the bundle/xdst for later comparison
instead of the skb's fl)

2)ping sends a packet (does a sendmsg) 
==> xfrm_find_bundle() ends up comparing skb's->fl (non-zero
fl->fl4_src) with what we stored from #1b. Fails.
==> we create a new bundle at attach the old one at the end of it.
...and now policy->bundles has two xdst entries

3) Repeat #2, and now we have 3 xdsts in policy bundles

4) Repeat #2, and now we have 4 xdsts in policy bundles..

5) Send 7 more pings and look at slabinfo and youll see
10 object all of which are active..

Essentially this seems to go on and on and i can cache a huge
number of xdsts..

Of course if i do a ping -I <srcaddr>, ping does a bind();
then no problem since the correct rth->fl.fl4_src shows up
from connect and all goes well.
My patch assumes that a fl4_src of 0.0.0.0 is a wildcard
since if you bind to INADDR_ANY thats what it would be.
Another way to solve this would be in #1 above to copy
the skb's->fl instead of the dst's.

cheers,
jamal

[-- Attachment #2: perp-xdst --]
[-- Type: text/x-patch, Size: 634 bytes --]

diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index 67107d6..8a58843 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -69,7 +69,8 @@ __xfrm4_find_bundle(struct flowi *fl, struct xfrm_policy *policy)
 		struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
 		if (xdst->u.rt.fl.oif == fl->oif &&	/*XXX*/
 		    xdst->u.rt.fl.fl4_dst == fl->fl4_dst &&
-		    xdst->u.rt.fl.fl4_src == fl->fl4_src &&
+		    (!xdst->u.rt.fl.fl4_src ||
+		     xdst->u.rt.fl.fl4_src == fl->fl4_src) &&
 		    xdst->u.rt.fl.fl4_tos == fl->fl4_tos &&
 		    xfrm_bundle_ok(policy, xdst, fl, AF_INET, 0)) {
 			dst_clone(dst);

Re: [RFC PATCH]xfrm: fix perpetual bundles

[Steffen Klassert]

On Wed, Feb 24, 2010 at 08:19:24AM -0500, jamal wrote:
> 
> 1)In the connect() stage, in the slow path a route cache is
> created with the rth->fl.fl4_src of 0.0.0.0...
> ==> policy->bundles is empty, so we do a lookup, fail, create
> one.. (remember rth->fl.fl4_src of 0.0.0.0 at this stage and
> thats what we end storing in the bundle/xdst for later comparison
> instead of the skb's fl)
> 
> 2)ping sends a packet (does a sendmsg) 
> ==> xfrm_find_bundle() ends up comparing skb's->fl (non-zero
> fl->fl4_src) with what we stored from #1b. Fails.
> ==> we create a new bundle at attach the old one at the end of it.
> ...and now policy->bundles has two xdst entries
> 
> 3) Repeat #2, and now we have 3 xdsts in policy bundles
> 
> 4) Repeat #2, and now we have 4 xdsts in policy bundles..
> 
> 5) Send 7 more pings and look at slabinfo and youll see
> 10 object all of which are active..
> 
> Essentially this seems to go on and on and i can cache a huge
> number of xdsts..
> 

Do you have CONFIG_XFRM_SUB_POLICY enabled?

I observed the same behaviour recently when I had CONFIG_XFRM_SUB_POLICY
enabled. The problem in my case is, that we do a route lookup based on a flow
with a source address of 0.0.0.0 in ip_route_output_flow() if we send a ping
packet. Then we update the flow's source address based on the routing
informations we got from __ip_route_output_key(). Now the actual flow does
not match the the flow information in the routing table anymore. As a result,
we generate a new xfrm bundle entry with every ping packet, as you pointed out.

I solved this by rerunning __ip_route_output_key() if we change the source or
destination address of the flow (patch below). I have not send the patch so
far because I'm not that familiar with the routing code and I'm still not sure
whether this is the right way to fix it, so I wanted to do some further
analysis first.

Interestingly this does not happen if CONFIG_XFRM_SUB_POLICY is disabled.

When ping is started, it opens a udp socket. This triggers a xfrm_lookup()
and a xfrm bundle entry is generated. In the standard case, the flow of the
ping packets matching the flow informations from the bundle entry generated 
by the opening of the udp socket, because we don't care for the upper layer
flow information here. Unlike the standard case, if CONFIG_XFRM_SUB_POLICY is
enabled we do upper layer information matching with flow_cache_uli_match().
Now the flow of the ping packets does, of course, not match the flow
informations of the bundle entry and we generate a new bundle entry with
every packet...


---
 net/ipv4/route.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index d62b05d..3bf0b89 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2778,15 +2778,26 @@ int ip_route_output_flow(struct net *net, struct rtable **rp, struct flowi *flp,
 			 struct sock *sk, int flags)
 {
 	int err;
+	int update_route = 0;
 
 	if ((err = __ip_route_output_key(net, rp, flp)) != 0)
 		return err;
 
 	if (flp->proto) {
-		if (!flp->fl4_src)
+		if (!flp->fl4_src) {
 			flp->fl4_src = (*rp)->rt_src;
-		if (!flp->fl4_dst)
+			update_route = 1;
+		}
+		if (!flp->fl4_dst) {
 			flp->fl4_dst = (*rp)->rt_dst;
+			update_route = 1;
+		}
+		if (update_route) {
+			dst_release(&(*rp)->u.dst);
+			if ((err = __ip_route_output_key(net, rp, flp)) != 0)
+				return err;
+		}
+
 		err = __xfrm_lookup(net, (struct dst_entry **)rp, flp, sk,
 				    flags ? XFRM_LOOKUP_WAIT : 0);
 		if (err == -EREMOTE)
-- 
1.5.6.5

Re: [RFC PATCH]xfrm: fix perpetual bundles

[jamal]

Hi Steffen,

On Thu, 2010-02-25 at 11:40 +0100, Steffen Klassert wrote:

> 
> Do you have CONFIG_XFRM_SUB_POLICY enabled?

I tried with and without. If in xfrm_bundle_create()
after the call to xfrm_fill_dst() somewhere i "fixed" the
xdst->u.rt.fl.fl4_src, as in:
---
                err = xfrm_fill_dst(xdst, dev);
                if (err)
                        goto free_dst;
 

+               if (!xdst->u.rt.fl.fl4_src) {
+                       xdst->u.rt.fl.fl4_src = fl->fl4_src;
+		}
----

Then this worked as long as i turned off CONFIG_XFRM_SUB_POLICY.
If i use the simple patch i posted - it worked with or without
CONFIG_XFRM_SUB_POLICY turned on.

> I observed the same behaviour recently when I had CONFIG_XFRM_SUB_POLICY
> enabled. The problem in my case is, that we do a route lookup based on a flow
> with a source address of 0.0.0.0 in ip_route_output_flow() if we send a ping
> packet. Then we update the flow's source address based on the routing
> informations we got from __ip_route_output_key(). Now the actual flow does
> not match the the flow information in the routing table anymore. As a result,
> we generate a new xfrm bundle entry with every ping packet, as you pointed out.
> 

nod.

> I solved this by rerunning __ip_route_output_key() if we change the source or
> destination address of the flow (patch below). I have not send the patch so
> far because I'm not that familiar with the routing code and I'm still not sure
> whether this is the right way to fix it, so I wanted to do some further
> analysis first.
> 
> Interestingly this does not happen if CONFIG_XFRM_SUB_POLICY is disabled.
>
> When ping is started, it opens a udp socket. This triggers a xfrm_lookup()
> and a xfrm bundle entry is generated. In the standard case, the flow of the
> ping packets matching the flow informations from the bundle entry generated 
> by the opening of the udp socket, because we don't care for the upper layer
> flow information here. Unlike the standard case, if CONFIG_XFRM_SUB_POLICY is
> enabled we do upper layer information matching with flow_cache_uli_match().

Precisely - i actually was failing at exactly the same spot with
CONFIG_XFRM_SUB_POLICY with the "fix" i described above. 
"Fixing it" at that path level you have below may have bigger effect - i
cant think of one right now; but that path is hit by all outgoing
packets...

Lets hear what other people have to say - but iam beginning to feel
probably the change i posted is not so unreasonable since 0.0.0.0
is INADDR_ANY.

cheers,
jamal

Re: [RFC PATCH]xfrm: fix perpetual bundles

[jamal]

Steffen,

Did you try the simple patch i posted? After contemplating
in the background i think it is the right thing to do.
Ive also fixed IPV6 side the same way.

cheers,
jamal
On Thu, 2010-02-25 at 08:19 -0500, jamal wrote:


> 
> Lets hear what other people have to say - but iam beginning to feel
> probably the change i posted is not so unreasonable since 0.0.0.0
> is INADDR_ANY.
> 
> cheers,
> jamal

Re: [RFC PATCH]xfrm: fix perpetual bundles

[Steffen Klassert]

Hi Jamal.

On Sun, Feb 28, 2010 at 09:07:15AM -0500, jamal wrote:
> Steffen,
> 
> Did you try the simple patch i posted? After contemplating
> in the background i think it is the right thing to do.
> Ive also fixed IPV6 side the same way.
> 

Yes, I did. Your patch works fine. As your solution is less
invasive we probaply should take your patches if nobody else
has an opinion on this.

Steffen

Re: [RFC PATCH]xfrm: fix perpetual bundles

[Herbert Xu]

On Wed, Feb 24, 2010 at 08:19:24AM -0500, jamal wrote:
> 
> Apologies for the shotgun email but this has been perplexing me for
> sometime and i am worried the the patch i have is a band-aid (although
> it fixes the problem), so here's a long-winded description..

First of all I don't think patch fixes the root (or rather, roots)
of the problem.
 
> 1)In the connect() stage, in the slow path a route cache is
> created with the rth->fl.fl4_src of 0.0.0.0...
> ==> policy->bundles is empty, so we do a lookup, fail, create
> one.. (remember rth->fl.fl4_src of 0.0.0.0 at this stage and
> thats what we end storing in the bundle/xdst for later comparison
> instead of the skb's fl)

So this is root number 1.  When this stuff was first written this
case simply wasn't possible.  So I think the question we need to
ask here is can we get a valid address there at the connect stage?

After all, for non-IPsec connect(2)s, you do get a valid IP address.
So I don't see why the IPsec case should be different.

Creating a bundle with a zero source address is just a hack to
make connect(2) succeed immediately.  AFAICS getting a valid IP
address can also be done without waiting for the whole IPsec state
to be created.

Of course if anybody is still interested we could also revisit
the neighbouresque queueing idea.

> 2)ping sends a packet (does a sendmsg) 
> ==> xfrm_find_bundle() ends up comparing skb's->fl (non-zero
> fl->fl4_src) with what we stored from #1b. Fails.
> ==> we create a new bundle at attach the old one at the end of it.
> ...and now policy->bundles has two xdst entries

This is the way it's supposed to work.

> 3) Repeat #2, and now we have 3 xdsts in policy bundles

This is what I don't understand.  The code is supposed to look
at every bundle attached to the policy.  So why doesn't it find
the one we created at step #2?

In conclusion, I think we have two problems, with the second
one being the most immediate cause of your symptoms.

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

Re: [RFC PATCH]xfrm: fix perpetual bundles

[jamal]

On Tue, 2010-03-02 at 19:27 +0800, Herbert Xu wrote:
> On Wed, Feb 24, 2010 at 08:19:24AM -0500, jamal wrote:

> > 1)In the connect() stage, in the slow path a route cache is
> > created with the rth->fl.fl4_src of 0.0.0.0...
> > ==> policy->bundles is empty, so we do a lookup, fail, create
> > one.. (remember rth->fl.fl4_src of 0.0.0.0 at this stage and
> > thats what we end storing in the bundle/xdst for later comparison
> > instead of the skb's fl)
> 
> So this is root number 1.  When this stuff was first written this
> case simply wasn't possible.  So I think the question we need to
> ask here is can we get a valid address there at the connect stage?

fl->fl4_src is valid non-zero. But in xfrm4_fill_dst()
we do wholesale copy of xdst->u.rt.fl = rt->fl; and rt->fl.fl4_src is
0.0.0.0.


> After all, for non-IPsec connect(2)s, you do get a valid IP address.
> So I don't see why the IPsec case should be different.
>
> Creating a bundle with a zero source address is just a hack to
> make connect(2) succeed immediately.  AFAICS getting a valid IP
> address can also be done without waiting for the whole IPsec state
> to be created.
> 

I did try to "fix it" above via:
+               if (!xdst->u.rt.fl.fl4_src) {
+                       xdst->u.rt.fl.fl4_src = fl->fl4_src;
+               }

But this breaks again later in sendmsg bundle lookup because of 
XFRM_SUB_POLICY. If i turned off config XFRM_SUB_POLICY, then
all works. I didnt look closely, but SUB_POLICY does do a memcpy
or two off the dst passed in connect() - which has the wrong src.
So i would have to "fix" a few more spots for it to work. This is
where i gave up concluding that i was just plugging with band-aids.

> Of course if anybody is still interested we could also revisit
> the neighbouresque queueing idea.

not plugged into that discussion..

> > 2)ping sends a packet (does a sendmsg) 
> > ==> xfrm_find_bundle() ends up comparing skb's->fl (non-zero
> > fl->fl4_src) with what we stored from #1b. Fails.
> > ==> we create a new bundle at attach the old one at the end of it.
> > ...and now policy->bundles has two xdst entries
> 
> This is the way it's supposed to work.

> > 3) Repeat #2, and now we have 3 xdsts in policy bundles
> 
> This is what I don't understand.  The code is supposed to look
> at every bundle attached to the policy.  So why doesn't it find
> the one we created at step #2?

The issue is that the comparison is between xdst->u.rt.fl.fl4_src and
fl->fl4_src. fl->fl4_src is always non-zero. stored
xdst->u.rt.fl.fl4_src is always zero 

> In conclusion, I think we have two problems, with the second
> one being the most immediate cause of your symptoms.

Remember the route cache (refer to dst copying above) is created at
connect time;->

So Steffen (on CC) tried to "fix it" by fixing at route cache creation
time. His approach:

--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2778,15 +2778,26 @@ int ip_route_output_flow(struct net *net, struct
rtable **rp, struct flowi *flp,
                         struct sock *sk, int flags)
 {
        int err;
+       int update_route = 0;
 
        if ((err = __ip_route_output_key(net, rp, flp)) != 0)
                return err;
 
        if (flp->proto) {
-               if (!flp->fl4_src)
+               if (!flp->fl4_src) {
                        flp->fl4_src = (*rp)->rt_src;
-               if (!flp->fl4_dst)
+                       update_route = 1;
+               }
+               if (!flp->fl4_dst) {
                        flp->fl4_dst = (*rp)->rt_dst;
+                       update_route = 1;
+               }
+               if (update_route) {
+                       dst_release(&(*rp)->u.dst);
+                       if ((err = __ip_route_output_key(net, rp,
flp)) != 0)
+                               return err;
+               }
+
                err = __xfrm_lookup(net, (struct dst_entry **)rp, flp,
sk,
                                    flags ? XFRM_LOOKUP_WAIT : 0);
                if (err == -EREMOTE)
--

I was worried about the impact of this on something else that expects
the behavior.

cheers,
jamal

Re: [RFC PATCH]xfrm: fix perpetual bundles

[Herbert Xu]

On Tue, Mar 02, 2010 at 07:11:45AM -0500, jamal wrote:
>
> fl->fl4_src is valid non-zero. But in xfrm4_fill_dst()

I see, so problem #1 doesn't exist.

> we do wholesale copy of xdst->u.rt.fl = rt->fl; and rt->fl.fl4_src is
> 0.0.0.0.

Heh, you've just discovered a bug that I carefully planted back
in 2007, while merging the v4/v6 policy code :)

It is a clear merging error, where *fl became rt->fl which is
totally different.  So please try this patch:

ipsec: Fix bogus bundle flowi

When I merged the bundle creation code, I introduced a bogus
flowi value in the bundle.  Instead of getting from the caller,
it was instead set to the flow in the route object, which is
totally different.

The end result is that the bundles we created never match, and
we instead end up with an ever growing bundle list.

Thanks to Jamal for find this problem.

Reported-by: Jamal Hadi Salim <hadi@cyberus.ca>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 60c2770..1e355d8 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -274,7 +274,8 @@ struct xfrm_policy_afinfo {
 					     struct dst_entry *dst,
 					     int nfheader_len);
 	int			(*fill_dst)(struct xfrm_dst *xdst,
-					    struct net_device *dev);
+					    struct net_device *dev,
+					    struct flowi *fl);
 };
 
 extern int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo);
diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index 67107d6..e4a1483 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -91,11 +91,12 @@ static int xfrm4_init_path(struct xfrm_dst *path, struct dst_entry *dst,
 	return 0;
 }
 
-static int xfrm4_fill_dst(struct xfrm_dst *xdst, struct net_device *dev)
+static int xfrm4_fill_dst(struct xfrm_dst *xdst, struct net_device *dev,
+			  struct flowi *fl)
 {
 	struct rtable *rt = (struct rtable *)xdst->route;
 
-	xdst->u.rt.fl = rt->fl;
+	xdst->u.rt.fl = *fl;
 
 	xdst->u.dst.dev = dev;
 	dev_hold(dev);
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index dbdc696..ae18165 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -116,7 +116,8 @@ static int xfrm6_init_path(struct xfrm_dst *path, struct dst_entry *dst,
 	return 0;
 }
 
-static int xfrm6_fill_dst(struct xfrm_dst *xdst, struct net_device *dev)
+static int xfrm6_fill_dst(struct xfrm_dst *xdst, struct net_device *dev,
+			  struct flowi *fl)
 {
 	struct rt6_info *rt = (struct rt6_info*)xdst->route;
 
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 0ecb16a..f12dd3d 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1354,7 +1354,8 @@ static inline int xfrm_init_path(struct xfrm_dst *path, struct dst_entry *dst,
 	return err;
 }
 
-static inline int xfrm_fill_dst(struct xfrm_dst *xdst, struct net_device *dev)
+static inline int xfrm_fill_dst(struct xfrm_dst *xdst, struct net_device *dev,
+				struct flowi *fl)
 {
 	struct xfrm_policy_afinfo *afinfo =
 		xfrm_policy_get_afinfo(xdst->u.dst.ops->family);
@@ -1363,7 +1364,7 @@ static inline int xfrm_fill_dst(struct xfrm_dst *xdst, struct net_device *dev)
 	if (!afinfo)
 		return -EINVAL;
 
-	err = afinfo->fill_dst(xdst, dev);
+	err = afinfo->fill_dst(xdst, dev, fl);
 
 	xfrm_policy_put_afinfo(afinfo);
 
@@ -1468,7 +1469,7 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	for (dst_prev = dst0; dst_prev != dst; dst_prev = dst_prev->child) {
 		struct xfrm_dst *xdst = (struct xfrm_dst *)dst_prev;
 
-		err = xfrm_fill_dst(xdst, dev);
+		err = xfrm_fill_dst(xdst, dev, fl);
 		if (err)
 			goto free_dst;
 
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

Re: [RFC PATCH]xfrm: fix perpetual bundles

[jamal]

On Tue, 2010-03-02 at 20:51 +0800, Herbert Xu wrote:

> Heh, you've just discovered a bug that I carefully planted back
> in 2007, while merging the v4/v6 policy code :)

;-> I am suprised it hasnt been noticed sooner given it accumulates
memory on a per-packet basis.

> It is a clear merging error, where *fl became rt->fl which is
> totally different.  So please try this patch:
> 

Looks like it would work.
I dont have time right now - but will by either tonight or tomorrow
evening. Steffen, if you have time - please go ahead and try it out
as well.

cheers,
jamal

Re: [RFC PATCH]xfrm: fix perpetual bundles

[Steffen Klassert]

On Tue, Mar 02, 2010 at 08:10:26AM -0500, jamal wrote:
> On Tue, 2010-03-02 at 20:51 +0800, Herbert Xu wrote:
> 
> > Heh, you've just discovered a bug that I carefully planted back
> > in 2007, while merging the v4/v6 policy code :)
> 
> ;-> I am suprised it hasnt been noticed sooner given it accumulates
> memory on a per-packet basis.

The problem was spotted by 
commit a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
xfrm: select sane defaults for xfrm[4|6] gc_thresh

Before this commit, the xfrm garbage collector started to remove
stale bundle entries as soon as we reached an amount of 1024
bundle entries. Now the default value for the gc_thresh is
based on the main route table hash size, so we can have much more
bundle entries.

> 
> > It is a clear merging error, where *fl became rt->fl which is
> > totally different.  So please try this patch:
> > 
> 
> Looks like it would work.
> I dont have time right now - but will by either tonight or tomorrow
> evening. Steffen, if you have time - please go ahead and try it out
> as well.
> 

I tried it, works for me too.

Thanks,

Steffen

Re: [RFC PATCH]xfrm: fix perpetual bundles

[jamal]

On Tue, 2010-03-02 at 14:46 +0100, Steffen Klassert wrote:

> The problem was spotted by 
> commit a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
> xfrm: select sane defaults for xfrm[4|6] gc_thresh
> 
> Before this commit, the xfrm garbage collector started to remove
> stale bundle entries as soon as we reached an amount of 1024
> bundle entries. Now the default value for the gc_thresh is
> based on the main route table hash size, so we can have much more
> bundle entries.

yikes. Ok. Seems this fix needs to go -stable as well then.

> I tried it, works for me too.

Did you try with CONFIG_XFRM_SUB_POLICY=y. Thats the only reason
i said "looks like it might work". If you tried with that, then
I dont need to test and I can add an ACKed-by;-> 

cheers,
jamal

Re: [RFC PATCH]xfrm: fix perpetual bundles

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Tue, 02 Mar 2010 08:54:30 -0500

> On Tue, 2010-03-02 at 14:46 +0100, Steffen Klassert wrote:
> 
>> The problem was spotted by 
>> commit a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
>> xfrm: select sane defaults for xfrm[4|6] gc_thresh
>> 
>> Before this commit, the xfrm garbage collector started to remove
>> stale bundle entries as soon as we reached an amount of 1024
>> bundle entries. Now the default value for the gc_thresh is
>> based on the main route table hash size, so we can have much more
>> bundle entries.
> 
> yikes. Ok. Seems this fix needs to go -stable as well then.

Jamal please do some testing then I'll push Herbert's fix around.

Re: [RFC PATCH]xfrm: fix perpetual bundles

[Steffen Klassert]

On Tue, Mar 02, 2010 at 08:54:30AM -0500, jamal wrote:
> 
> yikes. Ok. Seems this fix needs to go -stable as well then.

Indeed, it should.

> 
> > I tried it, works for me too.
> 
> Did you try with CONFIG_XFRM_SUB_POLICY=y. Thats the only reason
> i said "looks like it might work". If you tried with that, then
> I dont need to test and I can add an ACKed-by;-> 
> 

Yes, I tested with CONFIG_XFRM_SUB_POLICY=y. 
So for me I can add an
Acked-by: Steffen Klassert <steffen.klassert@secunet.com>

Re: [RFC PATCH]xfrm: fix perpetual bundles

[jamal]

On Tue, 2010-03-02 at 06:06 -0800, David Miller wrote:

> Jamal please do some testing then I'll push Herbert's fix around.

I will get to it tonight.

cheers,
jamal

Re: [RFC PATCH]xfrm: fix perpetual bundles

[jamal]

> Acked-by: Steffen Klassert <steffen.klassert@secunet.com>

tested - Looks good.
Acked-by: Jamal Hadi Salim <hadi@cyberus.ca>

cheers,
jamal

Re: [RFC PATCH]xfrm: fix perpetual bundles

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Tue, 02 Mar 2010 19:47:02 -0500

> 
>> Acked-by: Steffen Klassert <steffen.klassert@secunet.com>
> 
> tested - Looks good.
> Acked-by: Jamal Hadi Salim <hadi@cyberus.ca>

Applied and queued up for -stable, thanks everyone.