Re: [PATCH][IPSEC][4/7] inter address family ipsec tunnel
From: Kazunori MIYAZAWA <[EMAIL PROTECTED]> Date: Thu, 07 Dec 2006 20:23:48 +0900 > David Miller wrote: > > From: David Miller <[EMAIL PROTECTED]> > > Date: Wed, 06 Dec 2006 23:37:49 -0800 (PST) > > > >> From: Kazunori MIYAZAWA <[EMAIL PROTECTED]> > >> Date: Wed, 6 Dec 2006 20:35:37 +0900 > >> > >>> BTW, I have a question about descrementing the reference count of > >>> rt->peer. The reference cound in normal "dst" structure is > >>> decremented by calling inet_putpeer from ipv4_dst_destroy. But > >>> xfrm4_dst_destroy does not call inet_putpeer. Where do we decrement > >>> the count? Should xfrm4_dst_destroy do that? > >> Indeed, it is a real leak. And yes, I believe that xfrm4_dst_destroy() > >> should release it. I will make this fix, thank you. > > > > For reference, this is the fix I checked in. > > > > Thanks again for spotting this problem. > > Thank you for making the patch. > Will it be merged to 2.6.19.x? Yes, I submitted it to -stable last night and Chris W. just queued it up for the -stable branches (even 2.6.18.x etc.) - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][IPSEC][4/7] inter address family ipsec tunnel
David Miller wrote: From: David Miller <[EMAIL PROTECTED]> Date: Wed, 06 Dec 2006 23:37:49 -0800 (PST) From: Kazunori MIYAZAWA <[EMAIL PROTECTED]> Date: Wed, 6 Dec 2006 20:35:37 +0900 BTW, I have a question about descrementing the reference count of rt->peer. The reference cound in normal "dst" structure is decremented by calling inet_putpeer from ipv4_dst_destroy. But xfrm4_dst_destroy does not call inet_putpeer. Where do we decrement the count? Should xfrm4_dst_destroy do that? Indeed, it is a real leak. And yes, I believe that xfrm4_dst_destroy() should release it. I will make this fix, thank you. For reference, this is the fix I checked in. Thanks again for spotting this problem. Thank you for making the patch. Will it be merged to 2.6.19.x? I suppose your patch will need to add an address family check for this too. Actually... I think address family check is needed for 'idev' release in xfrm4_dst_destroy() too, if you agree please also add that fix to your patch. Yes, I will add address family check for your patch. Umm, I guess we don't need address family check because xdst is allocated with xfrm4_dst_ops and the family of rt0 is always equal to the original address family. They are always AF_INET in this case. I will try to fix the unresolved symbol issue. It is very complicated, using IPV6 route in xfrm4 code, because now all "X->u.rt" references need to be audited. commit 26db167702756d0022f8ea5f1f30cad3018cfe31 Author: David S. Miller <[EMAIL PROTECTED]> Date: Wed Dec 6 23:45:15 2006 -0800 [IPSEC]: Fix inetpeer leak in ipv4 xfrm dst entries. We grab a reference to the route's inetpeer entry but forget to release it in xfrm4_dst_destroy(). Bug discovered by Kazunori MIYAZAWA <[EMAIL PROTECTED]> Signed-off-by: David S. Miller <[EMAIL PROTECTED]> diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c index d4107bb..fb9f69c 100644 --- a/net/ipv4/xfrm4_policy.c +++ b/net/ipv4/xfrm4_policy.c @@ -274,6 +274,8 @@ static void xfrm4_dst_destroy(struct dst if (likely(xdst->u.rt.idev)) in_dev_put(xdst->u.rt.idev); + if (likely(xdst->u.rt.peer)) + inet_putpeer(xdst->u.rt.peer); xfrm_dst_destroy(xdst); } - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html -- Kazunori Miyazawa - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][IPSEC][4/7] inter address family ipsec tunnel
From: David Miller <[EMAIL PROTECTED]> Date: Wed, 06 Dec 2006 23:37:49 -0800 (PST) > From: Kazunori MIYAZAWA <[EMAIL PROTECTED]> > Date: Wed, 6 Dec 2006 20:35:37 +0900 > > > BTW, I have a question about descrementing the reference count of > > rt->peer. The reference cound in normal "dst" structure is > > decremented by calling inet_putpeer from ipv4_dst_destroy. But > > xfrm4_dst_destroy does not call inet_putpeer. Where do we decrement > > the count? Should xfrm4_dst_destroy do that? > > Indeed, it is a real leak. And yes, I believe that xfrm4_dst_destroy() > should release it. I will make this fix, thank you. For reference, this is the fix I checked in. Thanks again for spotting this problem. I suppose your patch will need to add an address family check for this too. Actually... I think address family check is needed for 'idev' release in xfrm4_dst_destroy() too, if you agree please also add that fix to your patch. It is very complicated, using IPV6 route in xfrm4 code, because now all "X->u.rt" references need to be audited. commit 26db167702756d0022f8ea5f1f30cad3018cfe31 Author: David S. Miller <[EMAIL PROTECTED]> Date: Wed Dec 6 23:45:15 2006 -0800 [IPSEC]: Fix inetpeer leak in ipv4 xfrm dst entries. We grab a reference to the route's inetpeer entry but forget to release it in xfrm4_dst_destroy(). Bug discovered by Kazunori MIYAZAWA <[EMAIL PROTECTED]> Signed-off-by: David S. Miller <[EMAIL PROTECTED]> diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c index d4107bb..fb9f69c 100644 --- a/net/ipv4/xfrm4_policy.c +++ b/net/ipv4/xfrm4_policy.c @@ -274,6 +274,8 @@ static void xfrm4_dst_destroy(struct dst if (likely(xdst->u.rt.idev)) in_dev_put(xdst->u.rt.idev); + if (likely(xdst->u.rt.peer)) + inet_putpeer(xdst->u.rt.peer); xfrm_dst_destroy(xdst); } - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][IPSEC][4/7] inter address family ipsec tunnel
From: David Miller <[EMAIL PROTECTED]> Date: Wed, 06 Dec 2006 23:37:49 -0800 (PST) > From: Kazunori MIYAZAWA <[EMAIL PROTECTED]> > Date: Wed, 6 Dec 2006 20:35:37 +0900 > > > Sorry, I mixed up changes in the patches. I described that [4/7] will add > > "IPv6 over IPv4 IPsec tunnel". However I send a patch for "IPv4 over IPv6" > > as a reply because it includes the discussing item. > > So this patch adds IPv4 over IPv6 IPsec tunnel. It's complicated. > > > > I deleted subustituting NULL for rt->peer and moved atomic_inc stuff > > under the "if" section. > > I have applied this patch, thanks for fixing it up. Actually, I have to revert this change again, it still has a problem. Sorry :-/ I very much feared the following kind of obstacle with these changes. Specifically, references to modular IPV6 code from statically compiled IPV4 code. Which produces the following build error: net/built-in.o: In function `__xfrm4_bundle_create':xfrm4_policy.c:(.text+0x59a88): undefined reference to `xfrm6_output' :xfrm4_policy.c:(.text+0x59ac0): undefined reference to `xfrm6_output' Please test with IPV6 built modular in the future, thank you. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][IPSEC][4/7] inter address family ipsec tunnel
From: Kazunori MIYAZAWA <[EMAIL PROTECTED]> Date: Wed, 6 Dec 2006 20:35:37 +0900 > Sorry, I mixed up changes in the patches. I described that [4/7] will add > "IPv6 over IPv4 IPsec tunnel". However I send a patch for "IPv4 over IPv6" > as a reply because it includes the discussing item. > So this patch adds IPv4 over IPv6 IPsec tunnel. It's complicated. > > I deleted subustituting NULL for rt->peer and moved atomic_inc stuff > under the "if" section. I have applied this patch, thanks for fixing it up. > BTW, I have a question about descrementing the reference count of > rt->peer. The reference cound in normal "dst" structure is > decremented by calling inet_putpeer from ipv4_dst_destroy. But > xfrm4_dst_destroy does not call inet_putpeer. Where do we decrement > the count? Should xfrm4_dst_destroy do that? Indeed, it is a real leak. And yes, I believe that xfrm4_dst_destroy() should release it. I will make this fix, thank you. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][IPSEC][4/7] inter address family ipsec tunnel
On Fri, 01 Dec 2006 13:41:39 +0900
Kazunori MIYAZAWA <[EMAIL PROTECTED]> wrote:
> David Miller wrote:
> > From: Kazunori MIYAZAWA <[EMAIL PROTECTED]>
> > Date: Fri, 24 Nov 2006 14:38:39 +0900
> >
> > What is going on here?
> >
> >> + /* Without this, the atomic inc below segfaults */
> >> + if (encap_family == AF_INET6) {
> >> + rt->peer = NULL;
> >> + rt_bind_peer(rt,1);
> >> + }
> > ...
> >> - dst_prev->output= xfrm4_output;
> >> + if (dst_prev->xfrm->props.family == AF_INET)
> >> + dst_prev->output = xfrm4_output;
> >> +#if defined(CONFIG_IPV6) || defined (CONFIG_IPV6_MODULE)
> >> + else
> >> + dst_prev->output = xfrm6_output;
> >> +#endif
> >>if (rt->peer)
> >>atomic_inc(&rt->peer->refcnt);
> >
> > If it's non-NULL and you get a segfault for atomic_inc() that
> > means there is garbage here, and it seems that if you're
> > setting it to NULL explicitly then it's just a workaround
> > for whatever problem is causing it to be non-NULL to begin
> > with.
> >
> > What is putting a non-valid pointer value there? Is this an IPV6 or
> > IPSEC dst route by chance? If so, that makes this change really
> > wrong, and we are corrupting the route by running rt_bind_peer() on
> > it. rt_bind_peer() is only valid on ipv4 route entries.
>
> Thank you for your good catch.
> I think atomic_inc must be done in case of props.family == AF_INET.
> And we probably should manage reference count of the device in case of
> AF_INET6.
>
> Anyway I'll check and fix it.
>
> Thank you.
>
Hello David,
Sorry, I mixed up changes in the patches. I described that [4/7] will add
"IPv6 over IPv4 IPsec tunnel". However I send a patch for "IPv4 over IPv6"
as a reply because it includes the discussing item.
So this patch adds IPv4 over IPv6 IPsec tunnel. It's complicated.
I deleted subustituting NULL for rt->peer and moved atomic_inc stuff
under the "if" section.
BTW, I have a question about descrementing the reference count of rt->peer.
The reference cound in normal "dst" structure is decremented by calling
inet_putpeer from ipv4_dst_destroy. But xfrm4_dst_destroy does not call
inet_putpeer.
Where do we decrement the count? Should xfrm4_dst_destroy do that?
Signed-off-by: Miika Komu <[EMAIL PROTECTED]>
Signed-off-by: Diego Beltrami <[EMAIL PROTECTED]>
Signed-off-by: Kazunori Miyazawa <[EMAIL PROTECTED]>
---
net/ipv4/xfrm4_policy.c | 68 --
net/ipv6/xfrm6_mode_tunnel.c | 42 --
2 files changed, 83 insertions(+), 27 deletions(-)
diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index d4107bb..37b14e8 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -72,17 +72,20 @@ __xfrm4_bundle_create(struct xfrm_policy
struct dst_entry *dst, *dst_prev;
struct rtable *rt0 = (struct rtable*)(*dst_p);
struct rtable *rt = rt0;
- __be32 remote = fl->fl4_dst;
- __be32 local = fl->fl4_src;
struct flowi fl_tunnel = {
.nl_u = {
.ip4_u = {
- .saddr = local,
- .daddr = remote,
+ .saddr = fl->fl4_src,
+ .daddr = fl->fl4_dst,
.tos = fl->fl4_tos
}
}
};
+ union {
+ struct in6_addr *in6;
+ struct in_addr *in;
+ } remote, local;
+ unsigned short encap_family = 0;
int i;
int err;
int header_len = 0;
@@ -94,7 +97,6 @@ __xfrm4_bundle_create(struct xfrm_policy
for (i = 0; i < nx; i++) {
struct dst_entry *dst1 = dst_alloc(&xfrm4_dst_ops);
struct xfrm_dst *xdst;
- int tunnel = 0;
if (unlikely(dst1 == NULL)) {
err = -ENOBUFS;
@@ -116,19 +118,45 @@ __xfrm4_bundle_create(struct xfrm_policy
dst1->next = dst_prev;
dst_prev = dst1;
- if (xfrm[i]->props.mode != XFRM_MODE_TRANSPORT) {
- remote = xfrm[i]->id.daddr.a4;
- local = xfrm[i]->props.saddr.a4;
- tunnel = 1;
+
+ if (xfrm[i]->props.mode == XFRM_MODE_TUNNEL) {
+ encap_family = xfrm[i]->props.family;
+
+ switch(encap_family) {
+ case AF_INET:
+ remote.in = (struct in_addr*)&xfrm[i]->id.daddr;
+ local.in = (struct
in_addr*)&xfrm[i]->props.saddr;
+ break;
+#if defined(CONFIG_IPV6) || defined (CONFIG_IPV6_MODULE)
+ case AF_INET6:
+ remo
Re: [PATCH][IPSEC][4/7] inter address family ipsec tunnel
David Miller wrote:
From: Kazunori MIYAZAWA <[EMAIL PROTECTED]>
Date: Fri, 24 Nov 2006 14:38:39 +0900
What is going on here?
+ /* Without this, the atomic inc below segfaults */
+ if (encap_family == AF_INET6) {
+ rt->peer = NULL;
+ rt_bind_peer(rt,1);
+ }
...
- dst_prev->output = xfrm4_output;
+ if (dst_prev->xfrm->props.family == AF_INET)
+ dst_prev->output = xfrm4_output;
+#if defined(CONFIG_IPV6) || defined (CONFIG_IPV6_MODULE)
+ else
+ dst_prev->output = xfrm6_output;
+#endif
if (rt->peer)
atomic_inc(&rt->peer->refcnt);
If it's non-NULL and you get a segfault for atomic_inc() that
means there is garbage here, and it seems that if you're
setting it to NULL explicitly then it's just a workaround
for whatever problem is causing it to be non-NULL to begin
with.
What is putting a non-valid pointer value there? Is this an IPV6 or
IPSEC dst route by chance? If so, that makes this change really
wrong, and we are corrupting the route by running rt_bind_peer() on
it. rt_bind_peer() is only valid on ipv4 route entries.
Thank you for your good catch.
I think atomic_inc must be done in case of props.family == AF_INET.
And we probably should manage reference count of the device in case of
AF_INET6.
Anyway I'll check and fix it.
Thank you.
--
Kazunori Miyazawa
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][IPSEC][4/7] inter address family ipsec tunnel
From: Kazunori MIYAZAWA <[EMAIL PROTECTED]>
Date: Fri, 24 Nov 2006 14:38:39 +0900
What is going on here?
> + /* Without this, the atomic inc below segfaults */
> + if (encap_family == AF_INET6) {
> + rt->peer = NULL;
> + rt_bind_peer(rt,1);
> + }
...
> - dst_prev->output= xfrm4_output;
> + if (dst_prev->xfrm->props.family == AF_INET)
> + dst_prev->output = xfrm4_output;
> +#if defined(CONFIG_IPV6) || defined (CONFIG_IPV6_MODULE)
> + else
> + dst_prev->output = xfrm6_output;
> +#endif
> if (rt->peer)
> atomic_inc(&rt->peer->refcnt);
If it's non-NULL and you get a segfault for atomic_inc() that
means there is garbage here, and it seems that if you're
setting it to NULL explicitly then it's just a workaround
for whatever problem is causing it to be non-NULL to begin
with.
What is putting a non-valid pointer value there? Is this an IPV6 or
IPSEC dst route by chance? If so, that makes this change really
wrong, and we are corrupting the route by running rt_bind_peer() on
it. rt_bind_peer() is only valid on ipv4 route entries.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
