[PATCH FOR REVIEW] interface description (revised)

[John Baldwin]

On Wednesday 18 November 2009 10:35:36 am Robert N. M. Watson wrote:
> 
> On 18 Nov 2009, at 15:12, John Baldwin wrote:
> 
> > 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");
> 
> This strikes me as a good idea -- there won't be much real-world contention on
> the lock, I'd think, and this greatly simplifies the code. We might think about
> whether there's other, currently under-synchronized, ifnet meta-data that could
> be protected usefully with the same lock.   
> 
> > 	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.
> 
> This code seems pretty reasonable to me, but there's a race here if two threads
> try to use the set ioctl on the same interface at once. We should test whether
> ifp->if_description is already set after the sx_xlock() above, and swap pointers,
> freeing the old one after xunlock(), if so.

Actually, it's just a straight up bug.  The race with two threads doing a set is
a userland race, but what I missed was freeing the old buffer if it existed.  One
would just modify the end of SIFDESCR case as follows:

	char *old;

		sx_xlock(&ifdescr_lock);
		old = ifp->if_description;
		ifp->if_description = buf;
		sx_xunlock(&ifdescr_lock);
		free(old, M_IFDESCR);
		break;

-- 
John Baldwin