IPSec: Policy dst bundles exhausting storage

IPSec: Policy dst bundles exhausting storage

[Tom Lendacky]

I've discovered a bug in IPv6 policy bundle creation/searching
(xfrm6_policy.c: __xfrm6_bundle_create and __xfrm6_find_bundle) during some
stress testing using udp (it happens with tcp also) in tunnel mode (it
happens in transport also).  Every time a udp packet is sent a new dst
bundle is created and chained to the policy.  Eventually after enough
packets are sent, the dst_alloc fails and no more packets can be sent.  In
IPv4, the first bundle that is created is used repeatedly as it should be.
In the __xfrm6_find_bundle function, the xdst->u.rt6.rt6i_src.addr appears
to not have been set correctly (it has a value of
0000:0000:0000:0000:0000:0001:0000:0000) and never matches the fl->fl6_src
value and so a match is never found causing the creation of a new bundle.
It would appear that some values aren't being set, or set correctly, during
the __xfrm6_bundle_create function.

One other thing I did notice in both the v4 and v6 bundle create functions
is the line x->u.rt.fl = *fl.  Shouldn't this be a memcpy?

Thanks,
Tom

Re: IPSec: Policy dst bundles exhausting storage

[David S. Miller]

   From: "Tom Lendacky" <toml@us.ibm.com>
   Date: Tue, 10 Jun 2003 18:32:00 -0500
   
   One other thing I did notice in both the v4 and v6 bundle create functions
   is the line x->u.rt.fl = *fl.  Shouldn't this be a memcpy?

Gcc emits a memcpy() check the assembly.  Structure assignment
is a perfectly legal to do this.

Re: IPSec: Policy dst bundles exhausting storage

[Tom Lendacky]

   Gcc emits a memcpy() check the assembly.  Structure assignment
   is a perfectly legal to do this.

Yes, that makes sense since the compiler doesn't complain.  I didn't
think to check the assembly.

As for the bug though, it appears that the "x->u.rt.fl = *fl" statement
shouldn't be performed in the IPv6 __xfrm6_bundle_create function.
Since the xfrm_dst structure is a union of the rtable structure and the
rt6_info structure (among others), setting the "x->u.rt.fl" is later
overwritten when statements such as "x->u.rt6.rt6i_... = ..." are
executed.  Should the IPv6 code be more like the IPv4 code?  A flowi
structure can be added to the rt6_info structure, rt6i_fl, which would
then allow the "x->u.rt.fl = *fl" to be changed to "x->u.rt6.rt6i_fl =
*fl".  And then the __xfrm6_find_bundle could make basically the same
checks as __xfrm4_find_bundle:

  if (xdst->u.rt6.rt6i_fl.oif == fl->oif &&
      !ipv6_addr_cmp(&xdst->u.rt6.rt6i_fl.fl6_dst, &fl->fl6_dst) &&
      !ipv6_addr_cmp(&xdst->u.rt6.rt6i_fl.fl6_src, &fl->fl6_src) && ...

I've tested this briefly and it appears to work, but I don't know all
of the intricacies of how this might effect other parts of the code.
I've attached a patch for review.  Let me know if this is ok (although
I'm leaving this afternoon for a long weekend and won't be back until
Monday).

Thanks,
Tom

diff -ur linux-2.5.70-orig/include/net/ip6_fib.h linux-2.5.70-new/include/net/ip6_fib.h
--- linux-2.5.70-orig/include/net/ip6_fib.h	2003-06-11 11:56:21.000000000 -0500
+++ linux-2.5.70-new/include/net/ip6_fib.h	2003-06-11 11:58:42.000000000 -0500
@@ -73,6 +73,8 @@
 	struct rt6key			rt6i_src;
 
 	u8				rt6i_protocol;
+
+	struct flowi			rt6i_fl;
 };
 
 struct fib6_walker_t
