From owner-freebsd-net@FreeBSD.ORG Tue Jan 3 19:36:26 2012 Return-Path: Delivered-To: net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5314E106566B; Tue, 3 Jan 2012 19:36:26 +0000 (UTC) (envelope-from pluknet@gmail.com) Received: from mail-tul01m020-f182.google.com (mail-tul01m020-f182.google.com [209.85.214.182]) by mx1.freebsd.org (Postfix) with ESMTP id 020828FC0A; Tue, 3 Jan 2012 19:36:25 +0000 (UTC) Received: by obbwd18 with SMTP id wd18so19162373obb.13 for ; Tue, 03 Jan 2012 11:36:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=a8ZEoAlLi7dHwuCXefoc/AcGmHtWHbPr9fepw0dvkdU=; b=rKcNs1Ah2f5E3Ec1TPg8n3HdSImJDUXov+7hGZfY5cMikN5ZjYLPOmW3E9nuWrLoxM fUOQ2l8wRGrYncFAbTINv033zRQGOHaNOybXKwO37NP1NoUpJ4TPCsgd6wKYkd0vsjki ju1SZKs/y9wigL96gZodWF5fyEgnhdebhdt9w= MIME-Version: 1.0 Received: by 10.182.45.102 with SMTP id l6mr46060874obm.0.1325619385543; Tue, 03 Jan 2012 11:36:25 -0800 (PST) Sender: pluknet@gmail.com Received: by 10.182.171.67 with HTTP; Tue, 3 Jan 2012 11:36:25 -0800 (PST) In-Reply-To: <201112231508.52861.jhb@freebsd.org> References: <201112231508.52861.jhb@freebsd.org> Date: Tue, 3 Jan 2012 22:36:25 +0300 X-Google-Sender-Auth: u3E2DURq3QNvbebMsNqjwdc0cVg Message-ID: From: Sergey Kandaurov To: John Baldwin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Bjoern Zeeb , net@freebsd.org Subject: Re: [PATCH] Use of unreferenced ifa in in6 X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Jan 2012 19:36:26 -0000 On 24 December 2011 00:08, John Baldwin wrote: > The code to handle the SIOCGLIFADDR and SIOCDLIFADDR ioctls in > in6_lifaddr_ioctl() does not grab a reference to an ifnet address structu= re > that it uses after dropping the IF_ADDR_LOCK(). =A0Based on other code th= at uses > a similar pattern of finding an ifa while under the lock and then using i= t > after dropping the lock, I believe it should be acquiring a reference on = the > ifa and then dropping that reference when it is done using the ifa. =A0Th= is > (untested) patch should fix this I believe: [Some thoughts on this.] FYI, a similar code exists in in_lifaddr_ioctl() under netinet/ which uses an unreferenced ifa. Even when ifa reference is acquired, does this protect ifa internals from concurrent changes? I would additionally lock ifa to serialize multiple bcopy() operations. To do that, IFA_LOCK/UNLOCK() pair exists to lock ifa with ifa_mtx. But there is only one place where such locking is used explicitly. Initially IFA_LOCK/UNLOCK() were introduced in 2002 and used implicitly in IFAREF()/IFAFREE() to lock up ifaddr reference counts. Two years later ifa_mtx started to be used explicitly in one place to protect SIOCSIFNAME in net/if.c:ifhwioctl(). In 8.0 they are removed in favor of refcount(9), and IFAREF/IFAFREE() moved to ifa_ref()/ifa_free(), and now as said in r194602: "The ifa_mtx is now used for exactly one ioctl, and possibly should be removed." Now I'm losing the chain, sorry.. > Index: in6.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- in6.c =A0 =A0 =A0 (revision 228777) > +++ in6.c =A0 =A0 =A0 (working copy) > @@ -1767,6 +1767,8 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (IN6_ARE_ADDR_EQUAL(&ca= ndidate, &match)) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ifa !=3D NULL) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ifa_ref(ifa); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0IF_ADDR_UNLOCK(ifp); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (!ifa) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return EADDRNOTAVAIL; > @@ -1779,16 +1781,20 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, = c > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcopy(&ia->ia_addr, &iflr-= >addr, ia->ia_addr.sin6_len); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error =3D sa6_recoverscope= ( > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(struct sockaddr_i= n6 *)&iflr->addr); > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error !=3D 0) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error !=3D 0) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ifa_free(if= a); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (er= ror); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if ((ifp->if_flags & IFF_P= OINTOPOINT) !=3D 0) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcopy(&ia-= >ia_dstaddr, &iflr->dstaddr, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ia= ->ia_dstaddr.sin6_len); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error =3D = sa6_recoverscope( > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(s= truct sockaddr_in6 *)&iflr->dstaddr); > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error != =3D 0) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error != =3D 0) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 ifa_free(ifa); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0return (error); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bzero(&ifl= r->dstaddr, sizeof(iflr->dstaddr)); > > @@ -1796,6 +1802,7 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0in6_mask2len(&ia->= ia_prefixmask.sin6_addr, NULL); > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0iflr->flags =3D ia->ia6_fl= ags; =A0 =A0/* XXX */ > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ifa_free(ifa); > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 0; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else { > @@ -1819,6 +1826,7 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ia->ia_prefixmask.= sin6_len); > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ifra.ifra_flags =3D ia->ia= 6_flags; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ifa_free(ifa); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return in6_control(so, SIO= CDIFADDR_IN6, (caddr_t)&ifra, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ifp, td); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} --=20 wbr, pluknet