* 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).