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

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

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

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