Add a __anon_inode_getfd helper

Add a __anon_inode_getfd helper

[Christoph Hellwig]

Hi Al,

this series (against your work.epoll branch), adds a new
__anon_inode_getfd helper, which exposes the functionality in
anon_inode_getfd minus installing the file descriptor.  This
allows to clean up a lot of the places that currently open code
the functionality.

[PATCH 01/12] fd: add a new __anon_inode_getfd helper

[Christoph Hellwig]

Add a helper that is equivalent to anon_inode_getfd minus the final
fd_install.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/anon_inodes.c            | 41 ++++++++++++++++++++++---------------
 include/linux/anon_inodes.h |  2 ++
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 89714308c25b8..1b2acd2bbaa32 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -106,6 +106,26 @@ struct file *anon_inode_getfile(const char *name,
 }
 EXPORT_SYMBOL_GPL(anon_inode_getfile);
 
+int __anon_inode_getfd(const char *name, const struct file_operations *fops,
+		     void *priv, int flags, struct file **file)
+{
+	int fd;
+
+	fd = get_unused_fd_flags(flags);
+	if (fd < 0)
+		return fd;
+
+	*file = anon_inode_getfile(name, fops, priv, flags);
+	if (IS_ERR(*file))
+		goto err_put_unused_fd;
+	return fd;
+
+err_put_unused_fd:
+	put_unused_fd(fd);
+	return PTR_ERR(*file);
+}
+EXPORT_SYMBOL_GPL(__anon_inode_getfd);
+
 /**
  * anon_inode_getfd - creates a new file instance by hooking it up to an
  *                    anonymous inode, and a dentry that describe the "class"
@@ -125,26 +145,13 @@ EXPORT_SYMBOL_GPL(anon_inode_getfile);
 int anon_inode_getfd(const char *name, const struct file_operations *fops,
 		     void *priv, int flags)
 {
-	int error, fd;
 	struct file *file;
+	int fd;
 
-	error = get_unused_fd_flags(flags);
-	if (error < 0)
-		return error;
-	fd = error;
-
-	file = anon_inode_getfile(name, fops, priv, flags);
-	if (IS_ERR(file)) {
-		error = PTR_ERR(file);
-		goto err_put_unused_fd;
-	}
-	fd_install(fd, file);
-
+	fd = __anon_inode_getfd(name, fops, priv, flags, &file);
+	if (fd >= 0)
+		fd_install(fd, file);
 	return fd;
-
-err_put_unused_fd:
-	put_unused_fd(fd);
-	return error;
 }
 EXPORT_SYMBOL_GPL(anon_inode_getfd);
 
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index d0d7d96261adf..338449d06b617 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -16,6 +16,8 @@ struct file *anon_inode_getfile(const char *name,
 				void *priv, int flags);
 int anon_inode_getfd(const char *name, const struct file_operations *fops,
 		     void *priv, int flags);
+int __anon_inode_getfd(const char *name, const struct file_operations *fops,
+		     void *priv, int flags, struct file **file);
 
 #endif /* _LINUX_ANON_INODES_H */
 
-- 
2.26.2

[PATCH 02/12] kvm: use __anon_inode_getfd

[Christoph Hellwig]

Use __anon_inode_getfd instead of opencoding the logic using
get_unused_fd_flags + anon_inode_getfile.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 virt/kvm/kvm_main.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 74bdb7bf32952..d20a7c2a30f1d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3822,17 +3822,11 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	if (r < 0)
 		goto put_kvm;
 #endif
-	r = get_unused_fd_flags(O_CLOEXEC);
+	r = __anon_inode_getfd("kvm-vm", &kvm_vm_fops, kvm, O_CLOEXEC | O_RDWR,
+			&file);
 	if (r < 0)
 		goto put_kvm;
 
