Date: Fri, 17 Sep 2004 09:25:35 -0700 From: Brooks Davis <brooks@one-eyed-alien.net> To: re@FreeBSD.org Cc: current@FreeBSD.org Subject: Re: 5.3-RELEASE TODO Message-ID: <20040917162535.GA5750@odin.ac.hmc.edu> In-Reply-To: <200409170741.i8H7fGV3011078@pooker.samsco.org> References: <200409170741.i8H7fGV3011078@pooker.samsco.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--ZPt4rx8FFjLCG7dd Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 17, 2004 at 01:41:16AM -0600, Scott Long wrote: > |--------------------+-----------+-------------+------------------------= -| > | | | |The ifconf() ioctl for = | > | | | |listing network = | > | | | |interfaces performs a = | > | | | |copyout() while holding = | > |ifconf() sleep | | |the global ifnet list = | > |warning |In progress|Robert Watson|mutex. This generates a = | > | | | |witness warning in the = | > | | | |event that copyout() = | > | | | |generates a page fault, = | > | | | |and risks more serious = | > | | | |problems. = | > +-----------------------------------------------------------------------= -+ I've got a patch for this one. If someone will review it, I will commit it to HEAD for further testing. -- Brooks http://perforce.freebsd.org/chv.cgi?CH=3D61588 Change 61588 by brooks@brooks_minya on 2004/09/16 05:07:10 Fix a potential LOR in ifconf caused by using copyout while holding a lock. =09 Reported by: rwatson Affected files ... =2E. //depot/user/brooks/cleanup/sys/net/if.c#38 edit Differences ... =3D=3D=3D=3D //depot/user/brooks/cleanup/sys/net/if.c#38 (text+ko) =3D=3D= =3D=3D @@ -123,6 +123,7 @@ SYSINIT(interfaces, SI_SUB_INIT_IF, SI_ORDER_FIRST, if_init, NULL) SYSINIT(interface_check, SI_SUB_PROTO_IF, SI_ORDER_FIRST, if_check, NULL) =20 +MALLOC_DEFINE(M_IFCONF, "ifconf", "interface configuration info"); MALLOC_DEFINE(M_IFADDR, "ifaddr", "interface address"); MALLOC_DEFINE(M_IFMADDR, "ether_multi", "link-level multicast address"); =20 @@ -1484,10 +1485,27 @@ struct ifconf *ifc =3D (struct ifconf *)data; struct ifnet *ifp; struct ifaddr *ifa; - struct ifreq ifr, *ifrp; + struct ifreq ifr, *ifrp, *ifrbuf =3D NULL; int space =3D ifc->ifc_len, error =3D 0; + size_t buflen =3D 0; =20 - ifrp =3D ifc->ifc_req; + /* + * Because it is not safe to do a copyout while a lock it held, + * we need to avoid doing a copyout while walking the interface + * list. The easiest way to do that is to stage all the data + * into a single buffer rather then doing many copyouts. + * Unfortunatly, we can't just trust the users buffer length + * because that might cause use to allocate too much kernel + * memory. Thus we need to bound the memory by allocate by the=20 + * actual space needed. In order to do that with the minimum + * duplication of code, we make two passes through the loop over + * interfaces and addresses. In the first pass, we have no + * buffer and we count how much space we needed and malloc it + * after releasing the lock. Then we go through again and + * actually fill the buffer followed by copying it out. + */ +again: + ifrp =3D ifrbuf; IFNET_RLOCK(); /* could sleep XXX */ TAILQ_FOREACH(ifp, &ifnet, if_link) { int addrs; @@ -1516,47 +1534,68 @@ (struct osockaddr *)&ifr.ifr_addr; ifr.ifr_addr =3D *sa; osa->sa_family =3D sa->sa_family; - error =3D copyout((caddr_t)&ifr, (caddr_t)ifrp, - sizeof (ifr)); - ifrp++; + if (ifrp =3D=3D NULL) { + buflen +=3D sizeof(ifr); + } else { + bcopy(&ifr, ifrp, sizeof(ifr)); + ifrp++; + } + =09 } else #endif if (sa->sa_len <=3D sizeof(*sa)) { ifr.ifr_addr =3D *sa; - error =3D copyout((caddr_t)&ifr, (caddr_t)ifrp, - sizeof (ifr)); - ifrp++; + if (ifrp =3D=3D NULL) { + buflen +=3D sizeof(struct ifreq); + } else { + bcopy(&ifr, ifrp, sizeof(ifr)); + ifrp++; + } } else { if (space < sizeof (ifr) + sa->sa_len - sizeof(*sa)) break; space -=3D sa->sa_len - sizeof(*sa); - error =3D copyout((caddr_t)&ifr, (caddr_t)ifrp, - sizeof (ifr.ifr_name)); - if (error =3D=3D 0) - error =3D copyout((caddr_t)sa, - (caddr_t)&ifrp->ifr_addr, sa->sa_len); - ifrp =3D (struct ifreq *) - (sa->sa_len + (caddr_t)&ifrp->ifr_addr); + if (ifrp =3D=3D NULL) { + buflen +=3D + offsetof(struct ifreq, ifr_addr) + + sa->sa_len; + } else { + bcopy(&ifr, ifrp, + sizeof(ifr.ifr_name)); + bcopy(sa, &ifrp->ifr_addr, sa->sa_len); + ifrp =3D (struct ifreq *) + (sa->sa_len + + (caddr_t)&ifrp->ifr_addr); + } } - if (error) - break; space -=3D sizeof (ifr); } - if (error) - break; - if (!addrs) { + if (addrs =3D=3D 0) { bzero((caddr_t)&ifr.ifr_addr, sizeof(ifr.ifr_addr)); - error =3D copyout((caddr_t)&ifr, (caddr_t)ifrp, - sizeof (ifr)); - if (error) - break; + if (ifrp =3D=3D NULL) { + buflen +=3D sizeof(struct ifreq); + } else { + bcopy(&ifr, ifrp, sizeof(ifr)); + ifrp++; + } space -=3D sizeof (ifr); - ifrp++; } } IFNET_RUNLOCK(); - ifc->ifc_len -=3D space; + + if (error !=3D 0) + return (error); + + if (ifrp =3D=3D NULL) { + ifrbuf =3D malloc(buflen, M_IFCONF, M_ZERO | M_NOWAIT); + space =3D buflen; + goto again; + } + + ifc->ifc_len =3D buflen - space; + error =3D copyout(ifrbuf, ifc->ifc_req, ifc->ifc_len); + free(ifrbuf, M_IFCONF); return (error); } =20 --=20 Any statement of the form "X is the one, true Y" is FALSE. PGP fingerprint 655D 519C 26A7 82E7 2529 9BF0 5D8E 8BE9 F238 1AD4 --ZPt4rx8FFjLCG7dd Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.1 (GNU/Linux) iD8DBQFBSw//XY6L6fI4GtQRAjOfAKDGreIHIlf3BXP8z4iuqS5QKRF0hQCg5SFv 2/ozmga1Wqw8nW70ozJx3v0= =JSys -----END PGP SIGNATURE----- --ZPt4rx8FFjLCG7dd--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040917162535.GA5750>