lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <87iq1j5r7z.fsf@small.ssi.corp> Date: Sun, 03 Oct 2010 15:41:04 +0200 From: arno@...isbad.org (Arnaud Ebalard) To: Herbert Xu <herbert@...dor.apana.org.au> Cc: David Miller <davem@...emloft.net>, eric.dumazet@...il.com, yoshfuji@...ux-ipv6.org, netdev@...r.kernel.org Subject: Re: [PATCHv3 net-next-2.6 3/5] XFRM,IPv6: Add IRO src/dst address remapping XFRM types and i/o handlers Hi Herbert, Herbert Xu <herbert@...dor.apana.org.au> writes: > On Sat, Oct 02, 2010 at 12:17:35PM +0200, Arnaud Ebalard wrote: >> >> and I see no reason not to keep the lock we have on the state until the >> end of the function when the state is valid (when we break), instead of >> releasing it to get it again later. Something like the following would >> allow removing the spin_lock()/spin_unlock() calls from all mip6 input >> handlers (mip6_{destopt,rthdr,iro_src,iro_dst}_input()): > > No I moved the state lock down precisely because it should not > be taken at a higher level as that breaks asynchronous IPsec > processing and the fact that it isn't needed in most places. > > If your code needs it then you should take it rather than impose > it on real IPsec users. Understood. Note that I am on your side with this: my primary concern while pushing the feature is *to not break or slow down standard IPsec*. I do not expect my code to be accepted or even read otherwise. As for the current point raised by David on the position of the locks in my input handlers, they are based on the position of the locks in the *existing* RH2 (mip6_rthdr_input()) and HAO (mip6_destopt_input()) handlers. As they serve the same purpose (src/dst address check against state's address) and the code is basically the same, I have no reason to do things differently as what is currently upstream. After your reply, I took a (too long) look at the history of xfrm6_input_addr() to understand why it is as it is. If it can spare you some time, here is what I think happened: - Initially (commit fbd9a5b4, Aug 23 2006), the checks on the status of state, the call to x->type->input() and the changes on state's processing stats (x->curlft changes) were *globally* protected by a call to spin_lock(). The same day, a related commit (3d126890) added support for RH2/HAO input handler. No lock inside the handler. The content of xfrm6_input_addr() was: spin_lock(&x->lock); <...snip...> nh = x->type->input(x, skb); if (nh <= 0) { spin_unlock(&x->lock); xfrm_state_put(x); x = NULL; continue; } x->curlft.bytes += skb->len; x->curlft.packets++; spin_unlock(&x->lock); - Then, as you wrote, the state lock was moved in all input handlers (commit 0ebea8ef, Nov 13 2007), including RH2/HAO ones: @@ -128,12 +128,15 @@ static int mip6_destopt_input(struct xfrm_state *x, struct sk_buff *skb) { struct ipv6hdr *iph = ipv6_hdr(skb); struct ipv6_destopt_hdr *destopt = (struct ipv6_destopt_hdr *)skb->data; + int err = destopt->nexthdr; + spin_lock(&x->lock); if (!ipv6_addr_equal(&iph->saddr, (struct in6_addr *)x->coaddr) && !ipv6_addr_any((struct in6_addr *)x->coaddr)) - return -ENOENT; + err = -ENOENT; + spin_unlock(&x->lock); - return destopt->nexthdr; + return err; } With that commit, I think a deadlock was introduced in MIPv6 code because xfm6_input_addr() was left unchanged, i.e. x->type->input() was called with the lock held. Am I correct? - The code of xfrm6_input_addr() was then optimized by commit a002c6fd in such a way that x->type->input() was then put outside the protection of the lock, which (if I am not mistaken) removed the deadlock: spin_lock(&x->lock); if ((!i || (x->props.flags & XFRM_STATE_WILDRECV)) && likely(x->km.state == XFRM_STATE_VALID) && !xfrm_state_check_expire(x)) { spin_unlock(&x->lock); if (x->type->input(x, skb) > 0) { /* found a valid state */ break; } } else spin_unlock(&x->lock); I don't know if this is was intentional. But the main question remains on the position of the lock. Here, checks are done on the status of the state, lock is released, reacquired in the input handler to do additional check and then released again, to be reacquired later in the function to act on statistics. Is my reading of the code correct? Herbert, you certainly have a better understanding of XFRM code than I have and can probably tell if the locking behavior above is valid or buggy. Yoshifuji-san, David or Eric may also have good ideas on that. As a side note (I think I was not explicit enough in my previous email), I think the possible changes to xfrm_input_addr() and MIPv6 handlers we are discussing are not expected to impact standard IPsec code because there are 2 different cases in which states input handlers are called (i.e. x->type->input()): - xfrm_input(): for standard IPsec case (incl. async resumption). This is only for esp, ah, ipcomp and tunneling. - xfrm6_input_addr(): for MIPv6 extension header, i.e. RH2 and HAO in destopt. and we are discussing the second. David, as for my patches, if this is ok for you, I will keep the code of my input handlers aligned on the code of RH2/HAO handlers and will modify it later based on the possible corrections made on those upstream. Don't hesitate to slap me if I made some mistakes in my analysis ;-) Cheers, a+ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists