From owner-freebsd-net@FreeBSD.ORG Wed Nov 18 09:49:18 2009 Return-Path: Delivered-To: freebsd-net@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 27CE01065679; Wed, 18 Nov 2009 09:49:18 +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 C84398FC13; Wed, 18 Nov 2009 09:49:17 +0000 (UTC) Received: from [192.168.2.101] (host217-43-176-60.range217-43.btcentralplus.com [217.43.176.60]) by cyrus.watson.org (Postfix) with ESMTPSA id 5CB3A46B65; Wed, 18 Nov 2009 04:49:15 -0500 (EST) Mime-Version: 1.0 (Apple Message framework v1077) From: "Robert N. M. Watson" In-Reply-To: Date: Wed, 18 Nov 2009 09:49:12 +0000 Message-Id: <01D9CB64-F04C-4506-ACF2-1DE459FC69CD@freebsd.org> References: To: Xin LI X-Mailer: Apple Mail (2.1077) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.5 Cc: FreeBSD Net , Brooks Davis , John Baldwin , "M. Warner Losh" , Antoine Brodin Subject: Re: [PATCH FOR REVIEW] interface description (revised) X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Nov 2009 09:49:18 -0000 On 18 Nov 2009, at 02:39, Xin LI wrote: > Here is the revised implementation for the interface description > feature, based on feedback from src-all@. Hi Xin Li, Thanks for the updated patch. This looks significantly improved. = Comments inline. > --- contrib/libpcap/inet.c (revision 199463) > +++ contrib/libpcap/inet.c (working copy) > @@ -403,22 +403,30 @@ add_addr_to_iflist(pcap_if_t **alldevs, const = char > pcap_addr_t *curaddr, *prevaddr, *nextaddr; > #ifdef SIOCGIFDESCR > struct ifreq ifrdesc; > +#ifndef IFDESCRSIZE > +#define _IFDESCRSIZE 64 > + char ifdescr[_IFDESCRSIZE]; > +#else > char ifdescr[IFDESCRSIZE]; > +#endif > int s; > -#endif > =20 > -#ifdef SIOCGIFDESCR I'm pretty sure the intent here is that 'int s' be defined regardless of = SIOCGIFDESCR, it would be worth confirming that these patches applied to = a pre-SIOCGIFDESCR tcpdump compile correctly. We will want to upstream = these patches to the vendor so we should make sure the changes are = acceptable there. > + descr =3D reallocf(descr, descrlen); > + if (descr !=3D NULL) { > + do { > + ifr.ifr_buffer.buffer =3D descr; > + ifr.ifr_buffer.length =3D descrlen; > + if (ioctl(s, SIOCGIFDESCR, &ifr) =3D=3D 0) { > + if (strlen(descr) > 0) > + printf("\tdescription: %s\n", descr); > + break; > + } > + if (errno =3D=3D ENAMETOOLONG) { > + descrlen *=3D 2; > + descr =3D reallocf(descr, descrlen); > + } > + } while ((errno =3D=3D ENAMETOOLONG) && (descr !=3D = NULL)); > + } The error non-handling throughout ifconfig worries me; on the whole, = your patch seems consistent with the existing model, but here I wonder = if we should be printing a warning if reallocf() fails? Perhaps the = above loop could be restructured so that there is only a single calling = point to reallocf -- perhaps while ((descr =3D reallocf()) !=3D NULL) { = ... }? It might make the invariants a bit more clear. > + DEF_CMD_ARG("description", setifdescr), > + DEF_CMD_ARG("descr", setifdescr), > + DEF_CMD("-description", 0, unsetifdescr), > + DEF_CMD("-descr", 0, unsetifdescr), Does having two undocumented short-form aliases make this more usable? = We should either document them or not have them, I guess. > -.Dd June 18, 2004 > +.Dd November 26, 2009 Curious choice of dates. :-) > +.It Dv SIOCGIFDESCR > +Get the interface description, returned in the > +.Va buffer > +field of > +.Va ifru_buffer > +struct. > +The user supplied buffer length should defined in the > +.Va length > +field of > +.Va ifru_buffer > +struct passed in as parameter. > +.It Dv SIOCSIFDESCR > +Set the interface description to the value of the > +.Va buffer > +field of > +.Va ifru_buffer > +struct, with > +.Va length > +field specifying its length. No mention of nul's in the man page yet, but the code now seems much = more consistent about including nul's throughout. > + case SIOCGIFDESCR: > + error =3D 0; > + if (ifr->ifr_buffer.length > ifdescr_maxlen) { > + error =3D ENOMEM; > + break; > + } I have three worries about this comparison: (1) ifdescr_maxlen is signed, perhaps it should be unsigned? (2) ifdescr_maxlen could be reduced between SIOCSIFDESCR and = SIOCGIFDESCR, in which case you can no longer query an interface = description even though the kernel is still storing it. (3) The loop logic in the userland ifconfig tool assumes that it is OK = to ask for more bytes than the largest description, so it doubles the = buffer each time it loops. This means that it may overshoot = ifdescr_maxlen leading to the call failing even though a large enough = buffer has been passed. One of the reasons that potentially unbounded kernel buffers are so = awkward is that they lead to live-locky logic looping trying to grow a = buffer using sleeping allocation while grabbing and releasing locks = trying to get a buffer that's large enough. Most of the time the other = theoretical thread you're racing with won't exist, but this still has to = be handled because someday it will. I suggest a loop in which you create = an sbuf to match the current length of ifp->if_description, and then = after the loop ends and you have a copy, see if it fits in userspace. = 99.9999% of the time, userspace will have passed a big enough buffer, = and the loop won't be exercised growing the kernel buffer. > + else { > + if (ifr->ifr_buffer.length <=3D = sbuf_len(ifp->if_description)) > + error =3D ENAMETOOLONG; This may be more clear if it's written as "< = sbuf_len(ifp->if_description) + 1". However, if you make the above = changes, this goes away, or is at least refactored. > + /* > + * Copy 1 more byte to make sure that the = copyout is NUL Often in the BSD source code, the character is 'nul' and the pointer is = 'NULL'. > --- sys/net/if_var.h (revision 199463) > +++ sys/net/if_var.h (working copy) > @@ -205,7 +205,8 @@ struct ifnet { > * be used with care where binary compatibility is required. > */ > char if_cspare[3]; > - void *if_pspare[8]; > + void *if_pspare[7]; > + struct sbuf *if_description; /* interface description */ > int if_ispare[4]; > }; I think the conclusion here was that the ifnet spares were a mistake, = but I'm not sure how we decided to resolve that mistake. Should we first = remove the spares in one commit, and then just append new fields in a = separate commit? (Brooks?) > Some limitations: > * Not yet able to send announce through route socket. I need to > figure out a proper way to do this, maybe a future feature; > * 32-bit vs 64-bit API compatibility. Since the kernel has to copy > in a string, is there a clean way to do this? I think we will also > need to deal with similar issue with SIOCSIFNAME as well. I'm not sure there's a clean way to deal with it; pointers embedded in = ioctl arguments are becoming more of a problem, so I wonder if the = answer isn't to stop introducing any new ones. The Mac OS X kernel is a = bit more thorough than us in implementing ioctls, since they more = aggressively select kernel based on hardware, and contains a lot of = fairly awkward compatibility code switching on whether the process is a = 32-bit or 64-bit process and then selecting the right data structure. Maybe John has some thoughts on how to handle that. Robert=