From owner-svn-src-all@FreeBSD.ORG Wed Sep 18 18:46:56 2013 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 954E61C8; Wed, 18 Sep 2013 18:46:56 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wg0-x236.google.com (mail-wg0-x236.google.com [IPv6:2a00:1450:400c:c00::236]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id B7C382E17; Wed, 18 Sep 2013 18:46:55 +0000 (UTC) Received: by mail-wg0-f54.google.com with SMTP id m15so7102678wgh.9 for ; Wed, 18 Sep 2013 11:46:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=g4525PR1YmpQ83tSykadVTKmP5Lt1uaXnQmp+2DZqvA=; b=eX4+G539bKr74vBUUHhosQyC1W3HHFh82zlLd7YyvhiL06lPBOte55isiZjniInRa2 VIfudH1aan9qWRArNIiSBdsSWbCGOXcMXr0IF2g+sACWlQ+9z2DiDHymkdgApjTJq6jQ j7u5GNFkD/C78zR8gnQYn268lBMqmUFRTeQ/yxpJaB3Sgsttoibj3zkL0U4HFab7hzvn Fvf9HYjS6kfzMvwmNlavhXyU/pPQ6f0XT/xehkOZz/FQMvxFKj6jnomYAkP/Dk8bd+c/ aOTW5d9ZMDvfgXRj4EfU22eYryLvPr4Ozlna5K1+wcVVX8uU1rYUuXTfMgAzmr4RpCOO 6MOg== X-Received: by 10.194.222.2 with SMTP id qi2mr33668560wjc.14.1379530014087; Wed, 18 Sep 2013 11:46:54 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id dx7sm4346979wib.8.1969.12.31.16.00.00 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 18 Sep 2013 11:46:53 -0700 (PDT) Date: Wed, 18 Sep 2013 20:46:48 +0200 From: Mateusz Guzik To: Roman Divacky Subject: Re: svn commit: r255672 - in head/sys: amd64/linux32 compat/linux conf i386/linux kern modules/linux sys Message-ID: <20130918184648.GA31748@dft-labs.eu> References: <201309181756.r8IHu4qV052882@svn.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <201309181756.r8IHu4qV052882@svn.freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, yuri@rawbw.com, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Sep 2013 18:46:56 -0000 On Wed, Sep 18, 2013 at 05:56:04PM +0000, Roman Divacky wrote: > Author: rdivacky > Date: Wed Sep 18 17:56:04 2013 > New Revision: 255672 > URL: http://svnweb.freebsd.org/changeset/base/255672 > > Log: > Implement epoll support in Linuxulator. This is a tiny wrapper around kqueue > to implement epoll subset of functionality. The kqueue user data are 32bit > on i386 which is not enough for epoll user data so this patch overrides > kqueue fileops to maintain enough space in struct file. > > Initial patch developed by me in 2007 and then extended and finished > by Yuri Victorovich. > First of all thank you both for doing this work. I have some important though (I didn't spend too much on this, so maybe I missed some stuff or what I write here is incorrect). In general some lines are longer than 80 characters and simple stile violations are present ("if (!error)"). > +/* Create a new epoll file descriptor. */ > + > +static int > +linux_epoll_create_common(struct thread *td) > +{ > + struct file *fp; > + int error; > + > + error = kern_kqueue_locked(td, &fp); > +#if EPOLL_WIDE_USER_DATA > + if (error == 0) { > + epoll_init_user_data(td, fp); > + fdrop(fp, td); > + } > +#endif > + return (error); > +} This leaks fd reference if EPOLL_WIDE_USER_DATA is not defined. > +int > +linux_epoll_create1(struct thread *td, struct linux_epoll_create1_args *args) > +{ > + int error; > + > + error = linux_epoll_create_common(td); > + > + if (!error) { > + if (args->flags & LINUX_EPOLL_CLOEXEC) > + td->td_proc->p_fd->fd_ofiles[td->td_retval[0]].fde_flags |= UF_EXCLOSE; This is very racy for no good reason. This should be passed down to kqueue and be set on fd creation. > + if (args->flags & LINUX_EPOLL_NONBLOCK) > + linux_msg(td, "epoll_create1 doesn't yet support EPOLL_NONBLOCK flag\n"); > + } > + > + return (error); > +} > + > + > +static void > +epoll_init_user_data(struct thread *td, struct file *epfp) > +{ > + struct epoll_user_data *udv; > + > + /* override file ops to have our close operation */ > + atomic_store_rel_ptr((volatile uintptr_t *)&epfp->f_ops, (uintptr_t)&epollops); > + > + /* allocate epoll_user_data initially for up to 16 file descriptor values */ > + udv = malloc(EPOLL_USER_DATA_SIZE(EPOLL_USER_DATA_MARGIN), M_LINUX_EPOLL, M_WAITOK); > + udv->sz = EPOLL_USER_DATA_MARGIN; > + EPOLL_USER_DATA_SET(epfp, udv); > +} Is not this racy? There is a window when fd is installed with epoll ops, yet no userdata is allocated. > +/*ARGSUSED*/ > +static int > +epoll_close(struct file *epfp, struct thread *td) > +{ > + /* free user data vector */ > + free(EPOLL_USER_DATA_GET(epfp), M_LINUX_EPOLL); > + /* over to kqueue parent */ > + return (kqueue_close(epfp, td)); > +} > +#endif Unnecessary comments. > + > +static struct file* > +epoll_fget(struct thread *td, int epfd) > +{ > + struct file *fp; > + cap_rights_t rights; > + > + if (fget(td, epfd, cap_rights_init(&rights, CAP_POLL_EVENT), &fp) != 0) > + panic("epoll: no file object found for kqueue descriptor"); > + > + return (fp); > +} > + Callers pass arbitrary fd here (provided by the user), yet this panics if fd is bad. > int > +kern_kqueue(struct thread *td) > +{ > + struct file *fp; > + int error; > + > + error = kern_kqueue_locked(td, &fp); > + Why is this _locked? Typically such naming is used when some locks are held around the call. > + fdrop(fp, td); If there was an error, fdrop is called even though there is nothing to fdrop. > + return (error); > +} -- Mateusz Guzik