Date: Tue, 3 Jan 2012 23:44:50 +0300 From: Sergey Kandaurov <pluknet@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: Bjoern Zeeb <bz@freebsd.org>, net@freebsd.org Subject: Re: [PATCH] Use of unreferenced ifa in in6 Message-ID: <CAE-mSOLhJ0O5DCjje%2BhHj1%2BO6c=MKR=7K0p5trj9T_XkPgWgng@mail.gmail.com> In-Reply-To: <201201031517.36251.jhb@freebsd.org> References: <201112231508.52861.jhb@freebsd.org> <CAE-mSOJ40XE3GH4aRw2coJz8LZ61dQ7PDJWWqK64H6cP5Updrw@mail.gmail.com> <201201031517.36251.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 4 January 2012 00:17, John Baldwin <jhb@freebsd.org> wrote:
> On Tuesday, January 03, 2012 2:36:25 pm Sergey Kandaurov wrote:
>> On 24 December 2011 00:08, John Baldwin <jhb@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAE-mSOLhJ0O5DCjje%2BhHj1%2BO6c=MKR=7K0p5trj9T_XkPgWgng>