Date: Sun, 21 Jun 2009 21:42:29 +0000 (UTC) From: Robert Watson <rwatson@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r194610 - head/sys/netipx Message-ID: <200906212142.n5LLgTtr019354@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: rwatson Date: Sun Jun 21 21:42:29 2009 New Revision: 194610 URL: http://svn.freebsd.org/changeset/base/194610 Log: Add ipx_ifaddr locking to ipx_control(), which should close most remaining potential races in ifconfig's management of IPX addresses. This is largely accomplished by dropping a global write lock for the IPX address list over the body of in_control(), although there are some places we bump the refcount on an ifaddr of interest while calling out to the routing code or link layer code, which might require revisiting. Annotate one as a potential race if two simultaneous delete ioctls are issued for the same IPX addresses at once. MFC after: 3 weeks Modified: head/sys/netipx/ipx.c Modified: head/sys/netipx/ipx.c ============================================================================== --- head/sys/netipx/ipx.c Sun Jun 21 21:38:12 2009 (r194609) +++ head/sys/netipx/ipx.c Sun Jun 21 21:42:29 2009 (r194610) @@ -1,6 +1,8 @@ /*- * Copyright (c) 1984, 1985, 1986, 1987, 1993 - * The Regents of the University of California. All rights reserved. + * The Regents of the University of California. + * Copyright (c) 2009 Robert N. M. Watson + * All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -109,32 +111,44 @@ ipx_control(struct socket *so, u_long cm */ if (ifp == NULL) return (EADDRNOTAVAIL); + + IPX_IFADDR_WLOCK(); for (ia = ipx_ifaddr; ia != NULL; ia = ia->ia_next) if (ia->ia_ifp == ifp) break; switch (cmd) { case SIOCGIFADDR: - if (ia == NULL) - return (EADDRNOTAVAIL); + if (ia == NULL) { + error = EADDRNOTAVAIL; + goto out; + } *(struct sockaddr_ipx *)&ifr->ifr_addr = ia->ia_addr; - return (0); + goto out; case SIOCGIFBRDADDR: - if (ia == NULL) - return (EADDRNOTAVAIL); - if ((ifp->if_flags & IFF_BROADCAST) == 0) - return (EINVAL); + if (ia == NULL) { + error = EADDRNOTAVAIL; + goto out; + } + if ((ifp->if_flags & IFF_BROADCAST) == 0) { + error = EINVAL; + goto out; + } *(struct sockaddr_ipx *)&ifr->ifr_dstaddr = ia->ia_broadaddr; - return (0); + goto out; case SIOCGIFDSTADDR: - if (ia == NULL) - return (EADDRNOTAVAIL); - if ((ifp->if_flags & IFF_POINTOPOINT) == 0) - return (EINVAL); + if (ia == NULL) { + error = EADDRNOTAVAIL; + goto out; + } + if ((ifp->if_flags & IFF_POINTOPOINT) == 0) { + error = EINVAL; + goto out; + } *(struct sockaddr_ipx *)&ifr->ifr_dstaddr = ia->ia_dstaddr; - return (0); + goto out; } switch (cmd) { @@ -143,7 +157,7 @@ ipx_control(struct socket *so, u_long cm priv = (cmd == SIOCAIFADDR) ? PRIV_NET_ADDIFADDR : PRIV_NET_DELIFADDR; if (td && (error = priv_check(td, priv)) != 0) - return (error); + goto out; if (ifra->ifra_addr.sipx_family == AF_IPX) for (oia = ia; ia != NULL; ia = ia->ia_next) { if (ia->ia_ifp == ifp && @@ -151,20 +165,24 @@ ipx_control(struct socket *so, u_long cm ifra->ifra_addr.sipx_addr)) break; } - if (cmd == SIOCDIFADDR && ia == NULL) - return (EADDRNOTAVAIL); + if (cmd == SIOCDIFADDR && ia == NULL) { + error = EADDRNOTAVAIL; + goto out; + } /* FALLTHROUGH */ case SIOCSIFADDR: case SIOCSIFDSTADDR: if (td && (error = priv_check(td, PRIV_NET_SETLLADDR)) != 0) - return (error); + goto out; if (ia == NULL) { oia = (struct ipx_ifaddr *) malloc(sizeof(*ia), M_IFADDR, - M_WAITOK | M_ZERO); - if (oia == NULL) - return (ENOBUFS); + M_NOWAIT | M_ZERO); + if (oia == NULL) { + error = ENOBUFS; + goto out; + } if ((ia = ipx_ifaddr) != NULL) { for ( ; ia->ia_next != NULL; ia = ia->ia_next) ; @@ -193,32 +211,48 @@ ipx_control(struct socket *so, u_long cm default: if (td && (error = priv_check(td, PRIV_NET_HWIOCTL)) != 0) - return (error); + goto out; } switch (cmd) { case SIOCSIFDSTADDR: - if ((ifp->if_flags & IFF_POINTOPOINT) == 0) - return (EINVAL); + if ((ifp->if_flags & IFF_POINTOPOINT) == 0) { + error = EINVAL; + goto out; + } if (ia->ia_flags & IFA_ROUTE) { rtinit(&(ia->ia_ifa), (int)RTM_DELETE, RTF_HOST); ia->ia_flags &= ~IFA_ROUTE; } + ifa_ref(&ia->ia_ifa); + IPX_IFADDR_WUNLOCK(); if (ifp->if_ioctl) { error = (*ifp->if_ioctl)(ifp, SIOCSIFDSTADDR, (void *)ia); - if (error) + if (error) { + ifa_free(&ia->ia_ifa); return (error); + } } *(struct sockaddr *)&ia->ia_dstaddr = ifr->ifr_dstaddr; + ifa_free(&ia->ia_ifa); return (0); case SIOCSIFADDR: - return (ipx_ifinit(ifp, ia, - (struct sockaddr_ipx *)&ifr->ifr_addr, 1)); + ifa_ref(&ia->ia_ifa); + IPX_IFADDR_WUNLOCK(); + error = ipx_ifinit(ifp, ia, + (struct sockaddr_ipx *)&ifr->ifr_addr, 1); + ifa_free(&ia->ia_ifa); + return (error); case SIOCDIFADDR: + /* XXXRW: Potential race here while ipx_ifaddr_rw is dropped. */ + ifa_ref(&ia->ia_ifa); + IPX_IFADDR_WUNLOCK(); ipx_ifscrub(ifp, ia); + IPX_IFADDR_WLOCK(); + ifa_free(&ia->ia_ifa); ifa = (struct ifaddr *)ia; IF_ADDR_LOCK(ifp); TAILQ_REMOVE(&ifp->if_addrhead, ifa, ifa_link); @@ -236,6 +270,7 @@ ipx_control(struct socket *so, u_long cm printf("Didn't unlink ipxifadr from list\n"); } ifa_free(&oia->ia_ifa); + IPX_IFADDR_WUNLOCK(); return (0); case SIOCAIFADDR: @@ -249,6 +284,8 @@ ipx_control(struct socket *so, u_long cm ia->ia_addr.sipx_addr)) hostIsNew = 0; } + ifa_ref(&ia->ia_ifa); + IPX_IFADDR_WUNLOCK(); if ((ifp->if_flags & IFF_POINTOPOINT) && (ifra->ifra_dstaddr.sipx_family == AF_IPX)) { if (hostIsNew == 0) @@ -259,13 +296,19 @@ ipx_control(struct socket *so, u_long cm if (ifra->ifra_addr.sipx_family == AF_IPX && (hostIsNew || dstIsNew)) error = ipx_ifinit(ifp, ia, &ifra->ifra_addr, 0); + ifa_free(&ia->ia_ifa); return (error); default: + IPX_IFADDR_WUNLOCK(); if (ifp->if_ioctl == NULL) return (EOPNOTSUPP); return ((*ifp->if_ioctl)(ifp, cmd, data)); } + +out: + IPX_IFADDR_WUNLOCK(); + return (error); } /*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200906212142.n5LLgTtr019354>