diff -ur linux-2.5.70-orig/net/ipv6/xfrm6_policy.c linux-2.5.70-new/net/ipv6/xfrm6_policy.c
--- linux-2.5.70-orig/net/ipv6/xfrm6_policy.c	2003-06-11 11:56:22.000000000 -0500
+++ linux-2.5.70-new/net/ipv6/xfrm6_policy.c	2003-06-11 11:58:52.000000000 -0500
@@ -60,8 +60,9 @@
 	read_lock_bh(&policy->lock);
 	for (dst = policy->bundles; dst; dst = dst->next) {
 		struct xfrm_dst *xdst = (struct xfrm_dst*)dst;
-		if (!ipv6_addr_cmp(&xdst->u.rt6.rt6i_dst.addr, &fl->fl6_dst) &&
-		    !ipv6_addr_cmp(&xdst->u.rt6.rt6i_src.addr, &fl->fl6_src) &&
+		if (xdst->u.rt6.rt6i_fl.oif == fl->oif && 
+		    !ipv6_addr_cmp(&xdst->u.rt6.rt6i_fl.fl6_dst, &fl->fl6_dst) &&
+		    !ipv6_addr_cmp(&xdst->u.rt6.rt6i_fl.fl6_src, &fl->fl6_src) &&
 		    __xfrm6_bundle_ok(xdst, fl)) {
 			dst_clone(dst);
 			break;
@@ -133,7 +134,7 @@
 	dst_prev->child = &rt->u.dst;
 	for (dst_prev = dst; dst_prev != &rt->u.dst; dst_prev = dst_prev->child) {
 		struct xfrm_dst *x = (struct xfrm_dst*)dst_prev;
-		x->u.rt.fl = *fl;
+		x->u.rt6.rt6i_fl = *fl;
 
 		dst_prev->dev = rt->u.dst.dev;
 		if (rt->u.dst.dev)

Re: IPSec: Policy dst bundles exhausting storage

[David S. Miller]

   From: Tom Lendacky <toml@us.ibm.com>
   Date: 11 Jun 2003 12:20:33 -0500

   As for the bug though, it appears that the "x->u.rt.fl = *fl"
   statement shouldn't be performed in the IPv6 __xfrm6_bundle_create
   function.

I have a better suggestion for fix:

1) Delete the "x->u.rt.fl = *fl;" line completely.

2) Fix the test in __xfrm6_find_bundle() to do a proper
   prefix-mask based address comparison.

   rt6->rt6i_{dst,src} are masked addresses, so direct
   comparison is wrong.

Can someone code this up?
Thanks.

Re: IPSec: Policy dst bundles exhausting storage

[Tom Lendacky]

   I have a better suggestion for fix:

   1) Delete the "x->u.rt.fl = *fl;" line completely.

   2) Fix the test in __xfrm6_find_bundle() to do a proper
      prefix-mask based address comparison.

      rt6->rt6i_{dst,src} are masked addresses, so direct
      comparison is wrong.

   Can someone code this up?

It doesn't look like anyone worked on this yet, so now that I'm back I'm
starting to look at it.  In prep for coding this up I deleted the
"x->u.rt.fl = *fl;" line and then I noticed that (at least in my
configuration) that the rt6->rt6i_src address and prefix length in the
xfrm_dst structure are always zero.  It's hard to tell if this is what the
actual value should be or if it is just not getting initialized.  Doing a
ping from Machine 1 to Machine 3 and based on my configuration I would
expect to see a source address/prefix length of fec0::/64.  The
rt6->rt6i_dst address and prefix length are correct with fec0:0:0:2::/64.
Any ideas?  My IPSec configuration looks like this:

  Machine 1           Machine 2           Machine 3
  fec0:0:0:1::10
  fec0::1 ----------- fec0::2
                      fec0:0:0:2::10 ---- fec0:0:0:2::11

  Machine 1:
  spdadd fec0:0:0:0::/64 fec0:0:0:2::/64 -P out ipsec
         esp/tunnel/fec0::1-fec0::2/require;
  spdadd fec0:0:0:2::/64 fec0:0:0:0::/64 -P in  ipsec
         esp/tunnel/fec0::2-fec0::1/require;

  add fec0::1 fec0::2 ... (spi, algorithms and keys)
  add fec0::2 fec0::1 ... (spi, algorithms and keys)

  Machine 2:
  spdadd fec0:0:0:0::/64 fec0:0:0:2::/64 -P in  ipsec
         esp/tunnel/fec0::1-fec0::2/require;
  spdadd fec0:0:0:2::/64 fec0:0:0:0::/64 -P out ipsec
         esp/tunnel/fec0::2-fec0::1/require;

  add fec0::1 fec0::2 ... (spi, algorithms and keys)
  add fec0::2 fec0::1 ... (spi, algorithms and keys)

