From patchwork Tue Aug 20 03:18:21 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Al Viro X-Patchwork-Id: 268353 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 4E7982C010A for ; Tue, 20 Aug 2013 13:18:44 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751932Ab3HTDS2 (ORCPT ); Mon, 19 Aug 2013 23:18:28 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:52225 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751912Ab3HTDS1 (ORCPT ); Mon, 19 Aug 2013 23:18:27 -0400 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.76 #1 (Red Hat Linux)) id 1VBcSb-0006yI-Jg; Tue, 20 Aug 2013 03:18:21 +0000 Date: Tue, 20 Aug 2013 04:18:21 +0100 From: Al Viro To: Bjorn Helgaas Cc: Alex Williamson , "linux-pci@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "kvm@vger.kernel.org" , Benjamin Herrenschmidt , linux-fsdevel , Davide Libenzi , Tejun Heo Subject: Re: [PATCH] vfio-pci: PCI hot reset interface Message-ID: <20130820031821.GF27005@ZenIV.linux.org.uk> References: <20130814200845.21923.64284.stgit@bling.home> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Wed, Aug 14, 2013 at 04:42:14PM -0600, Bjorn Helgaas wrote: > [+cc Al, linux-fsdevel for fdget/fdput usage] fdget/fdput use looks sane, the only thing is that I would rather have an explicit include of linux/file.h instead of relying upon linux/eventfd.h pulling it. Incidentally, there are only 5 files that include the latter without an explicit include of the former - drivers/vfio/pci/vfio_pci.c, drivers/vhost/scsi.c, kernel/cgroup.c, mm/memcontrol.c and mm/vmpressure.c. And only kernel/cgroup.c (and, with this patch, vfio_pci.c) really wants anything from linux/file.h, so I'd rather kill that indirect include in eventfd.h and slapped an explicit include of file.h in these two files... BTW, most of the eventfd_fget() users might as well be using fget() (or fdget(), for that matter). They tend to be immediately followed by eventfd_ctx_fileget(), which repeats the "is that an eventfd file?" check anyway. Completely untested patch below does that to kernel/cgroup.c; Tejun, Davide - do you have any objections against the following? Kill indirect include of file.h from eventfd.h, use fdget() in cgroup.c kernel/cgroup.c is the only place in the tree that relies on eventfd.h pulling file.h; move that include there. Switch from eventfd_fget()/fput() to fdget()/fdput(), while we are at it - eventfd_ctx_fileget() will fail on non-eventfd descriptors just fine, no need to do that check twice... Signed-off-by: Al Viro --- -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/include/linux/eventfd.h b/include/linux/eventfd.h index cf5d2af..ff0b981 100644 --- a/include/linux/eventfd.h +++ b/include/linux/eventfd.h @@ -9,7 +9,6 @@ #define _LINUX_EVENTFD_H #include -#include #include /* @@ -26,6 +25,8 @@ #define EFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK) #define EFD_FLAGS_SET (EFD_SHARED_FCNTL_FLAGS | EFD_SEMAPHORE) +struct file; + #ifdef CONFIG_EVENTFD struct file *eventfd_file_create(unsigned int count, int flags); diff --git a/kernel/cgroup.c b/kernel/cgroup.c index 781845a..f88ecaf 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -60,6 +60,7 @@ #include #include /* used in cgroup_attach_task */ #include +#include #include @@ -3969,8 +3970,8 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft, struct cgroup_event *event = NULL; struct cgroup *cgrp_cfile; unsigned int efd, cfd; - struct file *efile = NULL; - struct file *cfile = NULL; + struct fd efile; + struct fd cfile; char *endp; int ret; @@ -3993,31 +3994,31 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft, init_waitqueue_func_entry(&event->wait, cgroup_event_wake); INIT_WORK(&event->remove, cgroup_event_remove); - efile = eventfd_fget(efd); - if (IS_ERR(efile)) { - ret = PTR_ERR(efile); - goto fail; + efile = fdget(efd); + if (!efile.file) { + ret = -EBADF; + goto fail1; } - event->eventfd = eventfd_ctx_fileget(efile); + event->eventfd = eventfd_ctx_fileget(efile.file); if (IS_ERR(event->eventfd)) { ret = PTR_ERR(event->eventfd); - goto fail; + goto fail2; } - cfile = fget(cfd); - if (!cfile) { + cfile = fdget(cfd); + if (!cfile.file) { ret = -EBADF; - goto fail; + goto fail3; } /* the process need read permission on control file */ /* AV: shouldn't we check that it's been opened for read instead? */ - ret = inode_permission(file_inode(cfile), MAY_READ); + ret = inode_permission(file_inode(cfile.file), MAY_READ); if (ret < 0) goto fail; - event->cft = __file_cft(cfile); + event->cft = __file_cft(cfile.file); if (IS_ERR(event->cft)) { ret = PTR_ERR(event->cft); goto fail; @@ -4027,7 +4028,7 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft, * The file to be monitored must be in the same cgroup as * cgroup.event_control is. */ - cgrp_cfile = __d_cgrp(cfile->f_dentry->d_parent); + cgrp_cfile = __d_cgrp(cfile.file->f_dentry->d_parent); if (cgrp_cfile != cgrp) { ret = -EINVAL; goto fail; @@ -4043,7 +4044,7 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft, if (ret) goto fail; - efile->f_op->poll(efile, &event->pt); + efile.file->f_op->poll(efile.file, &event->pt); /* * Events should be removed after rmdir of cgroup directory, but before @@ -4056,21 +4057,18 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft, list_add(&event->list, &cgrp->event_list); spin_unlock(&cgrp->event_list_lock); - fput(cfile); - fput(efile); + fdput(cfile); + fdput(efile); return 0; fail: - if (cfile) - fput(cfile); - - if (event && event->eventfd && !IS_ERR(event->eventfd)) - eventfd_ctx_put(event->eventfd); - - if (!IS_ERR_OR_NULL(efile)) - fput(efile); - + fdput(cfile); +fail3: + eventfd_ctx_put(event->eventfd); +fail2: + fdput(efile); +fail1: kfree(event); return ret;