* Re: IPSec Oops when deleting an ip address
[not found] <20040510134958.13691.qmail@mason.oriente.labs.it>
@ 2004-05-21 13:19 ` Herbert Xu
2004-05-21 21:43 ` David S. Miller
0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2004-05-21 13:19 UTC (permalink / raw)
To: Michele Bergonzoni; +Cc: David S. Miller, netdev, linux-net
[-- Attachment #1: Type: text/plain, Size: 654 bytes --]
On Mon, May 10, 2004 at 12:51:23PM +0000, Michele Bergonzoni wrote:
>
> CPU: 0
> EIP: 0060:[<c030415a>] Not tainted
> EFLAGS: 00010202 (2.6.5)
> EIP is at xfrm_state_gc_destroy+0x1a/0xc0
Sorry, that's probably my fault. I don't know what I was thinking, but
doing a mod_timer on a live state without holding a lock or for that
matter not even checking whether the state is dead is definitely a bad
idea.
This patch should fix it.
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
[-- Attachment #2: p --]
[-- Type: text/plain, Size: 847 bytes --]
Index: net/xfrm/xfrm_state.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.5/net/xfrm/xfrm_state.c,v
retrieving revision 1.12
diff -u -r1.12 xfrm_state.c
--- a/net/xfrm/xfrm_state.c 29 Nov 2003 06:48:37 -0000 1.12
+++ b/net/xfrm/xfrm_state.c 21 May 2004 13:13:34 -0000
@@ -489,15 +489,16 @@
memcpy(x1->encap, x->encap, sizeof(*x1->encap));
memcpy(&x1->lft, &x->lft, sizeof(x1->lft));
x1->km.dying = 0;
+
+ if (!mod_timer(&x1->timer, jiffies + HZ))
+ xfrm_state_hold(x1);
+ if (x1->curlft.use_time)
+ xfrm_state_check_expire(x1);
+
err = 0;
}
spin_unlock_bh(&x1->lock);
- if (!mod_timer(&x1->timer, jiffies + HZ))
- xfrm_state_hold(x1);
- if (x1->curlft.use_time)
- xfrm_state_check_expire(x1);
-
xfrm_state_put(x1);
return err;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: IPSec Oops when deleting an ip address
2004-05-21 13:19 ` IPSec Oops when deleting an ip address Herbert Xu
@ 2004-05-21 21:43 ` David S. Miller
2004-05-24 11:47 ` Herbert Xu
0 siblings, 1 reply; 6+ messages in thread
From: David S. Miller @ 2004-05-21 21:43 UTC (permalink / raw)
To: Herbert Xu; +Cc: bergonz, netdev, linux-net
On Fri, 21 May 2004 23:19:50 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:
> doing a mod_timer on a live state without holding a lock or for that
> matter not even checking whether the state is dead is definitely a bad
> idea
Applied, thanks Herbert.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: IPSec Oops when deleting an ip address
2004-05-21 21:43 ` David S. Miller
@ 2004-05-24 11:47 ` Herbert Xu
2004-05-24 17:14 ` David S. Miller
2004-05-25 11:52 ` Herbert Xu
0 siblings, 2 replies; 6+ messages in thread
From: Herbert Xu @ 2004-05-24 11:47 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-net
[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]
On Fri, May 21, 2004 at 02:43:46PM -0700, David S. Miller wrote:
> On Fri, 21 May 2004 23:19:50 +1000
> Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> > doing a mod_timer on a live state without holding a lock or for that
> > matter not even checking whether the state is dead is definitely a bad
> > idea
>
> Applied, thanks Herbert.
Looks like I was too hasty in blaming myself :) Although my patch does
fix a real bug, it cannot have been responsible for the crash that the OP
reported. The reason is that the state timer always keeps a reference to
the state so even if it is incorrectly re-added the reference will prevent
the crash.
Hence the problem is still a bug in the ref counting. I think I've found
the real culprit now. __xfrm?_find_acq() is missing an xfrm_state_hold
on the create path. This also explains why I never see it myself since
Openswan never creates states through that code-path.
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
[-- Attachment #2: p --]
[-- Type: text/plain, Size: 1273 bytes --]
===== xfrm4_state.c 1.6 vs edited =====
--- 1.6/net/ipv4/xfrm4_state.c 2003-07-21 21:49:43 +10:00
+++ edited/xfrm4_state.c 2004-05-24 21:41:29 +10:00
@@ -83,9 +83,7 @@
break;
}
}
- if (x0) {
- xfrm_state_hold(x0);
- } else if (create && (x0 = xfrm_state_alloc()) != NULL) {
+ if (!x0 && create && (x0 = xfrm_state_alloc()) != NULL) {
x0->sel.daddr.a4 = daddr->a4;
x0->sel.saddr.a4 = saddr->a4;
x0->sel.prefixlen_d = 32;
@@ -105,6 +103,8 @@
list_add_tail(&x0->bydst, xfrm4_state_afinfo.state_bydst+h);
wake_up(&km_waitq);
}
+ if (x0)
+ xfrm_state_hold(x0);
return x0;
}
===== xfrm6_state.c 1.8 vs edited =====
--- 1.8/net/ipv6/xfrm6_state.c 2003-07-21 21:49:43 +10:00
+++ edited/xfrm6_state.c 2004-05-24 21:44:06 +10:00
@@ -90,9 +90,7 @@
break;
}
}
- if (x0) {
- xfrm_state_hold(x0);
- } else if (create && (x0 = xfrm_state_alloc()) != NULL) {
+ if (!x0 && create && (x0 = xfrm_state_alloc()) != NULL) {
ipv6_addr_copy((struct in6_addr *)x0->sel.daddr.a6,
(struct in6_addr *)daddr);
ipv6_addr_copy((struct in6_addr *)x0->sel.saddr.a6,
@@ -115,6 +113,8 @@
list_add_tail(&x0->bydst, xfrm6_state_afinfo.state_bydst+h);
wake_up(&km_waitq);
}
+ if (x0)
+ xfrm_state_hold(x0);
return x0;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: IPSec Oops when deleting an ip address
2004-05-24 11:47 ` Herbert Xu
@ 2004-05-24 17:14 ` David S. Miller
2004-05-25 11:52 ` Herbert Xu
1 sibling, 0 replies; 6+ messages in thread
From: David S. Miller @ 2004-05-24 17:14 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, linux-net
On Mon, 24 May 2004 21:47:51 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:
> Hence the problem is still a bug in the ref counting. I think I've found
> the real culprit now. __xfrm?_find_acq() is missing an xfrm_state_hold
> on the create path. This also explains why I never see it myself since
> Openswan never creates states through that code-path.
Applied, thanks Herbert.
How the heck are you generating your patches? Because the file paths
look like this in the patch:
--- 1.6/net/ipv4/xfrm4_state.c 2003-07-21 21:49:43 +10:00
+++ edited/xfrm4_state.c 2004-05-24 21:41:29 +10:00
My auto-patch-application scripts explode since the path
depth of the file is different in the second line there.
How come it doesn't look like:
--- 1.6/net/ipv4/xfrm4_state.c 2003-07-21 21:49:43 +10:00
+++ edited/net/ipv4/xfrm4_state.c 2004-05-24 21:41:29 +10:00
So that I can throw "-p1" to tools like diffstat and get sane
output?
Your patches never did this in the past :-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: IPSec Oops when deleting an ip address
2004-05-24 11:47 ` Herbert Xu
2004-05-24 17:14 ` David S. Miller
@ 2004-05-25 11:52 ` Herbert Xu
2004-05-25 18:01 ` David S. Miller
1 sibling, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2004-05-25 11:52 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-net
[-- Attachment #1: Type: text/plain, Size: 837 bytes --]
On Mon, May 24, 2004 at 09:47:51PM +1000, herbert wrote:
>
> Hence the problem is still a bug in the ref counting. I think I've found
> the real culprit now. __xfrm?_find_acq() is missing an xfrm_state_hold
> on the create path. This also explains why I never see it myself since
> Openswan never creates states through that code-path.
The same bug exists in xfrm_state_find. This is actually used by
Openswan. However, the larval state never actually matures with
Openswan so it only ever gets deleted by the timer which means that
the timer crash can't happen :) It becomes a (possible) memory leak
instead.
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
[-- Attachment #2: p --]
[-- Type: text/plain, Size: 789 bytes --]
===== net/xfrm/xfrm_state.c 1.39 vs edited =====
--- 1.39/net/xfrm/xfrm_state.c 2004-05-22 17:32:10 +10:00
+++ edited/net/xfrm/xfrm_state.c 2004-05-25 21:42:43 +10:00
@@ -331,14 +331,8 @@
}
}
- if (best) {
- xfrm_state_hold(best);
- spin_unlock_bh(&xfrm_state_lock);
- return best;
- }
-
- x = NULL;
- if (!error && !acquire_in_progress &&
+ x = best;
+ if (!x && !error && !acquire_in_progress &&
((x = xfrm_state_alloc()) != NULL)) {
/* Initialize temporary selector matching only
* to current session. */
@@ -363,10 +357,12 @@
error = 1;
}
}
- spin_unlock_bh(&xfrm_state_lock);
- if (!x)
+ if (x)
+ xfrm_state_hold(x);
+ else
*err = acquire_in_progress ? -EAGAIN :
(error ? -ESRCH : -ENOMEM);
+ spin_unlock_bh(&xfrm_state_lock);
return x;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: IPSec Oops when deleting an ip address
2004-05-25 11:52 ` Herbert Xu
@ 2004-05-25 18:01 ` David S. Miller
0 siblings, 0 replies; 6+ messages in thread
From: David S. Miller @ 2004-05-25 18:01 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, linux-net
On Tue, 25 May 2004 21:52:20 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:
> The same bug exists in xfrm_state_find. This is actually used by
> Openswan. However, the larval state never actually matures with
> Openswan so it only ever gets deleted by the timer which means that
> the timer crash can't happen :) It becomes a (possible) memory leak
> instead.
Looks good, applied.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-05-25 18:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20040510134958.13691.qmail@mason.oriente.labs.it>
2004-05-21 13:19 ` IPSec Oops when deleting an ip address Herbert Xu
2004-05-21 21:43 ` David S. Miller
2004-05-24 11:47 ` Herbert Xu
2004-05-24 17:14 ` David S. Miller
2004-05-25 11:52 ` Herbert Xu
2004-05-25 18:01 ` David S. Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).