From owner-freebsd-net@FreeBSD.ORG Tue Jan 3 20:44:51 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 67ADA106564A; Tue, 3 Jan 2012 20:44:51 +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 14BD58FC1C; Tue, 3 Jan 2012 20:44:50 +0000 (UTC) Received: by obbwd18 with SMTP id wd18so19261492obb.13 for ; Tue, 03 Jan 2012 12:44:50 -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=apUZyQFi7WQAwmxNTnKaD8RiMhDd6d+PFotOHaS+rdY=; b=RtEG9xAGnHQ/BK8W90O5w2yqlBjT6VvHEU/PmHxRpDw/DqKjzw+d7iAAie6hZo7jBj 5R230Z820UkqheLvb2D8TGmj38hVsiIeZh9lwrS/fYWNwa7fay10CCXxoq9rOF6mcagt n3CAJuxaaGoBiD5hmg1OgV5z34dmC2mlvGXw8= MIME-Version: 1.0 Received: by 10.182.117.97 with SMTP id kd1mr19706642obb.50.1325623490543; Tue, 03 Jan 2012 12:44:50 -0800 (PST) Sender: pluknet@gmail.com Received: by 10.182.171.67 with HTTP; Tue, 3 Jan 2012 12:44:50 -0800 (PST) In-Reply-To: <201201031517.36251.jhb@freebsd.org> References: <201112231508.52861.jhb@freebsd.org> <201201031517.36251.jhb@freebsd.org> Date: Tue, 3 Jan 2012 23:44:50 +0300 X-Google-Sender-Auth: 7HkS5KXwzhXMpEgly1XF-TPGDa4 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 20:44:51 -0000 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 stru= cture >> > that it uses after dropping the IF_ADDR_LOCK(). =A0Based on other code= that uses >> > a similar pattern of finding an ifa while under the lock and then usin= g 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. = =A0This >> > (untested) patch should fix this I believe: >> >> [Some thoughts on this.] >> >> FYI, a similar code exists in in_lifaddr_ioctl() under netinet/ which us= es >> an unreferenced ifa. Even when ifa reference is acquired, does this prot= ect >> ifa internals from concurrent changes? I would additionally lock ifa to >> serialize multiple bcopy() operations. To do that, IFA_LOCK/UNLOCK() pai= r >> 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 referen= ce >> counts. Two years later ifa_mtx started to be used explicitly in one pla= ce >> 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 ioc= tl, >> 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. =A0Other 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(). :( =A0The below patch > should fix that and add the same fix as done to the in6.c code. > > Index: in.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 > --- in.c =A0 =A0 =A0 =A0(revision 229406) > +++ in.c =A0 =A0 =A0 =A0(working copy) > @@ -784,6 +784,7 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca > =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 IF_ADDR_LOCK(ifp); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_= link) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (ifa->ifa_addr->sa_fami= ly !=3D AF_INET6) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0continue; > @@ -794,6 +795,9 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (candidate.s_addr =3D= =3D match.s_addr) > =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 IF_ADDR_UNLOCK(ifp); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (ifa =3D=3D NULL) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (EADDRNOTAVAIL); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ia =3D (struct in_ifaddr *)ifa; > @@ -812,6 +816,7 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0in_mask2le= n(&ia->ia_sockmask.sin_addr); > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0iflr->flags =3D 0; =A0 =A0= =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 { > @@ -830,6 +835,7 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcopy(&ia->ia_sockmask, &i= fra.ifra_dstaddr, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ia->ia_soc= kmask.sin_len); > + =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 (in_control(so, SIO= CDIFADDR, (caddr_t)&ifra, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ifp, 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. --=20 wbr, pluknet