svn commit: r199201 - in head: contrib/libpcap sbin/ifconfig
share/man/man4 sys/kern sys/net sys/sys
Robert Watson
rwatson at FreeBSD.org
Thu Nov 12 13:52:42 UTC 2009
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
More information about the svn-src-all
mailing list