From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krishna Kumar Subject: Re: [PATCH] Bugs in xfrm Date: Sat, 10 Jan 2004 10:33:22 -0800 Sender: netdev-bounce@oss.sgi.com Message-ID: Mime-Version: 1.0 Content-Type: multipart/alternative; Boundary="0__=07BBE484DFCD6FFA8f9e8a93df938690918c07BBE484DFCD6FFA" Cc: krkumar@us.ibm.com, netdev@oss.sgi.com Return-path: To: "David S. Miller" Content-Disposition: inline Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org --0__=07BBE484DFCD6FFA8f9e8a93df938690918c07BBE484DFCD6FFA Content-type: text/plain; charset=US-ASCII > 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 --0__=07BBE484DFCD6FFA8f9e8a93df938690918c07BBE484DFCD6FFA Content-type: text/html; charset=US-ASCII Content-Disposition: inline

> 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
--0__=07BBE484DFCD6FFA8f9e8a93df938690918c07BBE484DFCD6FFA-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David S. Miller" Subject: Re: [PATCH] Bugs in xfrm Date: Sat, 10 Jan 2004 12:11:34 -0800 Sender: netdev-bounce@oss.sgi.com Message-ID: <20040110121134.08481951.davem@redhat.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: krkumar@us.ibm.com, netdev@oss.sgi.com Return-path: To: Krishna Kumar In-Reply-To: Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org On Sat, 10 Jan 2004 10:33:22 -0800 Krishna Kumar wrote: > Or should I send separate patches this time ? I think this would be a good idea. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krishna Kumar Subject: [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Date: Tue, 13 Jan 2004 13:21:19 -0800 (PST) Sender: netdev-bounce@oss.sgi.com Message-ID: References: <20040110121134.08481951.davem@redhat.com> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: Krishna Kumar , Return-path: To: "David S. Miller" In-Reply-To: <20040110121134.08481951.davem@redhat.com> Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org > > 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; From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krishna Kumar Subject: [PATCH 2/5] Bad dereference of xfrm_state in pf_key Date: Tue, 13 Jan 2004 13:22:36 -0800 (PST) Sender: netdev-bounce@oss.sgi.com Message-ID: References: Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: netdev@oss.sgi.com Return-path: To: "David S. Miller" In-Reply-To: Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org 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; From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krishna Kumar Subject: [PATCH 3/5] xfrm_lookup bugs Date: Tue, 13 Jan 2004 13:23:35 -0800 (PST) Sender: netdev-bounce@oss.sgi.com Message-ID: References: Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: netdev@oss.sgi.com Return-path: To: "David S. Miller" In-Reply-To: Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org 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; From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krishna Kumar Subject: [PATCH 4/5] xfrm_state_construct doesn't set tunnel state Date: Tue, 13 Jan 2004 13:24:23 -0800 (PST) Sender: netdev-bounce@oss.sgi.com Message-ID: References: Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: netdev@oss.sgi.com, KK Return-path: To: "David S. Miller" In-Reply-To: Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org 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; From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krishna Kumar Subject: [PATCH 5/5] schedule() bug in xfrm_lookup() Date: Tue, 13 Jan 2004 13:26:21 -0800 (PST) Sender: netdev-bounce@oss.sgi.com Message-ID: References: Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: netdev@oss.sgi.com, KK Return-path: To: "David S. Miller" In-Reply-To: Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org 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; From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David S. Miller" Subject: Re: [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Date: Tue, 13 Jan 2004 23:20:09 -0800 Sender: netdev-bounce@oss.sgi.com Message-ID: <20040113232009.008a57a7.davem@redhat.com> References: <20040110121134.08481951.davem@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: krkumar@us.ibm.com, netdev@oss.sgi.com Return-path: To: Krishna Kumar In-Reply-To: Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org On Tue, 13 Jan 2004 13:21:19 -0800 (PST) Krishna Kumar 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David S. Miller" Subject: Re: [PATCH 2/5] Bad dereference of xfrm_state in pf_key Date: Tue, 13 Jan 2004 23:21:37 -0800 Sender: netdev-bounce@oss.sgi.com Message-ID: <20040113232137.463554c9.davem@redhat.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: netdev@oss.sgi.com Return-path: To: Krishna Kumar In-Reply-To: Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org On Tue, 13 Jan 2004 13:22:36 -0800 (PST) Krishna Kumar 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David S. Miller" Subject: Re: [PATCH 3/5] xfrm_lookup bugs Date: Tue, 13 Jan 2004 23:23:35 -0800 Sender: netdev-bounce@oss.sgi.com Message-ID: <20040113232335.2236706f.davem@redhat.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: netdev@oss.sgi.com Return-path: To: Krishna Kumar In-Reply-To: Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org On Tue, 13 Jan 2004 13:23:35 -0800 (PST) Krishna Kumar 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David S. Miller" Subject: Re: [PATCH 4/5] xfrm_state_construct doesn't set tunnel state Date: Tue, 13 Jan 2004 23:25:24 -0800 Sender: netdev-bounce@oss.sgi.com Message-ID: <20040113232524.6ca5b0fe.davem@redhat.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: netdev@oss.sgi.com, krkumar@us.ibm.com Return-path: To: Krishna Kumar In-Reply-To: Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org On Tue, 13 Jan 2004 13:24:23 -0800 (PST) Krishna Kumar 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David S. Miller" Subject: Re: [PATCH 5/5] schedule() bug in xfrm_lookup() Date: Tue, 13 Jan 2004 23:27:22 -0800 Sender: netdev-bounce@oss.sgi.com Message-ID: <20040113232722.2c04bc03.davem@redhat.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: netdev@oss.sgi.com, krkumar@us.ibm.com Return-path: To: Krishna Kumar In-Reply-To: Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org On Tue, 13 Jan 2004 13:26:21 -0800 (PST) Krishna Kumar wrote: > When xfrm_tmpl_resolve fails, schedule() before retrying. Also, cleaned > up the set_task_* routines to use set_current_*, etc. Applied, thanks Krishna. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krishna Kumar Subject: [PATCH] Trivial kmalloc failure checks Date: Tue, 9 Mar 2004 13:36:14 -0800 (PST) Sender: netdev-bounce@oss.sgi.com Message-ID: References: <20040113232009.008a57a7.davem@redhat.com> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: netdev@oss.sgi.com, Return-path: To: "David S. Miller" In-Reply-To: <20040113232009.008a57a7.davem@redhat.com> Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org 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*/ From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David S. Miller" Subject: Re: [PATCH] Trivial kmalloc failure checks Date: Tue, 9 Mar 2004 15:24:30 -0800 Sender: netdev-bounce@oss.sgi.com Message-ID: <20040309152430.2a2164bb.davem@redhat.com> References: <20040113232009.008a57a7.davem@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: netdev@oss.sgi.com, krkumar@us.ibm.com Return-path: To: Krishna Kumar In-Reply-To: Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org On Tue, 9 Mar 2004 13:36:14 -0800 (PST) Krishna Kumar wrote: > A couple of checks for memory allocation failures. Looks great, applied. Thanks Krishna.