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>