-	file = anon_inode_getfile("kvm-vm", &kvm_vm_fops, kvm, O_RDWR);
-	if (IS_ERR(file)) {
-		put_unused_fd(r);
-		r = PTR_ERR(file);
-		goto put_kvm;
-	}
-
 	/*
 	 * Don't call kvm_put_kvm anymore at this point; file->f_op is
 	 * already set, with ->release() being kvm_vm_release().  In error
-- 
2.26.2

[PATCH 03/12] pidfd: use __anon_inode_getfd

[Christoph Hellwig]

Use __anon_inode_getfd instead of opencoding the logic using
get_unused_fd_flags + anon_inode_getfile.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/fork.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 4385f3d639f23..31e0face01072 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2113,19 +2113,11 @@ static __latent_entropy struct task_struct *copy_process(
 	 * if the fd table isn't shared).
 	 */
 	if (clone_flags & CLONE_PIDFD) {
-		retval = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
+		retval = __anon_inode_getfd("[pidfd]", &pidfd_fops, pid,
+				O_RDWR | O_CLOEXEC, &pidfile);
 		if (retval < 0)
 			goto bad_fork_free_pid;
-
 		pidfd = retval;
-
-		pidfile = anon_inode_getfile("[pidfd]", &pidfd_fops, pid,
-					      O_RDWR | O_CLOEXEC);
-		if (IS_ERR(pidfile)) {
-			put_unused_fd(pidfd);
-			retval = PTR_ERR(pidfile);
-			goto bad_fork_free_pid;
-		}
 		get_pid(pid);	/* held by pidfile now */
 
 		retval = put_user(pidfd, args->pidfd);
-- 
2.26.2

[PATCH 04/12] bpf: use __anon_inode_getfd

[Christoph Hellwig]

Use __anon_inode_getfd instead of opencoding the logic using
get_unused_fd_flags + anon_inode_getfile.  Also switch the
bpf_link_new_file calling conventions to match __anon_inode_getfd.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/bpf.h  |  2 +-
 kernel/bpf/cgroup.c  |  6 +++---
 kernel/bpf/syscall.c | 31 +++++++++----------------------
 3 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index fd2b2322412d7..539649fe8b885 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1103,7 +1103,7 @@ void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
 void bpf_link_inc(struct bpf_link *link);
 void bpf_link_put(struct bpf_link *link);
 int bpf_link_new_fd(struct bpf_link *link);
-struct file *bpf_link_new_file(struct bpf_link *link, int *reserved_fd);
+int bpf_link_new_file(struct bpf_link *link, struct file **link_file);
 struct bpf_link *bpf_link_get_from_fd(u32 ufd);
 
 int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index cb305e71e7deb..8605287adcd9e 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -836,10 +836,10 @@ int cgroup_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 	link->cgroup = cgrp;
 	link->type = attr->link_create.attach_type;
 
-	link_file = bpf_link_new_file(&link->link, &link_fd);
-	if (IS_ERR(link_file)) {
+	link_fd = bpf_link_new_file(&link->link, &link_file);
+	if (link_fd < 0) {
 		kfree(link);
-		err = PTR_ERR(link_file);
+		err = link_fd;
 		goto out_put_cgroup;
 	}
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 64783da342020..cb2364e17423c 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2307,23 +2307,10 @@ int bpf_link_new_fd(struct bpf_link *link)
  * complicated and expensive operations and should be delayed until all the fd
  * reservation and anon_inode creation succeeds.
  */
-struct file *bpf_link_new_file(struct bpf_link *link, int *reserved_fd)
+int bpf_link_new_file(struct bpf_link *link, struct file **file)
 {
-	struct file *file;
-	int fd;
-
-	fd = get_unused_fd_flags(O_CLOEXEC);
-	if (fd < 0)
-		return ERR_PTR(fd);
-
-	file = anon_inode_getfile("bpf_link", &bpf_link_fops, link, O_CLOEXEC);
-	if (IS_ERR(file)) {
-		put_unused_fd(fd);
-		return file;
-	}
-
-	*reserved_fd = fd;
-	return file;
+	return __anon_inode_getfd("bpf_link", &bpf_link_fops, link, O_CLOEXEC,
+			file);
 }
 
 struct bpf_link *bpf_link_get_from_fd(u32 ufd)
@@ -2406,10 +2393,10 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 	}
 	bpf_link_init(&link->link, &bpf_tracing_link_lops, prog);
 
-	link_file = bpf_link_new_file(&link->link, &link_fd);
-	if (IS_ERR(link_file)) {
+	link_fd = bpf_link_new_file(&link->link, &link_file);
+	if (link_fd < 0) {
 		kfree(link);
-		err = PTR_ERR(link_file);
+		err = link_fd;
 		goto out_put_prog;
 	}
 
@@ -2520,10 +2507,10 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
 	bpf_link_init(&link->link, &bpf_raw_tp_lops, prog);
 	link->btp = btp;
 
-	link_file = bpf_link_new_file(&link->link, &link_fd);
-	if (IS_ERR(link_file)) {
+	link_fd = bpf_link_new_file(&link->link, &link_file);
+	if (link_fd < 0) {
 		kfree(link);
-		err = PTR_ERR(link_file);
+		err = link_fd;
 		goto out_put_btp;
 	}
 
-- 
2.26.2

Re: [PATCH 04/12] bpf: use __anon_inode_getfd

[Andrii Nakryiko]

On Fri, May 8, 2020 at 8:39 AM Christoph Hellwig <hch@lst.de> wrote:
>
> Use __anon_inode_getfd instead of opencoding the logic using
> get_unused_fd_flags + anon_inode_getfile.  Also switch the
> bpf_link_new_file calling conventions to match __anon_inode_getfd.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/bpf.h  |  2 +-
>  kernel/bpf/cgroup.c  |  6 +++---
>  kernel/bpf/syscall.c | 31 +++++++++----------------------
>  3 files changed, 13 insertions(+), 26 deletions(-)
>

[...]

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 64783da342020..cb2364e17423c 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2307,23 +2307,10 @@ int bpf_link_new_fd(struct bpf_link *link)
>   * complicated and expensive operations and should be delayed until all the fd
>   * reservation and anon_inode creation succeeds.
>   */

The comment above explains the reason why we do want to split getting
fd, getting file, and installing fd later. I'd like to keep it this
way. Also, this code was refactored in bpf-next by [0] (it still uses
get_unused_fd_flag + anon_inode_getfile + fd_install, by design).

  [0] https://patchwork.ozlabs.org/project/netdev/patch/20200429001614.1544-3-andriin@fb.com/

> -struct file *bpf_link_new_file(struct bpf_link *link, int *reserved_fd)
> +int bpf_link_new_file(struct bpf_link *link, struct file **file)
>  {
> -       struct file *file;
> -       int fd;
> -
> -       fd = get_unused_fd_flags(O_CLOEXEC);
> -       if (fd < 0)
> -               return ERR_PTR(fd);
> -
> -       file = anon_inode_getfile("bpf_link", &bpf_link_fops, link, O_CLOEXEC);
> -       if (IS_ERR(file)) {
> -               put_unused_fd(fd);
> -               return file;
> -       }
> -
> -       *reserved_fd = fd;
> -       return file;
> +       return __anon_inode_getfd("bpf_link", &bpf_link_fops, link, O_CLOEXEC,
> +                       file);
>  }
>

[...]

Re: [PATCH 04/12] bpf: use __anon_inode_getfd

[Andrii Nakryiko]

On Fri, May 8, 2020 at 8:39 AM Christoph Hellwig <hch@lst.de> wrote:
>
> Use __anon_inode_getfd instead of opencoding the logic using
> get_unused_fd_flags + anon_inode_getfile.  Also switch the
> bpf_link_new_file calling conventions to match __anon_inode_getfd.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/bpf.h  |  2 +-
>  kernel/bpf/cgroup.c  |  6 +++---
>  kernel/bpf/syscall.c | 31 +++++++++----------------------
>  3 files changed, 13 insertions(+), 26 deletions(-)
>

[...]

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 64783da342020..cb2364e17423c 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2307,23 +2307,10 @@ int bpf_link_new_fd(struct bpf_link *link)
>   * complicated and expensive operations and should be delayed until all the fd
>   * reservation and anon_inode creation succeeds.
>   */

The comment above explains the reason why we do want to split getting
fd, getting file, and installing fd later. I'd like to keep it this
way. Also, this code was refactored in bpf-next by [0] (it still uses
get_unused_fd_flag + anon_inode_getfile + fd_install, by design).

  [0] https://patchwork.ozlabs.org/project/netdev/patch/20200429001614.1544-3-andriin@fb.com/

> -struct file *bpf_link_new_file(struct bpf_link *link, int *reserved_fd)
> +int bpf_link_new_file(struct bpf_link *link, struct file **file)
>  {
> -       struct file *file;
> -       int fd;
> -
> -       fd = get_unused_fd_flags(O_CLOEXEC);
> -       if (fd < 0)
> -               return ERR_PTR(fd);
> -
> -       file = anon_inode_getfile("bpf_link", &bpf_link_fops, link, O_CLOEXEC);
> -       if (IS_ERR(file)) {
> -               put_unused_fd(fd);
> -               return file;
> -       }
> -
> -       *reserved_fd = fd;
> -       return file;
> +       return __anon_inode_getfd("bpf_link", &bpf_link_fops, link, O_CLOEXEC,
> +                       file);
>  }
>

[...]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/12] io_uring: use __anon_inode_getfd

[Christoph Hellwig]

Use __anon_inode_getfd instead of opencoding the logic using
get_unused_fd_flags + anon_inode_getfile.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/io_uring.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 5190bfb6a6657..4104f8a45d11e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7709,18 +7709,11 @@ static int io_uring_get_fd(struct io_ring_ctx *ctx)
 		return ret;
 #endif
 
-	ret = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
+	ret = __anon_inode_getfd("[io_uring]", &io_uring_fops, ctx,
+			O_RDWR | O_CLOEXEC, &file);
 	if (ret < 0)
 		goto err;
 
-	file = anon_inode_getfile("[io_uring]", &io_uring_fops, ctx,
-					O_RDWR | O_CLOEXEC);
-	if (IS_ERR(file)) {
-		put_unused_fd(ret);
-		ret = PTR_ERR(file);
-		goto err;
-	}
-
 #if defined(CONFIG_UNIX)
 	ctx->ring_sock->file = file;
 #endif
-- 
2.26.2

[PATCH 06/12] eventpoll: use __anon_inode_getfd

[Christoph Hellwig]

Use __anon_inode_getfd instead of opencoding the logic using
get_unused_fd_flags + anon_inode_getfile.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/eventpoll.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 8c596641a72b0..8abdb9fff611a 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -2055,23 +2055,14 @@ static int do_epoll_create(int flags)
 	 * Creates all the items needed to setup an eventpoll file. That is,
 	 * a file structure and a free file descriptor.
 	 */
-	fd = get_unused_fd_flags(O_RDWR | (flags & O_CLOEXEC));
-	if (fd < 0) {
-		error = fd;
+	fd = __anon_inode_getfd("[eventpoll]", &eventpoll_fops, ep,
+				 O_RDWR | (flags & O_CLOEXEC), &file);
+	if (fd < 0)
 		goto out_free_ep;
-	}
-	file = anon_inode_getfile("[eventpoll]", &eventpoll_fops, ep,
-				 O_RDWR | (flags & O_CLOEXEC));
-	if (IS_ERR(file)) {
-		error = PTR_ERR(file);
-		goto out_free_fd;
-	}
 	ep->file = file;
 	fd_install(fd, file);
 	return fd;
 
-out_free_fd:
-	put_unused_fd(fd);
 out_free_ep:
 	ep_free(ep);
 	return error;
-- 
2.26.2

[PATCH 07/12] eventfd: use __anon_inode_getfd

[Christoph Hellwig]

Use __anon_inode_getfd instead of opencoding the logic using
get_unused_fd_flags + anon_inode_getfile.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/eventfd.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/fs/eventfd.c b/fs/eventfd.c
index df466ef81dddf..9b6d5137679b2 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -423,20 +423,11 @@ static int do_eventfd(unsigned int count, int flags)
 	ctx->count = count;
 	ctx->flags = flags;
 	ctx->id = ida_simple_get(&eventfd_ida, 0, 0, GFP_KERNEL);
-
-	flags &= EFD_SHARED_FCNTL_FLAGS;
-	flags |= O_RDWR;
-	fd = get_unused_fd_flags(flags);
+	fd = __anon_inode_getfd("[eventfd]", &eventfd_fops, ctx,
+		(flags & EFD_SHARED_FCNTL_FLAGS) | O_RDWR, &file);
 	if (fd < 0)
 		goto err;
 
-	file = anon_inode_getfile("[eventfd]", &eventfd_fops, ctx, flags);
-	if (IS_ERR(file)) {
-		put_unused_fd(fd);
-		fd = PTR_ERR(file);
-		goto err;
-	}
-
 	file->f_mode |= FMODE_NOWAIT;
 	fd_install(fd, file);
 	return fd;
-- 
2.26.2

[PATCH 08/12] vfio: use __anon_inode_getfd

[Christoph Hellwig]

Use __anon_inode_getfd instead of opencoding the logic using
get_unused_fd_flags + anon_inode_getfile.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/vfio/vfio.c | 37 ++++++++-----------------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 765e0e5d83ed9..33a88103f857f 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1451,42 +1451,21 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 		return ret;
 	}
 
-	/*
-	 * We can't use anon_inode_getfd() because we need to modify
-	 * the f_mode flags directly to allow more than just ioctls
-	 */
-	ret = get_unused_fd_flags(O_CLOEXEC);
-	if (ret < 0) {
-		device->ops->release(device->device_data);
-		vfio_device_put(device);
-		return ret;
-	}
-
-	filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops,
-				   device, O_RDWR);
-	if (IS_ERR(filep)) {
-		put_unused_fd(ret);
-		ret = PTR_ERR(filep);
-		device->ops->release(device->device_data);
-		vfio_device_put(device);
-		return ret;
-	}
-
-	/*
-	 * TODO: add an anon_inode interface to do this.
-	 * Appears to be missing by lack of need rather than
-	 * explicitly prevented.  Now there's need.
-	 */
+	ret = __anon_inode_getfd("[vfio-device]", &vfio_device_fops,
+				   device, O_CLOEXEC | O_RDWR, &filep);
+	if (ret < 0)
+		goto release;
 	filep->f_mode |= (FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
-
 	atomic_inc(&group->container_users);
-
 	fd_install(ret, filep);
 
 	if (group->noiommu)
 		dev_warn(device->dev, "vfio-noiommu device opened by user "
 			 "(%s:%d)\n", current->comm, task_pid_nr(current));
-
+	return ret;
+release:
+	device->ops->release(device->device_data);
+	vfio_device_put(device);
 	return ret;
 }
 
