5.3-RELEASE TODO
Brooks Davis
brooks at one-eyed-alien.net
Fri Sep 17 09:24:10 PDT 2004
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=61588
Change 61588 by brooks at brooks_minya on 2004/09/16 05:07:10
Fix a potential LOR in ifconf caused by using copyout while
holding a lock.
Reported by: rwatson
Affected files ...
.. //depot/user/brooks/cleanup/sys/net/if.c#38 edit
Differences ...
==== //depot/user/brooks/cleanup/sys/net/if.c#38 (text+ko) ====
@@ -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)
+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");
@@ -1484,10 +1485,27 @@
struct ifconf *ifc = (struct ifconf *)data;
struct ifnet *ifp;
struct ifaddr *ifa;
- struct ifreq ifr, *ifrp;
+ struct ifreq ifr, *ifrp, *ifrbuf = NULL;
int space = ifc->ifc_len, error = 0;
+ size_t buflen = 0;
- ifrp = 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
+ * 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 = 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 = *sa;
osa->sa_family = sa->sa_family;
- error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
- sizeof (ifr));
- ifrp++;
+ if (ifrp == NULL) {
+ buflen += sizeof(ifr);
+ } else {
+ bcopy(&ifr, ifrp, sizeof(ifr));
+ ifrp++;
+ }
+
} else
#endif
if (sa->sa_len <= sizeof(*sa)) {
ifr.ifr_addr = *sa;
- error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
- sizeof (ifr));
- ifrp++;
+ if (ifrp == NULL) {
+ buflen += sizeof(struct ifreq);
+ } else {
+ bcopy(&ifr, ifrp, sizeof(ifr));
+ ifrp++;
+ }
} else {
if (space < sizeof (ifr) + sa->sa_len -
sizeof(*sa))
break;
space -= sa->sa_len - sizeof(*sa);
- error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
- sizeof (ifr.ifr_name));
- if (error == 0)
- error = copyout((caddr_t)sa,
- (caddr_t)&ifrp->ifr_addr, sa->sa_len);
- ifrp = (struct ifreq *)
- (sa->sa_len + (caddr_t)&ifrp->ifr_addr);
+ if (ifrp == NULL) {
+ buflen +=
+ 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 = (struct ifreq *)
+ (sa->sa_len +
+ (caddr_t)&ifrp->ifr_addr);
+ }
}
- if (error)
- break;
space -= sizeof (ifr);
}
- if (error)
- break;
- if (!addrs) {
+ if (addrs == 0) {
bzero((caddr_t)&ifr.ifr_addr, sizeof(ifr.ifr_addr));
- error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
- sizeof (ifr));
- if (error)
- break;
+ if (ifrp == NULL) {
+ buflen += sizeof(struct ifreq);
+ } else {
+ bcopy(&ifr, ifrp, sizeof(ifr));
+ ifrp++;
+ }
space -= sizeof (ifr);
- ifrp++;
}
}
IFNET_RUNLOCK();
- ifc->ifc_len -= space;
+
+ if (error != 0)
+ return (error);
+
+ if (ifrp == NULL) {
+ ifrbuf = malloc(buflen, M_IFCONF, M_ZERO | M_NOWAIT);
+ space = buflen;
+ goto again;
+ }
+
+ ifc->ifc_len = buflen - space;
+ error = copyout(ifrbuf, ifc->ifc_req, ifc->ifc_len);
+ free(ifrbuf, M_IFCONF);
return (error);
}
--
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-current/attachments/20040917/a480adc3/attachment.bin
More information about the freebsd-current
mailing list