Re: 5.3-RELEASE TODO

On Sat, Sep 18, 2004 at 02:24:41PM -0700, Brooks Davis wrote:
> On Sat, Sep 18, 2004 at 01:42:24PM -0400, Robert Watson wrote:
> > 
> > On Sat, 18 Sep 2004, Brooks Davis wrote:
> > 
> > > > Have you tried seeing just how many addresses you can add before
> > > > getifaddrs() fails to return the complete list?  128k seems like a lot,
> > > > but I instrumente ifconf() locally a couple of weeks ago when I first
> > > > became aware of this problem, and discovered that even on my notebook
> > > > (which has a wireless card with one IP, and an unused ethernet card) that
> > > > I see moderately large buffers being read from user space:
> > > > 
> > > > ifconf: 16384 space
> > > 
> > > Those allocations don't seem to make any sense.  The actual space
> > > required is quite small.  All you do is copy one struct ifreq out for
> > > each address, plus one for each interface with no addresses.  The base
> > > size of a struct ifreq is 32 bytes and it extends to 34 for IPv6
> > > addresses.  The maximum size allowed by the data types is 273 (for a 255
> > > byte address).  Since I think IPv6 are the largest addresses used in
> > > practice, MAXPHYS is probably not too bad, though it does put a new cap
> > > on the number of interfaces at ~4k. 
> > > 
> > > If we want to keep kernel allocations small and allow all the itnerfaces
> > > to be reliably reported, we probably need to go back to my origional
> > > plan where we loop repeatidly.  I might do it differently by allocating
> > > up to MAXPHYS and only reallocating if we overflow.  That would avoid
> > > doing it twice (or more) on normal machines while still being correct. 
> > 
> > I'm not too worried about theory, mostly about practice.  I.e., if you add
> > a few thousand IP addresses to a tap device, does all go happily?
> 
> After adding the missing sbuf_delete(), I was able to create 2000
> aliases on a vlan.  I think the current practical limit is ~4090.
> I'm working on a new version that initially caps the allocation at
> MAXPHYS and then retries if it doesn't have enough space.

Here's a new an improved patch that let me allocate 8128 aliases on a
vlan.  It's a bit ugly because I had to fall back to looping if we don't
allocate enough space, but I think this is as good as it gets for this
rather poorly designed interface.

It's worth noting that the userland code that guesses the buffer size is
really bogus in this situation.  It guesses very badly (a few K when it
needs >100k) and then compounds the error by doing a linear search for
the right buffer size.  This means that instead of being O(addresses),
it's actully O(addresses^2).

-- Brooks

--- /home/brooks/working/freebsd/p4/freebsd/sys/net/if.c	Fri Sep 17 22:11:13 2004
+++ sys/net/if.c	Sat Sep 18 15:41:03 2004
_at__at_ -36,9 +36,11 _at__at_
 #include "opt_mac.h"
 
 #include <sys/param.h>
+#include <sys/types.h>
 #include <sys/conf.h>
 #include <sys/mac.h>
 #include <sys/malloc.h>
+#include <sys/sbuf.h>
 #include <sys/bus.h>
 #include <sys/mbuf.h>
 #include <sys/systm.h>
_at__at_ -1483,28 +1485,34 _at__at_
 	struct ifconf *ifc = (struct ifconf *)data;
 	struct ifnet *ifp;
 	struct ifaddr *ifa;
-	struct ifreq ifr, *ifrp;
-	int space = ifc->ifc_len, error = 0;
+	struct ifreq ifr;
+	struct sbuf *sb;
+	int error, full = 0, valid_len, max_len;
+
+	/* Limit initial buffer size to MAXPHYS to avoid DoS from userspace. */
+	max_len = MAXPHYS - 1;
+
+again:
+	if (ifc->ifc_len <= max_len) {
+		max_len = ifc->ifc_len;
+		full = 1;
+	}
+	sb = sbuf_new(NULL, NULL, max_len + 1, SBUF_FIXEDLEN);
+	max_len = 0;
+	valid_len = 0;
 
-	ifrp = ifc->ifc_req;
 	IFNET_RLOCK();		/* could sleep XXX */
 	TAILQ_FOREACH(ifp, &ifnet, if_link) {
 		int addrs;
 
-		if (space < sizeof(ifr))
-			break;
 		if (strlcpy(ifr.ifr_name, ifp->if_xname, sizeof(ifr.ifr_name))
-		    >= sizeof(ifr.ifr_name)) {
-			error = ENAMETOOLONG;
-			break;
-		}
+		    >= sizeof(ifr.ifr_name))
+			return (ENAMETOOLONG);
 
 		addrs = 0;
 		TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
 			struct sockaddr *sa = ifa->ifa_addr;
 
-			if (space < sizeof(ifr))
-				break;
 			if (jailed(curthread->td_ucred) &&
 			    prison_if(curthread->td_ucred, sa))
 				continue;
_at__at_ -1515,47 +1523,49 _at__at_
 					 (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++;
+				sbuf_bcat(sb, &ifr, sizeof(ifr));
+				max_len += sizeof(ifr);
 			} else
 #endif
 			if (sa->sa_len <= sizeof(*sa)) {
 				ifr.ifr_addr = *sa;
-				error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
-						sizeof (ifr));
-				ifrp++;
+				sbuf_bcat(sb, &ifr, sizeof(ifr));
+				max_len += sizeof(ifr);
 			} 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);
+				sbuf_bcat(sb, &ifr,
+				    offsetof(struct ifreq, ifr_addr));
+				max_len += offsetof(struct ifreq, ifr_addr);
+				sbuf_bcat(sb, sa, sa->sa_len);
+				max_len += sa->sa_len;
 			}
-			if (error)
-				break;
-			space -= sizeof (ifr);
+
+			if (!sbuf_overflowed(sb))
+				valid_len = sbuf_len(sb);
 		}
-		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;
-			space -= sizeof (ifr);
-			ifrp++;
+			sbuf_bcat(sb, &ifr, sizeof(ifr));
+			max_len += sizeof(ifr);
+
+			if (!sbuf_overflowed(sb))
+				valid_len = sbuf_len(sb);
 		}
 	}
 	IFNET_RUNLOCK();
-	ifc->ifc_len -= space;
+
+	/*
+	 * If we didn't allocate enough space (uncommon), try again.  If
+	 * we have already allocated as much space as we are allowed,
+	 * return what we've got.
+	 */
+	if (valid_len != max_len && !full) {
+		sbuf_delete(sb);
+		goto again;
+	}
+
+	ifc->ifc_len = valid_len;
+	error = copyout(sbuf_data(sb), ifc->ifc_req, ifc->ifc_len);
+	sbuf_delete(sb);
 	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