-- 
2.26.2

Re: [PATCH 08/12] vfio: use __anon_inode_getfd

[Alex Williamson]

On Fri,  8 May 2020 17:36:30 +0200
Christoph Hellwig <hch@lst.de> wrote:

> Use __anon_inode_getfd instead of opencoding the logic using
> get_unused_fd_flags + anon_inode_getfile.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/vfio/vfio.c | 37 ++++++++-----------------------------
>  1 file changed, 8 insertions(+), 29 deletions(-)


Thanks!

Acked-by: Alex Williamson <alex.williamson@redhat.com>

> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 765e0e5d83ed9..33a88103f857f 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1451,42 +1451,21 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
>  		return ret;
>  	}
>  
> -	/*
> -	 * We can't use anon_inode_getfd() because we need to modify
> -	 * the f_mode flags directly to allow more than just ioctls
> -	 */
> -	ret = get_unused_fd_flags(O_CLOEXEC);
> -	if (ret < 0) {
> -		device->ops->release(device->device_data);
> -		vfio_device_put(device);
> -		return ret;
> -	}
> -
> -	filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops,
> -				   device, O_RDWR);
> -	if (IS_ERR(filep)) {
> -		put_unused_fd(ret);
> -		ret = PTR_ERR(filep);
> -		device->ops->release(device->device_data);
> -		vfio_device_put(device);
> -		return ret;
> -	}
> -
> -	/*
> -	 * TODO: add an anon_inode interface to do this.
> -	 * Appears to be missing by lack of need rather than
> -	 * explicitly prevented.  Now there's need.
> -	 */
> +	ret = __anon_inode_getfd("[vfio-device]", &vfio_device_fops,
> +				   device, O_CLOEXEC | O_RDWR, &filep);
> +	if (ret < 0)
> +		goto release;
>  	filep->f_mode |= (FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
> -
>  	atomic_inc(&group->container_users);
> -
>  	fd_install(ret, filep);
>  
>  	if (group->noiommu)
>  		dev_warn(device->dev, "vfio-noiommu device opened by user "
>  			 "(%s:%d)\n", current->comm, task_pid_nr(current));
> -
> +	return ret;
> +release:
> +	device->ops->release(device->device_data);
> +	vfio_device_put(device);
>  	return ret;
>  }
>  

