From owner-freebsd-net@FreeBSD.ORG Wed Nov 18 15:44:22 2009 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5EFA3106566C; Wed, 18 Nov 2009 15:44:22 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 198AB8FC19; Wed, 18 Nov 2009 15:44:22 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id B5E0B46B39; Wed, 18 Nov 2009 10:44:21 -0500 (EST) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id 0460B8A021; Wed, 18 Nov 2009 10:44:21 -0500 (EST) From: John Baldwin To: "Robert N. M. Watson" Date: Wed, 18 Nov 2009 10:44:08 -0500 User-Agent: KMail/1.9.7 References: <200911181012.07167.jhb@freebsd.org> <70A1A5B3-1F19-4AC1-8975-F7E950785BBF@freebsd.org> In-Reply-To: <70A1A5B3-1F19-4AC1-8975-F7E950785BBF@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200911181044.08772.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Wed, 18 Nov 2009 10:44:21 -0500 (EST) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: FreeBSD Net , Brooks Davis , "M. Warner Losh" , Antoine Brodin Subject: Re: [PATCH FOR REVIEW] interface description (revised) X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Nov 2009 15:44:22 -0000 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