From owner-freebsd-net@FreeBSD.ORG Tue Jan 3 21:45:35 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 34A911065670; Tue, 3 Jan 2012 21:45:35 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id ECF7B8FC1C; Tue, 3 Jan 2012 21:45:34 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) by cyrus.watson.org (Postfix) with ESMTPSA id A0E6546B0D; Tue, 3 Jan 2012 16:45:34 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 0A8FFB944; Tue, 3 Jan 2012 16:45:34 -0500 (EST) From: John Baldwin To: Sergey Kandaurov Date: Tue, 3 Jan 2012 16:08:59 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p8; KDE/4.5.5; amd64; ; ) References: <201112231508.52861.jhb@freebsd.org> <201201031517.36251.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201201031608.59688.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Tue, 03 Jan 2012 16:45:34 -0500 (EST) 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 21:45:35 -0000 On Tuesday, January 03, 2012 3:44:50 pm Sergey Kandaurov wrote: > On 4 January 2012 00:17, John Baldwin wrote: > > On Tuesday, January 03, 2012 2:36:25 pm Sergey Kandaurov wrote: > >> 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 structure > >> > that it uses after dropping the IF_ADDR_LOCK(). Based on other code that uses > >> > a similar pattern of finding an ifa while under the lock and then using it > >> > 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. This > >> > (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.. > > > > Hmm, I'm not sure if ifa objects become immutable or not once they are > > referenced in the list. Other places in the code seem to use the ifa > > without locking it though, merely obtaining a reference. > > Yes, this is a main concern. > > > The in.c code doesn't even grab the IF_ADDR_LOCK(). :( The below patch > > should fix that and add the same fix as done to the in6.c code. > > > > Index: in.c > > =================================================================== > > --- in.c (revision 229406) > > +++ in.c (working copy) > > @@ -784,6 +784,7 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca > > } > > } > > > > + IF_ADDR_LOCK(ifp); > > TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) { > > if (ifa->ifa_addr->sa_family != AF_INET6) > > continue; > > @@ -794,6 +795,9 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca > > if (candidate.s_addr == match.s_addr) > > break; > > } > > + if (ifa != NULL) > > + ifa_ref(ifa); > > + IF_ADDR_UNLOCK(ifp); > > if (ifa == NULL) > > return (EADDRNOTAVAIL); > > ia = (struct in_ifaddr *)ifa; > > @@ -812,6 +816,7 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca > > in_mask2len(&ia->ia_sockmask.sin_addr); > > > > iflr->flags = 0; /*XXX*/ > > + ifa_free(ifa); > > > > return (0); > > } else { > > @@ -830,6 +835,7 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca > > } > > bcopy(&ia->ia_sockmask, &ifra.ifra_dstaddr, > > ia->ia_sockmask.sin_len); > > + ifa_free(ifa); > > > > return (in_control(so, SIOCDIFADDR, (caddr_t)&ifra, > > ifp, td)); > > > > > > With this patch in_lifaddr_ioctl() now looks more syntactically similar > to in6_lifaddr_ioctl(). They could look even more similar by eliminating > a lot of whitespace changes present here or there. Hmmm. Actually, it seems to be a bit more broken. Note that it is expecting to get a sockaddr_in, but it is checking for AF_INET6, not AF_INET in its loop. That bug seems to go back to the original import from KAME. I'm not sure if the two can be merged since they work on different underyling data structures though. -- John Baldwin