Re: [PATCH 08/12] vfio: use __anon_inode_getfd

[Alex Williamson]

On Fri,  8 May 2020 17:36:30 +0200
Christoph Hellwig <hch@lst.de> wrote:

> Use __anon_inode_getfd instead of opencoding the logic using
> get_unused_fd_flags + anon_inode_getfile.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/vfio/vfio.c | 37 ++++++++-----------------------------
>  1 file changed, 8 insertions(+), 29 deletions(-)


Thanks!

Acked-by: Alex Williamson <alex.williamson@redhat.com>

> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 765e0e5d83ed9..33a88103f857f 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1451,42 +1451,21 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
>  		return ret;
>  	}
>  
> -	/*
> -	 * We can't use anon_inode_getfd() because we need to modify
> -	 * the f_mode flags directly to allow more than just ioctls
> -	 */
> -	ret = get_unused_fd_flags(O_CLOEXEC);
> -	if (ret < 0) {
> -		device->ops->release(device->device_data);
> -		vfio_device_put(device);
> -		return ret;
> -	}
> -
> -	filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops,
> -				   device, O_RDWR);
> -	if (IS_ERR(filep)) {
> -		put_unused_fd(ret);
> -		ret = PTR_ERR(filep);
> -		device->ops->release(device->device_data);
> -		vfio_device_put(device);
> -		return ret;
> -	}
> -
> -	/*
> -	 * TODO: add an anon_inode interface to do this.
> -	 * Appears to be missing by lack of need rather than
> -	 * explicitly prevented.  Now there's need.
> -	 */
> +	ret = __anon_inode_getfd("[vfio-device]", &vfio_device_fops,
> +				   device, O_CLOEXEC | O_RDWR, &filep);
> +	if (ret < 0)
> +		goto release;
>  	filep->f_mode |= (FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
> -
>  	atomic_inc(&group->container_users);
> -
>  	fd_install(ret, filep);
>  
>  	if (group->noiommu)
>  		dev_warn(device->dev, "vfio-noiommu device opened by user "
>  			 "(%s:%d)\n", current->comm, task_pid_nr(current));
> -
> +	return ret;
> +release:
> +	device->ops->release(device->device_data);
> +	vfio_device_put(device);
>  	return ret;
>  }
>  

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/12] rdma: use __anon_inode_getfd

