Date: Wed, 18 Nov 2009 09:49:12 +0000 From: "Robert N. M. Watson" <rwatson@freebsd.org> To: Xin LI <delphij@gmail.com> Cc: FreeBSD Net <freebsd-net@FreeBSD.org>, Brooks Davis <brooks@freebsd.org>, John Baldwin <jhb@FreeBSD.org>, "M. Warner Losh" <imp@bsdimp.com>, Antoine Brodin <antoine.brodin@laposte.net> Subject: Re: [PATCH FOR REVIEW] interface description (revised) Message-ID: <01D9CB64-F04C-4506-ACF2-1DE459FC69CD@freebsd.org> In-Reply-To: <a78074950911171839u3e3fb4f1oae4aa3fc79f1b152@mail.gmail.com> References: <a78074950911171839u3e3fb4f1oae4aa3fc79f1b152@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?01D9CB64-F04C-4506-ACF2-1DE459FC69CD>