* Re: [PATCH] Bugs in xfrm @ 2004-01-10 18:33 Krishna Kumar 2004-01-10 20:11 ` David S. Miller 0 siblings, 1 reply; 16+ messages in thread From: Krishna Kumar @ 2004-01-10 18:33 UTC (permalink / raw) To: David S. Miller; +Cc: krkumar, netdev [-- Attachment #1: Type: text/plain, Size: 840 bytes --] > Explain to me how that won't loop forever if given a non-NULL dst? Oops, forgot the dst = next in the loop :-) > Next, this dst_bundle_free() thing is totally not needed as far as I can > tell. When dst_free() is made, the top-level of the bundle's dst gets added > to the garbage collection list, the garbage collection properly walks the > children to process the whole bundle. The garbage collector (called via __dst_free) takes dst_garbage_list and goes through the dst->next pointer. But dst_destroy() seems to destroy stuff on the dst->child list (I missed the part that the first dst has a refcnt of zero and all others on child have refcnt of 1). So this is not needed. > Please redo this patch and please test it this time :) Are the other "bugs" correct :-) Or should I send separate patches this time ? thanks, - KK [-- Attachment #2: Type: text/html, Size: 1338 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Bugs in xfrm 2004-01-10 18:33 [PATCH] Bugs in xfrm Krishna Kumar @ 2004-01-10 20:11 ` David S. Miller 2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar 0 siblings, 1 reply; 16+ messages in thread From: David S. Miller @ 2004-01-10 20:11 UTC (permalink / raw) To: Krishna Kumar; +Cc: krkumar, netdev On Sat, 10 Jan 2004 10:33:22 -0800 Krishna Kumar <kumarkr@us.ibm.com> wrote: > Or should I send separate patches this time ? I think this would be a good idea. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state 2004-01-10 20:11 ` David S. Miller @ 2004-01-13 21:21 ` Krishna Kumar 2004-01-13 21:22 ` [PATCH 2/5] Bad dereference of xfrm_state in pf_key Krishna Kumar ` (4 more replies) 0 siblings, 5 replies; 16+ messages in thread From: Krishna Kumar @ 2004-01-13 21:21 UTC (permalink / raw) To: David S. Miller; +Cc: Krishna Kumar, netdev > > Or should I send separate patches this time ? > > I think this would be a good idea. ipcomp_tunnel_create doesn't set x->km.state to XFRM_STATE_DEAD. This can lead to the BUG_TRAP in __xfrm_state_destroy when xfrm_state_put() finds this is the last reference. This was reported as one of the symptoms of [Bug 1754] some time back. thanks, - KK diff -ruN linux-2.6.0-rc2-bk6.org/net/ipv4/ipcomp.c linux-2.6.0-rc2-bk6/net/ipv4/ipcomp.c --- linux-2.6.0-rc2-bk6.org/net/ipv4/ipcomp.c 2004-01-05 13:43:50.000000000 -0800 +++ linux-2.6.0-rc2-bk6/net/ipv4/ipcomp.c 2004-01-09 13:00:22.000000000 -0800 @@ -294,6 +294,7 @@ return t; error: + t->km.state = XFRM_STATE_DEAD; xfrm_state_put(t); t = NULL; goto out; ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/5] Bad dereference of xfrm_state in pf_key 2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar @ 2004-01-13 21:22 ` Krishna Kumar 2004-01-14 7:21 ` David S. Miller 2004-01-13 21:23 ` [PATCH 3/5] xfrm_lookup bugs Krishna Kumar ` (3 subsequent siblings) 4 siblings, 1 reply; 16+ messages in thread From: Krishna Kumar @ 2004-01-13 21:22 UTC (permalink / raw) To: David S. Miller; +Cc: netdev In pfkey_get(), the xfrm_state is dereferenced after it is dropped, which could lead to dereferencing freed memory. This can also be done by dropping the reference before the pfkey_broadcast() and in the IS_ERR case. thanks, - KK diff -ruN linux-2.6.0-rc2-bk6.org/net/key/af_key.c linux-2.6.0-rc2-bk6/net/key/af_key.c --- linux-2.6.0-rc2-bk6.org/net/key/af_key.c 2004-01-05 13:45:47.000000000 -0800 +++ linux-2.6.0-rc2-bk6/net/key/af_key.c 2004-01-09 12:41:30.000000000 -0800 @@ -1283,6 +1283,7 @@ static int pfkey_get(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs) { + __u8 proto; struct sk_buff *out_skb; struct sadb_msg *out_hdr; struct xfrm_state *x; @@ -1297,6 +1298,7 @@ return -ESRCH; out_skb = pfkey_xfrm_state2msg(x, 1, 3); + proto = x->id.proto; xfrm_state_put(x); if (IS_ERR(out_skb)) return PTR_ERR(out_skb); @@ -1304,7 +1306,7 @@ out_hdr = (struct sadb_msg *) out_skb->data; out_hdr->sadb_msg_version = hdr->sadb_msg_version; out_hdr->sadb_msg_type = SADB_DUMP; - out_hdr->sadb_msg_satype = pfkey_proto2satype(x->id.proto); + out_hdr->sadb_msg_satype = pfkey_proto2satype(proto); out_hdr->sadb_msg_errno = 0; out_hdr->sadb_msg_reserved = 0; out_hdr->sadb_msg_seq = hdr->sadb_msg_seq; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/5] Bad dereference of xfrm_state in pf_key 2004-01-13 21:22 ` [PATCH 2/5] Bad dereference of xfrm_state in pf_key Krishna Kumar @ 2004-01-14 7:21 ` David S. Miller 0 siblings, 0 replies; 16+ messages in thread From: David S. Miller @ 2004-01-14 7:21 UTC (permalink / raw) To: Krishna Kumar; +Cc: netdev On Tue, 13 Jan 2004 13:22:36 -0800 (PST) Krishna Kumar <krkumar@us.ibm.com> wrote: > In pfkey_get(), the xfrm_state is dereferenced after it is dropped, > which could lead to dereferencing freed memory. This can also be done > by dropping the reference before the pfkey_broadcast() and in the IS_ERR > case. Obviously correct, patch applied thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/5] xfrm_lookup bugs 2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar 2004-01-13 21:22 ` [PATCH 2/5] Bad dereference of xfrm_state in pf_key Krishna Kumar @ 2004-01-13 21:23 ` Krishna Kumar 2004-01-14 7:23 ` David S. Miller 2004-01-13 21:24 ` [PATCH 4/5] xfrm_state_construct doesn't set tunnel state Krishna Kumar ` (2 subsequent siblings) 4 siblings, 1 reply; 16+ messages in thread From: Krishna Kumar @ 2004-01-13 21:23 UTC (permalink / raw) To: David S. Miller; +Cc: netdev In xfrm_lookup, a couple of bugs : - the found or allocated xfrm_states are not passed correctly to xfrm_bundle_create (and to the subsequent frees in case of create failing) if the first xfrm_tmpl_resolve failed and the second one succeeded. - error handling is wrong. thanks, - KK diff -ruN linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_policy.c linux-2.6.0-rc2-bk6/net/xfrm/xfrm_policy.c --- linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_policy.c 2004-01-09 12:42:53.000000000 -0800 +++ linux-2.6.0-rc2-bk6/net/xfrm/xfrm_policy.c 2004-01-12 09:55:26.000000000 -0800 @@ -783,25 +783,27 @@ __set_task_state(tsk, TASK_INTERRUPTIBLE); add_wait_queue(&km_waitq, &wait); - err = xfrm_tmpl_resolve(policy, fl, xfrm, family); - if (err == -EAGAIN) + nx = xfrm_tmpl_resolve(policy, fl, xfrm, family); + if (nx == -EAGAIN) schedule(); __set_task_state(tsk, TASK_RUNNING); remove_wait_queue(&km_waitq, &wait); - if (err == -EAGAIN && signal_pending(current)) { + if (nx == -EAGAIN && signal_pending(current)) { err = -ERESTART; goto error; } - if (err == -EAGAIN || + if (nx == -EAGAIN || genid != atomic_read(&flow_cache_genid)) { xfrm_pol_put(policy); goto restart; } + err = nx; } - if (err) + if (err < 0) goto error; - } else if (nx == 0) { + } + if (nx == 0) { /* Flow passes not transformed. */ xfrm_pol_put(policy); return 0; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] xfrm_lookup bugs 2004-01-13 21:23 ` [PATCH 3/5] xfrm_lookup bugs Krishna Kumar @ 2004-01-14 7:23 ` David S. Miller 0 siblings, 0 replies; 16+ messages in thread From: David S. Miller @ 2004-01-14 7:23 UTC (permalink / raw) To: Krishna Kumar; +Cc: netdev On Tue, 13 Jan 2004 13:23:35 -0800 (PST) Krishna Kumar <krkumar@us.ibm.com> wrote: > In xfrm_lookup, a couple of bugs : > - the found or allocated xfrm_states are not passed correctly to > xfrm_bundle_create (and to the subsequent frees in case of create > failing) if the first xfrm_tmpl_resolve failed and the second one > succeeded. > - error handling is wrong. Looks fine, patch applied thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/5] xfrm_state_construct doesn't set tunnel state 2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar 2004-01-13 21:22 ` [PATCH 2/5] Bad dereference of xfrm_state in pf_key Krishna Kumar 2004-01-13 21:23 ` [PATCH 3/5] xfrm_lookup bugs Krishna Kumar @ 2004-01-13 21:24 ` Krishna Kumar 2004-01-14 7:25 ` David S. Miller 2004-01-13 21:26 ` [PATCH 5/5] schedule() bug in xfrm_lookup() Krishna Kumar 2004-01-14 7:20 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state David S. Miller 4 siblings, 1 reply; 16+ messages in thread From: Krishna Kumar @ 2004-01-13 21:24 UTC (permalink / raw) To: David S. Miller; +Cc: netdev, KK xfrm_state_construct doesn't set x->km.state to XFRM_STATE_DEAD. This can lead to the BUG_TRAP in __xfrm_state_destroy when xfrm_state_put() finds this is the last reference. This was reported as one of the symptoms of [Bug 1754] some time back. thanks, - KK diff -ruN linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_user.c linux-2.6.0-rc2-bk6/net/xfrm/xfrm_user.c --- linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_user.c 2004-01-09 12:57:42.000000000 -0800 +++ linux-2.6.0-rc2-bk6/net/xfrm/xfrm_user.c 2004-01-09 12:59:00.000000000 -0800 @@ -241,6 +241,7 @@ return x; error: + x->km.state = XFRM_STATE_DEAD; xfrm_state_put(x); error_no_put: *errp = err; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/5] xfrm_state_construct doesn't set tunnel state 2004-01-13 21:24 ` [PATCH 4/5] xfrm_state_construct doesn't set tunnel state Krishna Kumar @ 2004-01-14 7:25 ` David S. Miller 0 siblings, 0 replies; 16+ messages in thread From: David S. Miller @ 2004-01-14 7:25 UTC (permalink / raw) To: Krishna Kumar; +Cc: netdev, krkumar On Tue, 13 Jan 2004 13:24:23 -0800 (PST) Krishna Kumar <krkumar@us.ibm.com> wrote: > xfrm_state_construct doesn't set x->km.state to XFRM_STATE_DEAD. This can lead > to the BUG_TRAP in __xfrm_state_destroy when xfrm_state_put() finds this is > the last reference. This was reported as one of the symptoms of [Bug 1754] > some time back. Smells just like the nearly identical case in ipcomp :-) Patch applied, thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/5] schedule() bug in xfrm_lookup() 2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar ` (2 preceding siblings ...) 2004-01-13 21:24 ` [PATCH 4/5] xfrm_state_construct doesn't set tunnel state Krishna Kumar @ 2004-01-13 21:26 ` Krishna Kumar 2004-01-14 7:27 ` David S. Miller 2004-01-14 7:20 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state David S. Miller 4 siblings, 1 reply; 16+ messages in thread From: Krishna Kumar @ 2004-01-13 21:26 UTC (permalink / raw) To: David S. Miller; +Cc: netdev, KK When xfrm_tmpl_resolve fails, schedule() before retrying. Also, cleaned up the set_task_* routines to use set_current_*, etc. This needs to be applied after [PATCH 3/5]. thanks, - KK diff -ruN linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_policy.c linux-2.6.0-rc2-bk6/net/xfrm/xfrm_policy.c --- linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_policy.c 2004-01-09 12:42:53.000000000 -0800 +++ linux-2.6.0-rc2-bk6/net/xfrm/xfrm_policy.c 2004-01-12 09:55:26.000000000 -0800 @@ -775,20 +775,17 @@ if (unlikely(nx<0)) { err = nx; - if (err == -EAGAIN) { - struct task_struct *tsk = current; - DECLARE_WAITQUEUE(wait, tsk); - if (!flags) - goto error; + if (err == -EAGAIN && !flags) { + DECLARE_WAITQUEUE(wait, current); - __set_task_state(tsk, TASK_INTERRUPTIBLE); add_wait_queue(&km_waitq, &wait); - nx = xfrm_tmpl_resolve(policy, fl, xfrm, family); - if (nx == -EAGAIN) - schedule(); - __set_task_state(tsk, TASK_RUNNING); + set_current_state(TASK_INTERRUPTIBLE); + schedule(); + set_current_state(TASK_RUNNING); remove_wait_queue(&km_waitq, &wait); + nx = xfrm_tmpl_resolve(policy, fl, xfrm, family); + if (nx == -EAGAIN && signal_pending(current)) { err = -ERESTART; goto error; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/5] schedule() bug in xfrm_lookup() 2004-01-13 21:26 ` [PATCH 5/5] schedule() bug in xfrm_lookup() Krishna Kumar @ 2004-01-14 7:27 ` David S. Miller 0 siblings, 0 replies; 16+ messages in thread From: David S. Miller @ 2004-01-14 7:27 UTC (permalink / raw) To: Krishna Kumar; +Cc: netdev, krkumar On Tue, 13 Jan 2004 13:26:21 -0800 (PST) Krishna Kumar <krkumar@us.ibm.com> wrote: > When xfrm_tmpl_resolve fails, schedule() before retrying. Also, cleaned > up the set_task_* routines to use set_current_*, etc. Applied, thanks Krishna. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state 2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar ` (3 preceding siblings ...) 2004-01-13 21:26 ` [PATCH 5/5] schedule() bug in xfrm_lookup() Krishna Kumar @ 2004-01-14 7:20 ` David S. Miller 2004-03-09 21:36 ` [PATCH] Trivial kmalloc failure checks Krishna Kumar 4 siblings, 1 reply; 16+ messages in thread From: David S. Miller @ 2004-01-14 7:20 UTC (permalink / raw) To: Krishna Kumar; +Cc: krkumar, netdev On Tue, 13 Jan 2004 13:21:19 -0800 (PST) Krishna Kumar <krkumar@us.ibm.com> wrote: > ipcomp_tunnel_create doesn't set x->km.state to XFRM_STATE_DEAD. This can lead > to the BUG_TRAP in __xfrm_state_destroy when xfrm_state_put() finds this is > the last reference. This was reported as one of the symptoms of [Bug 1754] > some time back. Patch applied, thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] Trivial kmalloc failure checks 2004-01-14 7:20 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state David S. Miller @ 2004-03-09 21:36 ` Krishna Kumar 2004-03-09 23:24 ` David S. Miller 0 siblings, 1 reply; 16+ messages in thread From: Krishna Kumar @ 2004-03-09 21:36 UTC (permalink / raw) To: David S. Miller; +Cc: netdev, krkumar Hi Dave, A couple of checks for memory allocation failures. Thanks, - KK diff -ruN linux-2.6.4-rc2-bk5.org/net/ipv4/esp4.c linux-2.6.4-rc2-bk5/net/ipv4/esp4.c --- linux-2.6.4-rc2-bk5.org/net/ipv4/esp4.c 2004-03-09 13:11:15.000000000 -0800 +++ linux-2.6.4-rc2-bk5/net/ipv4/esp4.c 2004-03-09 13:29:44.000000000 -0800 @@ -518,6 +518,8 @@ esp->conf.padlen = 0; if (esp->conf.ivlen) { esp->conf.ivec = kmalloc(esp->conf.ivlen, GFP_KERNEL); + if (unlikely(esp->conf.ivec == NULL)) + goto error; get_random_bytes(esp->conf.ivec, esp->conf.ivlen); } crypto_cipher_setkey(esp->conf.tfm, esp->conf.key, esp->conf.key_len); diff -ruN linux-2.6.4-rc2-bk5.org/net/ipv4/ip_output.c linux-2.6.4-rc2-bk5/net/ipv4/ip_output.c --- linux-2.6.4-rc2-bk5.org/net/ipv4/ip_output.c 2004-03-09 13:18:50.000000000 -0800 +++ linux-2.6.4-rc2-bk5/net/ipv4/ip_output.c 2004-03-09 13:20:10.000000000 -0800 @@ -761,8 +761,11 @@ */ opt = ipc->opt; if (opt) { - if (inet->cork.opt == NULL) + if (inet->cork.opt == NULL) { inet->cork.opt = kmalloc(sizeof(struct ip_options) + 40, sk->sk_allocation); + if (unlikely(inet->cork.opt == NULL)) + return -ENOBUFS; + } memcpy(inet->cork.opt, opt, sizeof(struct ip_options)+opt->optlen); inet->cork.flags |= IPCORK_OPT; inet->cork.addr = ipc->addr; diff -ruN linux-2.6.4-rc2-bk5.org/net/ipv6/esp6.c linux-2.6.4-rc2-bk5/net/ipv6/esp6.c --- linux-2.6.4-rc2-bk5.org/net/ipv6/esp6.c 2004-03-09 13:10:42.000000000 -0800 +++ linux-2.6.4-rc2-bk5/net/ipv6/esp6.c 2004-03-09 13:30:03.000000000 -0800 @@ -422,6 +422,8 @@ esp->conf.padlen = 0; if (esp->conf.ivlen) { esp->conf.ivec = kmalloc(esp->conf.ivlen, GFP_KERNEL); + if (unlikely(esp->conf.ivec == NULL)) + goto error; get_random_bytes(esp->conf.ivec, esp->conf.ivlen); } crypto_cipher_setkey(esp->conf.tfm, esp->conf.key, esp->conf.key_len); diff -ruN linux-2.6.4-rc2-bk5.org/net/ipv6/ip6_output.c linux-2.6.4-rc2-bk5/net/ipv6/ip6_output.c --- linux-2.6.4-rc2-bk5.org/net/ipv6/ip6_output.c 2004-03-09 13:10:42.000000000 -0800 +++ linux-2.6.4-rc2-bk5/net/ipv6/ip6_output.c 2004-03-09 13:20:29.000000000 -0800 @@ -816,9 +816,12 @@ * setup for corking */ if (opt) { - if (np->cork.opt == NULL) + if (np->cork.opt == NULL) { np->cork.opt = kmalloc(opt->tot_len, sk->sk_allocation); + if (unlikely(np->cork.opt == NULL)) + return -ENOBUFS; + } memcpy(np->cork.opt, opt, opt->tot_len); inet->cork.flags |= IPCORK_OPT; /* need source address above miyazawa*/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Trivial kmalloc failure checks 2004-03-09 21:36 ` [PATCH] Trivial kmalloc failure checks Krishna Kumar @ 2004-03-09 23:24 ` David S. Miller 0 siblings, 0 replies; 16+ messages in thread From: David S. Miller @ 2004-03-09 23:24 UTC (permalink / raw) To: Krishna Kumar; +Cc: netdev, krkumar On Tue, 9 Mar 2004 13:36:14 -0800 (PST) Krishna Kumar <krkumar@us.ibm.com> wrote: > A couple of checks for memory allocation failures. Looks great, applied. Thanks Krishna. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Hang in downing interface with IPv6 PRIVACY @ 2003-11-09 6:30 David S. Miller 2004-01-10 1:40 ` [PATCH] Bugs in xfrm Krishna Kumar 0 siblings, 1 reply; 16+ messages in thread From: David S. Miller @ 2003-11-09 6:30 UTC (permalink / raw) To: Krishna Kumar; +Cc: netdev, krkumar On Fri, 7 Nov 2003 11:01:41 -0800 (PST) Krishna Kumar <krkumar@us.ibm.com> wrote: > While using PRIVACY extensions, I sometimes get a hang when I remove the > interface. But I can reproduce this every time using the test script at > the end of the mail (hang depends on the order of address deletion). ... > The bug is in ipv6_del_addr() where if a temp address is being deleted ... > Also, the code at the top of the routine is unnecessary, the same is being > done when the address is found a little later in that routine. Looks great, applied. Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] Bugs in xfrm 2003-11-09 6:30 [PATCH] Hang in downing interface with IPv6 PRIVACY David S. Miller @ 2004-01-10 1:40 ` Krishna Kumar 2004-01-10 4:48 ` David S. Miller 0 siblings, 1 reply; 16+ messages in thread From: Krishna Kumar @ 2004-01-10 1:40 UTC (permalink / raw) To: David S. Miller; +Cc: netdev, KK, kumarkr Hi Dave, The following look to be bugs in xfrm related code. 1. In xfrm_lookup, a couple of bugs : - the found or allocated xfrm_states are not passed correctly to xfrm_bundle_create (and to the subsequent frees in case of create failing) if the first xfrm_tmpl_resolve failed and the second one succeeded. - error handling is wrong. 2. In pfkey_get(), the xfrm_state is dereferenced after it is dropped, which could lead to dereferencing freed memory. 3. ipcomp_tunnel_create and xfrm_state_construct don't set x->km.state to XFRM_STATE_DEAD. This can lead to the BUG_TRAP in __xfrm_state_destroy when xfrm_state_put() finds this is the last reference. This was reported as one of the problems of [Bug 1754] some time back. 4. I am not sure of this one. I think dst_free() cannot be used when a bundle of dst's are allocated and have to be freed. We need a new dst_bundle_free() routine to free all linked dst's ?? Change is in __xfrm[46]_bundle_create & xfrm_lookup(), the lookup one I am not very sure. Why are we doing a dst_put(), shouldn't this be a free of all dst's off the dst->child list since the routine marking 'dead' cleared all entries off the dst->next list ? These changes compile cleanly, but I couldn't test since these are corner cases. Please let me know if this can be applied. I am sending as one patch file for now instead of multiple files as they all small. Thanks, - KK diff -ruN linux-2.6.0-rc2-bk6.org/include/net/dst.h linux-2.6.0-rc2-bk6/include/net/dst.h --- linux-2.6.0-rc2-bk6.org/include/net/dst.h 2004-01-09 17:08:18.000000000 -0800 +++ linux-2.6.0-rc2-bk6/include/net/dst.h 2004-01-09 17:08:55.000000000 -0800 @@ -168,6 +168,7 @@ extern void * dst_alloc(struct dst_ops * ops); extern void __dst_free(struct dst_entry * dst); +extern void dst_bundle_free(struct dst_entry * dst); extern struct dst_entry *dst_destroy(struct dst_entry * dst); static inline void dst_free(struct dst_entry * dst) diff -ruN linux-2.6.0-rc2-bk6.org/net/ipv4/ipcomp.c linux-2.6.0-rc2-bk6/net/ipv4/ipcomp.c --- linux-2.6.0-rc2-bk6.org/net/ipv4/ipcomp.c 2004-01-05 13:43:50.000000000 -0800 +++ linux-2.6.0-rc2-bk6/net/ipv4/ipcomp.c 2004-01-09 13:00:22.000000000 -0800 @@ -294,6 +294,7 @@ return t; error: + t->km.state = XFRM_STATE_DEAD; xfrm_state_put(t); t = NULL; goto out; diff -ruN linux-2.6.0-rc2-bk6.org/net/ipv4/xfrm4_policy.c linux-2.6.0-rc2-bk6/net/ipv4/xfrm4_policy.c --- linux-2.6.0-rc2-bk6.org/net/ipv4/xfrm4_policy.c 2004-01-09 15:02:48.000000000 -0800 +++ linux-2.6.0-rc2-bk6/net/ipv4/xfrm4_policy.c 2004-01-09 16:11:57.000000000 -0800 @@ -162,7 +162,7 @@ error: if (dst) - dst_free(dst); + dst_bundle_free(dst); return err; } diff -ruN linux-2.6.0-rc2-bk6.org/net/ipv6/xfrm6_policy.c linux-2.6.0-rc2-bk6/net/ipv6/xfrm6_policy.c --- linux-2.6.0-rc2-bk6.org/net/ipv6/xfrm6_policy.c 2004-01-09 16:43:45.000000000 -0800 +++ linux-2.6.0-rc2-bk6/net/ipv6/xfrm6_policy.c 2004-01-09 16:44:03.000000000 -0800 @@ -184,7 +184,7 @@ error: if (dst) - dst_free(dst); + dst_bundle_free(dst); return err; } diff -ruN linux-2.6.0-rc2-bk6.org/net/key/af_key.c linux-2.6.0-rc2-bk6/net/key/af_key.c --- linux-2.6.0-rc2-bk6.org/net/key/af_key.c 2004-01-05 13:45:47.000000000 -0800 +++ linux-2.6.0-rc2-bk6/net/key/af_key.c 2004-01-09 12:41:30.000000000 -0800 @@ -1283,6 +1283,7 @@ static int pfkey_get(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs) { + __u8 proto; struct sk_buff *out_skb; struct sadb_msg *out_hdr; struct xfrm_state *x; @@ -1297,6 +1298,7 @@ return -ESRCH; out_skb = pfkey_xfrm_state2msg(x, 1, 3); + proto = x->id.proto; xfrm_state_put(x); if (IS_ERR(out_skb)) return PTR_ERR(out_skb); @@ -1304,7 +1306,7 @@ out_hdr = (struct sadb_msg *) out_skb->data; out_hdr->sadb_msg_version = hdr->sadb_msg_version; out_hdr->sadb_msg_type = SADB_DUMP; - out_hdr->sadb_msg_satype = pfkey_proto2satype(x->id.proto); + out_hdr->sadb_msg_satype = pfkey_proto2satype(proto); out_hdr->sadb_msg_errno = 0; out_hdr->sadb_msg_reserved = 0; out_hdr->sadb_msg_seq = hdr->sadb_msg_seq; diff -ruN linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_policy.c linux-2.6.0-rc2-bk6/net/xfrm/xfrm_policy.c --- linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_policy.c 2004-01-09 12:42:53.000000000 -0800 +++ linux-2.6.0-rc2-bk6/net/xfrm/xfrm_policy.c 2004-01-09 17:31:05.000000000 -0800 @@ -694,6 +694,16 @@ static int stale_bundle(struct dst_entry *dst); +void dst_bundle_free(struct dst_entry *dst) +{ + struct dst_entry *next; + + while (dst) { + next = dst->child; + dst_free(dst); + } +} + /* Main function: finds/creates a bundle for given flow. * * At the moment we eat a raw IP route. Mostly to speed up lookups @@ -799,9 +809,16 @@ goto restart; } } - if (err) + if (err < 0) goto error; - } else if (nx == 0) { + /* + * Save number of xfrm_state's found/created for both + * the nx == 0 check below as well as to pass the + * right value to xfrm_bundle_create(). + */ + nx = err; + } + if (nx == 0) { /* Flow passes not transformed. */ xfrm_pol_put(policy); return 0; @@ -827,8 +844,8 @@ write_unlock_bh(&policy->lock); xfrm_pol_put(policy); - if (dst) - dst_free(dst); + if (dst) /* 'dead' freed dst->next list only */ + dst_bundle_free(dst); goto restart; } dst->next = policy->bundles; diff -ruN linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_user.c linux-2.6.0-rc2-bk6/net/xfrm/xfrm_user.c --- linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_user.c 2004-01-09 12:57:42.000000000 -0800 +++ linux-2.6.0-rc2-bk6/net/xfrm/xfrm_user.c 2004-01-09 12:59:00.000000000 -0800 @@ -241,6 +241,7 @@ return x; error: + x->km.state = XFRM_STATE_DEAD; xfrm_state_put(x); error_no_put: *errp = err; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Bugs in xfrm 2004-01-10 1:40 ` [PATCH] Bugs in xfrm Krishna Kumar @ 2004-01-10 4:48 ` David S. Miller 0 siblings, 0 replies; 16+ messages in thread From: David S. Miller @ 2004-01-10 4:48 UTC (permalink / raw) To: Krishna Kumar; +Cc: netdev, krkumar, kumarkr On Fri, 9 Jan 2004 17:40:03 -0800 (PST) Krishna Kumar <krkumar@us.ibm.com> wrote: > These changes compile cleanly, but I couldn't test since these are > corner cases. Please let me know if this can be applied. I am sending > as one patch file for now instead of multiple files as they all small. Maybe you should actually try to test these changes before I think about applying them, for example: > +void dst_bundle_free(struct dst_entry *dst) > +{ > + struct dst_entry *next; > + > + while (dst) { > + next = dst->child; > + dst_free(dst); > + } > +} Explain to me how that won't loop forever if given a non-NULL dst? Next, this dst_bundle_free() thing is totally not needed as far as I can tell. When dst_free() is made, the top-level of the bundle's dst gets added to the garbage collection list, the garbage collection properly walks the children to process the whole bundle. Please redo this patch and please test it this time :) ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2004-03-09 23:24 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2004-01-10 18:33 [PATCH] Bugs in xfrm Krishna Kumar 2004-01-10 20:11 ` David S. Miller 2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar 2004-01-13 21:22 ` [PATCH 2/5] Bad dereference of xfrm_state in pf_key Krishna Kumar 2004-01-14 7:21 ` David S. Miller 2004-01-13 21:23 ` [PATCH 3/5] xfrm_lookup bugs Krishna Kumar 2004-01-14 7:23 ` David S. Miller 2004-01-13 21:24 ` [PATCH 4/5] xfrm_state_construct doesn't set tunnel state Krishna Kumar 2004-01-14 7:25 ` David S. Miller 2004-01-13 21:26 ` [PATCH 5/5] schedule() bug in xfrm_lookup() Krishna Kumar 2004-01-14 7:27 ` David S. Miller 2004-01-14 7:20 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state David S. Miller 2004-03-09 21:36 ` [PATCH] Trivial kmalloc failure checks Krishna Kumar 2004-03-09 23:24 ` David S. Miller -- strict thread matches above, loose matches on Subject: below -- 2003-11-09 6:30 [PATCH] Hang in downing interface with IPv6 PRIVACY David S. Miller 2004-01-10 1:40 ` [PATCH] Bugs in xfrm Krishna Kumar 2004-01-10 4:48 ` David S. Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.