A netstat -rn --inet6 on Machine 1 yields:

Kernel IPv6 routing table
Destination                   Next Hop  Flags Metric Ref    Use Iface
::1/128                       ::        U     0      0        0 lo
fe80::/128                    ::        U     0      0        0 lo
fe80::201:3ff:fe33:5355/128   ::        U     0      0        0 lo
fe80::202:55ff:fe7c:79b6/128  ::        U     0      0        0 lo
fe80::202:55ff:fee4:5bb6/128  ::        U     0      0        0 lo
fe80::/64                     ::        UA    256    0        0 eth0
fe80::/64                     ::        UA    256    0        0 eth1
fe80::/64                     ::        UA    256    0        0 eth2
fec0::/128                    ::        U     0      0        0 lo
fec0::1/128                   ::        U     0      38       0 lo
fec0::2/128                   fec0::2   UAC   0      2055    2049 eth1
fec0::/64                     ::        UA    256    1        0 eth1
fec0:0:0:1::/128              ::        U     0      0        0 lo
fec0:0:0:1::10/128            ::        U     0      0        0 lo
fec0:0:0:1::/64               ::        UA    256    0        0 eth2
fec0:0:0:2::/64               fec0::2   UG    1      15       0 eth1
ff00::/8                      ::        UA    256    0        0 eth0
ff00::/8                      ::        UA    256    0        0 eth1
ff00::/8                      ::        UA    256    0        0 eth2

Thanks,
Tom

Re: IPSec: Policy dst bundles exhausting storage

[David S. Miller]

   From: "Tom Lendacky" <toml@us.ibm.com>
   Date: Tue, 17 Jun 2003 11:26:55 -0500
   
   In prep for coding this up I deleted the "x->u.rt.fl = *fl;" line
   and then I noticed that (at least in my configuration) that the
   rt6->rt6i_src address and prefix length in the xfrm_dst structure
   are always zero.

Well, of course.  There is nothing initializing this.

You have to replace the x->u.rt.fl = *fl line with
assignments further down to rt6i_src and friends.
Something like:

	x->u.rt6.rt6i_src = rt0->rt6i_src;

etc. etc.

I don't understand where you expected these assignments
to be made.  This is where the objects get constructed, so
if it isn't being set here, it is being set nowhere :-)

Re: IPSec: Policy dst bundles exhausting storage

[Tom Lendacky]

   Well, of course.  There is nothing initializing this.

   You have to replace the x->u.rt.fl = *fl line with
   assignments further down to rt6i_src and friends.
   Something like:

             x->u.rt6.rt6i_src = rt0->rt6i_src;

   etc. etc.

   I don't understand where you expected these assignments
   to be made.  This is where the objects get constructed, so
   if it isn't being set here, it is being set nowhere :-)

Ok, my explanation could have been better.  In __xfrm6_bundle_create,
rt0->rt6i_src address and prefix length are zero (as well as rt->rt6i_src)
and so in __xfrm6_find_bundle the values in the xfrm_dst structure were
then zero.

So doing a tunnel mode ping from fec0::1 to fec0:0:0:2::11 in my
configuration, the following values exist in __xfrm6_bundle_create:

  rt0->rt6i_src.addr = 0000:0000:0000:0000:0000:0000:0000:0000
  rt0->rt6i_src.plen = 0
  rt0->rt6i_dst.addr = fec0:0000:0000:0002:0000:0000:0000:0000
  rt0->rt6i_dst.plen = 64

  rt->rt6i_src.addr  = 0000:0000:0000:0000:0000:0000:0000:0000
  rt->rt6i_src.plen  = 0
  rt->rt6i_dst.addr  = fec0:0000:0000:0000:0000:0000:0000:0002
  rt->rt6i_dst.plen  = 128

