On 24 December 2011 00:08, John Baldwin <j...@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 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..
> Index: in6.c
> ===================================================================
> --- in6.c (revision 228777)
> +++ in6.c (working copy)
> @@ -1767,6 +1767,8 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c
> if (IN6_ARE_ADDR_EQUAL(&candidate, &match))
> break;
> }
> + if (ifa != NULL)
> + ifa_ref(ifa);
> IF_ADDR_UNLOCK(ifp);
> if (!ifa)
> return EADDRNOTAVAIL;
> @@ -1779,16 +1781,20 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c
> bcopy(&ia->ia_addr, &iflr->addr, ia->ia_addr.sin6_len);
> error = sa6_recoverscope(
> (struct sockaddr_in6 *)&iflr->addr);
> - if (error != 0)
> + if (error != 0) {
> + ifa_free(ifa);
> return (error);
> + }
>
> if ((ifp->if_flags & IFF_POINTOPOINT) != 0) {
> bcopy(&ia->ia_dstaddr, &iflr->dstaddr,
> ia->ia_dstaddr.sin6_len);
> error = sa6_recoverscope(
> (struct sockaddr_in6 *)&iflr->dstaddr);
> - if (error != 0)
> + if (error != 0) {
> + ifa_free(ifa);
> return (error);
> + }
> } else
> bzero(&iflr->dstaddr, sizeof(iflr->dstaddr));
>
> @@ -1796,6 +1802,7 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c
> in6_mask2len(&ia->ia_prefixmask.sin6_addr, NULL);
>
> iflr->flags = ia->ia6_flags; /* XXX */
> + ifa_free(ifa);
>
> return 0;
> } else {
> @@ -1819,6 +1826,7 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c
> ia->ia_prefixmask.sin6_len);
>
> ifra.ifra_flags = ia->ia6_flags;
> + ifa_free(ifa);
> return in6_control(so, SIOCDIFADDR_IN6, (caddr_t)&ifra,
> ifp, td);
> }
--
wbr,
pluknet
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