[Christoph Hellwig]

Use __anon_inode_getfd instead of opencoding the logic using
get_unused_fd_flags + anon_inode_getfile.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/infiniband/core/rdma_core.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index 5128cb16bb485..541e5e06347f6 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -462,30 +462,21 @@ alloc_begin_fd_uobject(const struct uverbs_api_object *obj,
 	if (WARN_ON(fd_type->fops->release != &uverbs_uobject_fd_release))
 		return ERR_PTR(-EINVAL);
 
-	new_fd = get_unused_fd_flags(O_CLOEXEC);
-	if (new_fd < 0)
-		return ERR_PTR(new_fd);
-
 	uobj = alloc_uobj(attrs, obj);
 	if (IS_ERR(uobj))
-		goto err_fd;
+		return uobj;
 
 	/* Note that uverbs_uobject_fd_release() is called during abort */
-	filp = anon_inode_getfile(fd_type->name, fd_type->fops, NULL,
-				  fd_type->flags);
-	if (IS_ERR(filp)) {
-		uobj = ERR_CAST(filp);
+	new_fd = __anon_inode_getfd(fd_type->name, fd_type->fops, NULL,
+			fd_type->flags | O_CLOEXEC, &filp);
+	if (new_fd < 0)
 		goto err_uobj;
-	}
 	uobj->object = filp;
-
 	uobj->id = new_fd;
 	return uobj;
 
 err_uobj:
 	uverbs_uobject_put(uobj);
-err_fd:
-	put_unused_fd(new_fd);
 	return uobj;
 }
 
