[PATCH FOR REVIEW] interface description (revised)

[John Baldwin]

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