Date: Wed, 18 Nov 2009 10:12:06 -0500 From: John Baldwin <jhb@freebsd.org> To: "Robert N. M. Watson" <rwatson@freebsd.org> Cc: FreeBSD Net <freebsd-net@freebsd.org>, Brooks Davis <brooks@freebsd.org>, "M. Warner Losh" <imp@bsdimp.com>, Antoine Brodin <antoine.brodin@laposte.net> Subject: Re: [PATCH FOR REVIEW] interface description (revised) Message-ID: <200911181012.07167.jhb@freebsd.org> In-Reply-To: <01D9CB64-F04C-4506-ACF2-1DE459FC69CD@freebsd.org> References: <a78074950911171839u3e3fb4f1oae4aa3fc79f1b152@mail.gmail.com> <01D9CB64-F04C-4506-ACF2-1DE459FC69CD@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday 18 November 2009 4:49:12 am Robert N. M. Watson wrote:
> On 18 Nov 2009, at 02:39, Xin LI wrote:
> > + 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.
I've found the 'descr' shortcut useful when using a similar patch of this on 7
FWIW.
> > + case SIOCGIFDESCR:
> > + error = 0;
> > + if (ifr->ifr_buffer.length > ifdescr_maxlen) {
> > + error = 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.
I would use either of the following strategies:
1) Don't check ifdescr_maxlen when fetching a description, only when setting
it. If a sysadmin decides to shorten the maximum description length, then
they should perhaps shorten any really long descriptions to match. In
practice I suspect that sysadmins will not change this on the fly and this
policy gives the sanest user experience IMO.
2) Truncate the description copied out to ifdescr_maxlen chars.
Also, I'm not sure that using an sbuf rather than a plain char * is actually
buying you anything here. You aren't building a string which sbuf is good
for. Instead, you are just copying strings around. I would probably not use
sbuf at all and would just use copyin/copyout.
One issue with 1) for the current code is that you are using it to avoid
having userland ask for a really large buffer. Instead, I would change the
code to just do something like this (assuming you have a char *):
char *buf;
size_t len;
case SIOCFIGDESCR:
error = 0;
IF_AFDATA_RLOCK(ifp);
for (buf = NULL; buf; ) {
if (ifp->if_description == NULL) {
error = ENOMSG;
break;
}
len = strlen(ifp->if_description) + 1;
IF_AFDATA_RUNLOCK(ifp);
buf = malloc(len, M_TEMP, M_WAITOK);
IF_AFDATA_RLOCK(ifp);
if (len < strlen(description + 1) {
free(buf, M_TEMP);
buf = NULL;
}
}
if (error == 0)
strcpy(buf, ifp->if_desciption);
IF_AFDATA_RUNLOCK(ifp);
if (error == 0) {
if (len > ifr->ifr_buffer.length)
error = ENAMETOOLONG;
else
error = copyout(buf, ifr->ifr_buffer.buffer,
len);
}
if (buf != NULL)
free(buf, M_TEMP);
break;
However, this is a bit complicated, and to be honest, I don't think interface
descriptions are a critical path. Robert has already said before that
IF_AFDATA_RLOCK() isn't really the "correct" lock but is being abused for
this. Given that, I would probably just add a single global sx lock. This
has the added advantage that you can just use copyin/copyout directly and
skip all the extra complication. I don't think we need the extra concurrency
for interface descriptions to make this so complicated. If you used a global
sx lock with a simple string for descriptions, the code would end up looking
like this:
static struct sx ifdescr_lock;
SX_SYSINIT(&ifdescr_lock, "ifnet descr");
static MALLOC_DEFINE(M_IFDESCR, "ifdescr", "ifnet descriptions");
char *buf;
size_t len;
case SIOCGIFDESCR:
error = 0;
sx_slock(&ifdescr_lock);
if (ifp->if_description == NULL)
error = ENOMSG;
else {
len = strlen(ifp->if_description) + 1;
if (ifr->ifr_buffer.length < len)
error = ENAMETOOLONG;
else
error = copyout(ifr->ifr_buffer.buffer,
ifp->if_description, len);
}
sx_sunlock(&ifdescr_lock);
break;
case SIOCSIFDESCR:
error = priv_check();
if (error)
break;
if (ifr->ifr_buffer.length > ifdescr_maxlen)
return (ENAMETOOLONG);
buf = malloc(ifr->ifr_buffer.length, M_IFDESCR, M_WAITOK |
M_ZERO);
error = copyin(ifr->ifr_buffer.buffer, buf,
ifr->ifr_buffer.length - 1);
if (error) {
free(buf, M_IFDESCR);
break;
}
sx_xlock(&ifdescr_lock);
ifp->if_description = buf;
sx_xunlock(&ifdescr_lock);
break;
Note that this takes approach 1) from above, but it is also a moot point now
since the 'get' ioctl doesn't allocate memory anymore.
> > 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.
I wouldn't worry about 32-bit compat for this ioctl. The main consumer of
this ioctl is going to be ifconfig which will be a native binary. If someone
encounters a situation where they need 32-bit compat, then it can be added at
that time.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200911181012.07167.jhb>