-- 
2.26.2

Re: [PATCH 09/12] rdma: use __anon_inode_getfd

[Jason Gunthorpe]

On Fri, May 08, 2020 at 05:36:31PM +0200, Christoph Hellwig wrote:
> Use __anon_inode_getfd instead of opencoding the logic using
> get_unused_fd_flags + anon_inode_getfile.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/infiniband/core/rdma_core.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)

 
> diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> index 5128cb16bb485..541e5e06347f6 100644
> --- a/drivers/infiniband/core/rdma_core.c
> +++ b/drivers/infiniband/core/rdma_core.c
> @@ -462,30 +462,21 @@ alloc_begin_fd_uobject(const struct uverbs_api_object *obj,
>  	if (WARN_ON(fd_type->fops->release != &uverbs_uobject_fd_release))
>  		return ERR_PTR(-EINVAL);
>  
> -	new_fd = get_unused_fd_flags(O_CLOEXEC);
> -	if (new_fd < 0)
> -		return ERR_PTR(new_fd);
> -
>  	uobj = alloc_uobj(attrs, obj);
>  	if (IS_ERR(uobj))
> -		goto err_fd;
> +		return uobj;
>  
>  	/* Note that uverbs_uobject_fd_release() is called during abort */
> -	filp = anon_inode_getfile(fd_type->name, fd_type->fops, NULL,
> -				  fd_type->flags);
> -	if (IS_ERR(filp)) {
> -		uobj = ERR_CAST(filp);
> +	new_fd = __anon_inode_getfd(fd_type->name, fd_type->fops, NULL,
> +			fd_type->flags | O_CLOEXEC, &filp);
> +	if (new_fd < 0)
>  		goto err_uobj;

This will conflict with a fix (83a267021221 'RDMA/core: Fix
overwriting of uobj in case of error') that is going to go to -rc
soon.

Also the above misses returning an ERR_PTR if __anon_inode_getfd fails, it
returns a uobj that had been freed.. I suppose it should be something
like

if (new_fd < 0) {
   uverbs_uobject_put(uobj);
   return ERR_PTR(new_fd)
}

?

Jason

Re: [PATCH 09/12] rdma: use __anon_inode_getfd

[Jason Gunthorpe]

On Fri, May 08, 2020 at 05:36:31PM +0200, Christoph Hellwig wrote:
> Use __anon_inode_getfd instead of opencoding the logic using
> get_unused_fd_flags + anon_inode_getfile.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/infiniband/core/rdma_core.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)

 
> diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> index 5128cb16bb485..541e5e06347f6 100644
> --- a/drivers/infiniband/core/rdma_core.c
> +++ b/drivers/infiniband/core/rdma_core.c
> @@ -462,30 +462,21 @@ alloc_begin_fd_uobject(const struct uverbs_api_object *obj,
>  	if (WARN_ON(fd_type->fops->release != &uverbs_uobject_fd_release))
>  		return ERR_PTR(-EINVAL);
>  
> -	new_fd = get_unused_fd_flags(O_CLOEXEC);
> -	if (new_fd < 0)
> -		return ERR_PTR(new_fd);
> -
>  	uobj = alloc_uobj(attrs, obj);
>  	if (IS_ERR(uobj))
> -		goto err_fd;
> +		return uobj;
>  
>  	/* Note that uverbs_uobject_fd_release() is called during abort */
> -	filp = anon_inode_getfile(fd_type->name, fd_type->fops, NULL,
> -				  fd_type->flags);
> -	if (IS_ERR(filp)) {
> -		uobj = ERR_CAST(filp);
> +	new_fd = __anon_inode_getfd(fd_type->name, fd_type->fops, NULL,
> +			fd_type->flags | O_CLOEXEC, &filp);
> +	if (new_fd < 0)
>  		goto err_uobj;

This will conflict with a fix (83a267021221 'RDMA/core: Fix
overwriting of uobj in case of error') that is going to go to -rc
soon.

Also the above misses returning an ERR_PTR if __anon_inode_getfd fails, it
returns a uobj that had been freed.. I suppose it should be something
like

if (new_fd < 0) {
   uverbs_uobject_put(uobj);
   return ERR_PTR(new_fd)
}

?

Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/12] drm_syncobj: use __anon_inode_getfd

[Christoph Hellwig]

Use __anon_inode_getfd instead of opencoding the logic using
get_unused_fd_flags + anon_inode_getfile.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/drm_syncobj.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 42d46414f7679..b69a7be34e8c7 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -581,18 +581,11 @@ int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd)
 	struct file *file;
 	int fd;
 
-	fd = get_unused_fd_flags(O_CLOEXEC);
+	fd = __anon_inode_getfd("syncobj_file", &drm_syncobj_file_fops,
+			syncobj, O_CLOEXEC, &file);
 	if (fd < 0)
 		return fd;
 
-	file = anon_inode_getfile("syncobj_file",
-				  &drm_syncobj_file_fops,
-				  syncobj, 0);
-	if (IS_ERR(file)) {
-		put_unused_fd(fd);
-		return PTR_ERR(file);
-	}
-
 	drm_syncobj_get(syncobj);
 	fd_install(fd, file);
 
-- 
2.26.2

[PATCH 11/12] gpiolib: use __anon_inode_getfd

[Christoph Hellwig]

Use __anon_inode_getfd instead of opencoding the logic using
get_unused_fd_flags + anon_inode_getfile.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpio/gpiolib.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 40f2d7f69be26..cbcf47b1e6a40 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -736,21 +736,13 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	i--;
 	lh->numdescs = handlereq.lines;
 
-	fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
+	fd = __anon_inode_getfd("gpio-linehandle", &linehandle_fileops, lh,
+			O_RDONLY | O_CLOEXEC, &file);
 	if (fd < 0) {
 		ret = fd;
 		goto out_free_descs;
 	}
 
-	file = anon_inode_getfile("gpio-linehandle",
-				  &linehandle_fileops,
-				  lh,
-				  O_RDONLY | O_CLOEXEC);
-	if (IS_ERR(file)) {
-		ret = PTR_ERR(file);
-		goto out_put_unused_fd;
-	}
-
 	handlereq.fd = fd;
 	if (copy_to_user(ip, &handlereq, sizeof(handlereq))) {
 		/*
@@ -769,8 +761,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 
 	return 0;
 
-out_put_unused_fd:
-	put_unused_fd(fd);
 out_free_descs:
 	for (i = 0; i < count; i++)
 		gpiod_free(lh->descs[i]);
@@ -1110,21 +1100,13 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	if (ret)
 		goto out_free_desc;
 
-	fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
+	fd = __anon_inode_getfd("gpio-event", &lineevent_fileops, le,
+			O_RDONLY | O_CLOEXEC, &file);
 	if (fd < 0) {
 		ret = fd;
 		goto out_free_irq;
 	}
 
-	file = anon_inode_getfile("gpio-event",
-				  &lineevent_fileops,
-				  le,
-				  O_RDONLY | O_CLOEXEC);
-	if (IS_ERR(file)) {
-		ret = PTR_ERR(file);
-		goto out_put_unused_fd;
-	}
-
 	eventreq.fd = fd;
 	if (copy_to_user(ip, &eventreq, sizeof(eventreq))) {
 		/*
@@ -1140,8 +1122,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 
 	return 0;
 
-out_put_unused_fd:
-	put_unused_fd(fd);
 out_free_irq:
 	free_irq(le->irq, le);
 out_free_desc:
-- 
2.26.2

[PATCH 12/12] vtpm_proxy: use __anon_inode_getfd

[Christoph Hellwig]

Use __anon_inode_getfd instead of opencoding the logic using
get_unused_fd_flags + anon_inode_getfile.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/char/tpm/tpm_vtpm_proxy.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 91c772e38bb54..4c0a31209ae5a 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -534,7 +534,7 @@ static struct file *vtpm_proxy_create_device(
 				 struct vtpm_proxy_new_dev *vtpm_new_dev)
 {
 	struct proxy_dev *proxy_dev;
-	int rc, fd;
+	int fd;
 	struct file *file;
 
 	if (vtpm_new_dev->flags & ~VTPM_PROXY_FLAGS_ALL)
@@ -546,19 +546,10 @@ static struct file *vtpm_proxy_create_device(
 
 	proxy_dev->flags = vtpm_new_dev->flags;
 
-	/* setup an anonymous file for the server-side */
-	fd = get_unused_fd_flags(O_RDWR);
-	if (fd < 0) {
-		rc = fd;
+	fd = __anon_inode_getfd("[vtpms]", &vtpm_proxy_fops, proxy_dev, O_RDWR,
+			&file);
+	if (fd < 0)
 		goto err_delete_proxy_dev;
-	}
-
-	file = anon_inode_getfile("[vtpms]", &vtpm_proxy_fops, proxy_dev,
-				  O_RDWR);
-	if (IS_ERR(file)) {
-		rc = PTR_ERR(file);
-		goto err_put_unused_fd;
-	}
 
 	/* from now on we can unwind with put_unused_fd() + fput() */
 	/* simulate an open() on the server side */
@@ -576,13 +567,9 @@ static struct file *vtpm_proxy_create_device(
 
 	return file;
 
-err_put_unused_fd:
-	put_unused_fd(fd);
-
 err_delete_proxy_dev:
 	vtpm_proxy_delete_proxy_dev(proxy_dev);
-
-	return ERR_PTR(rc);
+	return ERR_PTR(fd);
 }
 
 /*
-- 
2.26.2