Sorry for the confusion.

Thanks,
Tom

Re: IPSec: Policy dst bundles exhausting storage

[David S. Miller]

   From: "Tom Lendacky" <toml@us.ibm.com>
   Date: Tue, 17 Jun 2003 13:38:15 -0500

   In __xfrm6_bundle_create, rt0->rt6i_src address and prefix length
   are zero (as well as rt->rt6i_src)

That's perfectly fine, a 0-length prefix will cause a matche
on all addresses.

Re: IPSec: Policy dst bundles exhausting storage

[David S. Miller]

   From: Tom Lendacky <toml@us.ibm.com>
   Date: 17 Jun 2003 14:57:04 -0500

      That's perfectly fine, a 0-length prefix will cause a matche
      on all addresses.
   
   Ok, I just wanted to verify that.  Here's a patch for your review.

Looks like it would work.

   I call ipv6_addr_prefix on both of the rt6i addresses just in case they
   aren't stored in prefix form at any point now or in the future.

I think this is a bit overkill, can you redo this patch without this?

If we un-prefix'ify ipv6 addresses in the routing entries, we're going
to have to go over the whole tree and audit this kind of stuff
anyways.

Thanks.

Re: IPSec: Policy dst bundles exhausting storage

[Tom Lendacky]

   That's perfectly fine, a 0-length prefix will cause a matche
   on all addresses.

Ok, I just wanted to verify that.  Here's a patch for your review.  I
call ipv6_addr_prefix on both of the rt6i addresses just in case they
aren't stored in prefix form at any point now or in the future.

Thanks,
Tom

