Date: Wed, 18 Nov 2009 10:44:08 -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: <200911181044.08772.jhb@freebsd.org> In-Reply-To: <70A1A5B3-1F19-4AC1-8975-F7E950785BBF@freebsd.org> References: <a78074950911171839u3e3fb4f1oae4aa3fc79f1b152@mail.gmail.com> <200911181012.07167.jhb@freebsd.org> <70A1A5B3-1F19-4AC1-8975-F7E950785BBF@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200911181044.08772.jhb>