From owner-svn-src-all@FreeBSD.ORG Thu Nov 12 13:52:41 2009 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D1205106568D; Thu, 12 Nov 2009 13:52:41 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 7E7338FC21; Thu, 12 Nov 2009 13:52:41 +0000 (UTC) Received: from fledge.watson.org (fledge.watson.org [65.122.17.41]) by cyrus.watson.org (Postfix) with ESMTPS id 3673A46B46; Thu, 12 Nov 2009 08:52:41 -0500 (EST) Date: Thu, 12 Nov 2009 13:52:41 +0000 (GMT) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: Xin LI In-Reply-To: <200911112130.nABLUw9b007768@svn.freebsd.org> Message-ID: References: <200911112130.nABLUw9b007768@svn.freebsd.org> User-Agent: Alpine 2.00 (BSF 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r199201 - in head: contrib/libpcap sbin/ifconfig share/man/man4 sys/kern sys/net sys/sys X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Nov 2009 13:52:42 -0000 On Wed, 11 Nov 2009, Xin LI wrote: > Author: delphij > Date: Wed Nov 11 21:30:58 2009 > New Revision: 199201 > URL: http://svn.freebsd.org/changeset/base/199201 > > Log: > Add interface description capability as inspired by OpenBSD. Patches like this keep being proposed and rejected, so I was a bit surprised to see it hit the tree (but also see that I missed yet another thread on it on net@ in August, apparently). Most of my thoughts on it are here: http://www.freebsd.org/cgi/query-pr.cgi?pr=83622 This commit seems to have quite a few problems, and would likely have benefited from further review before being committed. > Modified: head/contrib/libpcap/inet.c > ============================================================================== > --- head/contrib/libpcap/inet.c Wed Nov 11 21:18:27 2009 (r199200) > +++ head/contrib/libpcap/inet.c Wed Nov 11 21:30:58 2009 (r199201) > @@ -403,22 +403,30 @@ add_addr_to_iflist(pcap_if_t **alldevs, > pcap_addr_t *curaddr, *prevaddr, *nextaddr; > #ifdef SIOCGIFDESCR > struct ifreq ifrdesc; > +#ifdef __FreeBSD__ > +#define _IFDESCRSIZE 64 Seems like this should be #ifdef IFDESCRSIZE rather than ifdef FreeBSD? Also, have you checked with upstream to see if autoconf foo wouldn't be preferred? > + char ifdescr[_IFDESCRSIZE]; > +#else > char ifdescr[IFDESCRSIZE]; > - int s; > #endif > + int s; > > -#ifdef SIOCGIFDESCR > /* > * Get the description for the interface. > */ > memset(&ifrdesc, 0, sizeof ifrdesc); > strlcpy(ifrdesc.ifr_name, name, sizeof ifrdesc.ifr_name); > +#ifdef __FreeBSD__ > + ifrdesc.ifr_buffer.buffer = ifdescr; > + ifrdesc.ifr_buffer.length = _IFDESCRSIZE; > +#else > ifrdesc.ifr_data = (caddr_t)&ifdescr; sizeof(ifdescr) would be more robust in the presence of future code changes. > @@ -258,6 +258,12 @@ Disable permanently promiscuous mode. > Another name for the > .Fl alias > parameter. > +.It Cm description Ar value > +Specify a description of the interface. > +This can be used to label interfaces in situations where they may > +otherwise be difficult to distinguish. > +.It Cm -description > +Clear the interface description. Possibly a comment on length limits would be appropriate? > Modified: head/sbin/ifconfig/ifconfig.c > ============================================================================== > --- head/sbin/ifconfig/ifconfig.c Wed Nov 11 21:18:27 2009 (r199200) > +++ head/sbin/ifconfig/ifconfig.c Wed Nov 11 21:30:58 2009 (r199201) > @@ -83,6 +83,8 @@ static const char rcsid[] = > struct ifreq ifr; > > char name[IFNAMSIZ]; > +char *descr = NULL; > +size_t descrlen = 64; > int setaddr; > int setmask; > int doalias; > @@ -822,6 +824,36 @@ setifname(const char *val, int dummy __u > free(newname); > } > > +/* ARGSUSED */ > +static void > +setifdescr(const char *val, int dummy __unused, int s, > + const struct afswtch *afp) > +{ > + char *newdescr; > + > + newdescr = strdup(val); > + if (newdescr == NULL) { > + warn("no memory to set ifdescr"); > + return; > + } > + ifr.ifr_buffer.buffer = newdescr; > + ifr.ifr_buffer.length = strlen(newdescr); > + if (ioctl(s, SIOCSIFDESCR, (caddr_t)&ifr) < 0) { > + warn("ioctl (set descr)"); > + free(newdescr); > + return; I'm confused about the semantics here: is ifr_buffer.buffer guaranteed to be nul-terminated or not? In some cases the code seems to imply nul-termination (especially on the string coming back to userspace), but in other places (such as here), the nul termination is not included in the buffer. It would be nice to consistently do one or the other, and given the way the code is written, I suggest always having the nul termination. > @@ -866,6 +898,23 @@ status(const struct afswtch *afp, const > printf(" mtu %d", ifr.ifr_mtu); > putchar('\n'); > > + descr = reallocf(descr, descrlen); > + if (descr != NULL) { > + do { > + ifr.ifr_buffer.buffer = descr; > + ifr.ifr_buffer.length = descrlen; > + if (ioctl(s, SIOCGIFDESCR, &ifr) == 0) { > + if (strlen(descr) > 0) > + printf("\tdescription: %s\n", descr); Here, we seem to assume that the buffer is nul-terminated. > + break; > + } > + if (errno == ENAMETOOLONG) { > + descrlen *= 2; > + descr = reallocf(descr, descrlen); > + } > + } while (errno == ENAMETOOLONG); > + } Shouldn't we be erroring out if reallocf() fails? That seems to be normal practice elsewhere in ifconfig. > Modified: head/sys/net/if.c > ============================================================================== > --- head/sys/net/if.c Wed Nov 11 21:18:27 2009 (r199200) > +++ head/sys/net/if.c Wed Nov 11 21:30:58 2009 (r199201) > @@ -463,6 +463,8 @@ if_free_internal(struct ifnet *ifp) > #ifdef MAC > mac_ifnet_destroy(ifp); > #endif /* MAC */ > + if (ifp->if_description != NULL) > + sbuf_delete(ifp->if_description); > IF_AFDATA_DESTROY(ifp); > IF_ADDR_LOCK_DESTROY(ifp); > ifq_delete(&ifp->if_snd); > @@ -2090,6 +2092,45 @@ ifhwioctl(u_long cmd, struct ifnet *ifp, > ifr->ifr_phys = ifp->if_physical; > break; > > + case SIOCGIFDESCR: > + IF_AFDATA_RLOCK(ifp); > + if (ifp->if_description == NULL) > + error = ENOMSG; > + else > + error = copystr(sbuf_data(ifp->if_description), > + ifr->ifr_buffer.buffer, > + ifr->ifr_buffer.length, NULL); > + IF_AFDATA_RUNLOCK(ifp); Isn't copystr() for use only on kernel addresses, and isn't ifr_buffer a user address? And if this is a user address, you can't access it while the IF_AFDATA lock is held. > + case SIOCSIFDESCR: > + error = priv_check(td, PRIV_NET_SETIFDESCR); > + if (error) > + return (error); > + > + IF_AFDATA_WLOCK(ifp); This is not really what this lock is for, perhaps it's time to introduce an actual per-if lock. > + if (ifp->if_description == NULL) { > + ifp->if_description = sbuf_new_auto(); Can't do potentially sleeping memory allocations with the afdata lock held. > + if (ifp->if_description == NULL) { > + error = ENOMEM; > + IF_AFDATA_WUNLOCK(ifp); > + break; > + } > + } else > + sbuf_clear(ifp->if_description); > + > + if (sbuf_copyin(ifp->if_description, ifr->ifr_buffer.buffer, > + ifr->ifr_buffer.length) == -1) Access to user addresses while holding the afdata lock isn't allowed. Here, ifr_buffer.buffer is treated as non-nul-terminated. Some administrative limit on string length here would be appropriate; perhaps 128 bytes or 1024 bytes -- panicking because someone passes in a 1GB string by mistake in some C code is not what we want to do. > Modified: head/sys/net/if.h > ============================================================================== > --- head/sys/net/if.h Wed Nov 11 21:18:27 2009 (r199200) > +++ head/sys/net/if.h Wed Nov 11 21:30:58 2009 (r199201) > @@ -294,6 +294,7 @@ struct ifreq { > struct sockaddr ifru_addr; > struct sockaddr ifru_dstaddr; > struct sockaddr ifru_broadaddr; > + struct { size_t length; caddr_t buffer; } ifru_buffer; While ifreq already contains a pointer, this adds two more fields that vary in size across 32/64-bit, making it more difficult to support in 32-bit processes on a 64-bit kernel. > Modified: head/sys/net/if_var.h > ============================================================================== > --- head/sys/net/if_var.h Wed Nov 11 21:18:27 2009 (r199200) > +++ head/sys/net/if_var.h Wed Nov 11 21:30:58 2009 (r199201) > @@ -198,6 +198,7 @@ struct ifnet { > void *if_pf_kif; > void *if_lagg; /* lagg glue */ > u_char if_alloctype; /* if_type at time of allocation */ > + struct sbuf *if_description; /* interface description */ > > /* > * Spare fields are added so that we can modify sensitive data > @@ -205,7 +206,7 @@ struct ifnet { > * be used with care where binary compatibility is required. > */ > char if_cspare[3]; > - void *if_pspare[8]; > + void *if_pspare[7]; > int if_ispare[4]; > }; Could you confirm that ifnet hasn't changed size on arm, i386, and amd64? This looks like fairly dubious use of a spare field -- normally I'd expect to see if_cspare, which is 3 bytes long, remain after if_alloctype, and that pspare[0] be replaced in-place to prevent alignment decisions in the compiler from spacing out the structure further (i.e., adding another byte of padding on i386 and another 5 bytes of padding on amd64 after if_cspare and before if_pspare). Robert N M Watson Computer Laboratory University of Cambridge