diff -ur linux-2.5.71-orig/net/ipv6/xfrm6_policy.c linux-2.5.71-new/net/ipv6/xfrm6_policy.c
--- linux-2.5.71-orig/net/ipv6/xfrm6_policy.c	2003-06-14 14:18:02.000000000 -0500
+++ linux-2.5.71-new/net/ipv6/xfrm6_policy.c	2003-06-17 14:44:52.000000000 -0500
@@ -60,8 +60,23 @@
 	read_lock_bh(&policy->lock);
 	for (dst = policy->bundles; dst; dst = dst->next) {
 		struct xfrm_dst *xdst = (struct xfrm_dst*)dst;
-		if (!ipv6_addr_cmp(&xdst->u.rt6.rt6i_dst.addr, &fl->fl6_dst) &&
-		    !ipv6_addr_cmp(&xdst->u.rt6.rt6i_src.addr, &fl->fl6_src) &&
+		struct in6_addr rt_dst_prefix, fl_dst_prefix,
+				rt_src_prefix, fl_src_prefix;
+
+		ipv6_addr_prefix(&rt_dst_prefix,
+				 &xdst->u.rt6.rt6i_dst.addr,
+				 xdst->u.rt6.rt6i_dst.plen);
+		ipv6_addr_prefix(&fl_dst_prefix,
+				 &fl->fl6_dst,
+				 xdst->u.rt6.rt6i_dst.plen);
+		ipv6_addr_prefix(&rt_src_prefix,
+				 &xdst->u.rt6.rt6i_src.addr,
+				 xdst->u.rt6.rt6i_src.plen);
+		ipv6_addr_prefix(&fl_src_prefix,
+				 &fl->fl6_src,
+				 xdst->u.rt6.rt6i_src.plen);
+		if (!ipv6_addr_cmp(&rt_dst_prefix, &fl_dst_prefix) &&
+		    !ipv6_addr_cmp(&rt_src_prefix, &fl_src_prefix) &&
 		    __xfrm6_bundle_ok(xdst, fl)) {
 			dst_clone(dst);
 			break;
@@ -133,7 +148,6 @@
 	dst_prev->child = &rt->u.dst;
 	for (dst_prev = dst; dst_prev != &rt->u.dst; dst_prev = dst_prev->child) {
 		struct xfrm_dst *x = (struct xfrm_dst*)dst_prev;
-		x->u.rt.fl = *fl;
 
 		dst_prev->dev = rt->u.dst.dev;
 		if (rt->u.dst.dev)
@@ -157,6 +171,8 @@
 		x->u.rt6.rt6i_node     = rt0->rt6i_node;
 		x->u.rt6.rt6i_gateway  = rt0->rt6i_gateway;
 		memcpy(&x->u.rt6.rt6i_gateway, &rt0->rt6i_gateway, sizeof(x->u.rt6.rt6i_gateway)); 
+		x->u.rt6.rt6i_dst      = rt0->rt6i_dst;
+		x->u.rt6.rt6i_src      = rt0->rt6i_src;	
 		header_len -= x->u.dst.xfrm->props.header_len;
 		trailer_len -= x->u.dst.xfrm->props.trailer_len;
 	}

Re: IPSec: Policy dst bundles exhausting storage

[Tom Lendacky]

   I think this is a bit overkill, can you redo this patch without this?

No problem...  here's the new patch.

Thanks,
Tom

diff -ur linux-2.5.71-orig/net/ipv6/xfrm6_policy.c linux-2.5.71-new/net/ipv6/xfrm6_policy.c
--- linux-2.5.71-orig/net/ipv6/xfrm6_policy.c	2003-06-14 14:18:02.000000000 -0500
+++ linux-2.5.71-new/net/ipv6/xfrm6_policy.c	2003-06-17 15:34:48.000000000 -0500
@@ -60,8 +60,16 @@
 	read_lock_bh(&policy->lock);
 	for (dst = policy->bundles; dst; dst = dst->next) {
 		struct xfrm_dst *xdst = (struct xfrm_dst*)dst;
-		if (!ipv6_addr_cmp(&xdst->u.rt6.rt6i_dst.addr, &fl->fl6_dst) &&
-		    !ipv6_addr_cmp(&xdst->u.rt6.rt6i_src.addr, &fl->fl6_src) &&
+		struct in6_addr fl_dst_prefix, fl_src_prefix;
+
+		ipv6_addr_prefix(&fl_dst_prefix,
+				 &fl->fl6_dst,
+				 xdst->u.rt6.rt6i_dst.plen);
+		ipv6_addr_prefix(&fl_src_prefix,
+				 &fl->fl6_src,
+				 xdst->u.rt6.rt6i_src.plen);
+		if (!ipv6_addr_cmp(&xdst->u.rt6.rt6i_dst.addr, &fl_dst_prefix) &&
+		    !ipv6_addr_cmp(&xdst->u.rt6.rt6i_src.addr, &fl_src_prefix) &&
 		    __xfrm6_bundle_ok(xdst, fl)) {
 			dst_clone(dst);
 			break;
@@ -133,7 +141,6 @@
 	dst_prev->child = &rt->u.dst;
 	for (dst_prev = dst; dst_prev != &rt->u.dst; dst_prev = dst_prev->child) {
 		struct xfrm_dst *x = (struct xfrm_dst*)dst_prev;
-		x->u.rt.fl = *fl;
 
 		dst_prev->dev = rt->u.dst.dev;
 		if (rt->u.dst.dev)
@@ -157,6 +164,8 @@
 		x->u.rt6.rt6i_node     = rt0->rt6i_node;
 		x->u.rt6.rt6i_gateway  = rt0->rt6i_gateway;
 		memcpy(&x->u.rt6.rt6i_gateway, &rt0->rt6i_gateway, sizeof(x->u.rt6.rt6i_gateway)); 
+		x->u.rt6.rt6i_dst      = rt0->rt6i_dst;
+		x->u.rt6.rt6i_src      = rt0->rt6i_src;	
 		header_len -= x->u.dst.xfrm->props.header_len;
 		trailer_len -= x->u.dst.xfrm->props.trailer_len;
 	}

Re: IPSec: Policy dst bundles exhausting storage

[David S. Miller]

   From: Tom Lendacky <toml@us.ibm.com>
   Date: 17 Jun 2003 15:40:12 -0500

      I think this is a bit overkill, can you redo this patch without this?
   
   No problem...  here's the new patch.
   
Applied, thanks Tom.