Re: [PATCH v1 07/24] IB/uverbs: Add context import lock/unlock helper

[Jason Gunthorpe <jgg@mellanox.com>]

On Wed, Aug 21, 2019 at 05:21:08PM +0300, Yuval Shaia wrote:
> From: Shamir Rabinovitch <shamir.rabinovitch@oracle.com>
> 
> The lock/unlock helpers will be used in every import verb.
> 
> Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com>
> Signed-off-by: Shamir Rabinovitch <srabinov7@gmail.com>
>  drivers/infiniband/core/uverbs.h      |  2 +
>  drivers/infiniband/core/uverbs_cmd.c  | 73 +++++++++++++++++++++++++++
>  drivers/infiniband/core/uverbs_main.c |  1 +
>  3 files changed, 76 insertions(+)
> 
> diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
> index 1e5aeb39f774..cf76336cb460 100644
> +++ b/drivers/infiniband/core/uverbs.h
> @@ -163,6 +163,8 @@ struct ib_uverbs_file {
>  	struct page *disassociate_page;
>  
>  	struct xarray		idr;
> +
> +	struct file	       *filp;
>  };
>  
>  struct ib_uverbs_event {
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 4f42f9732dca..21f0a1a986f4 100644
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -43,6 +43,7 @@
>  
>  #include <rdma/uverbs_types.h>
>  #include <rdma/uverbs_std_types.h>
> +#include <rdma/uverbs_ioctl.h>
>  #include "rdma_core.h"
>  
>  #include "uverbs.h"
> @@ -3791,6 +3792,78 @@ static void uverbs_init_attrs_ufile(struct uverbs_attr_bundle *attrs_bundle,
>  	};
>  }
>  
> +/* ib_uverbs_import_lock - Function which gathers code that is
> + *	common in the import verbs.
> + *
> + *	This function guarntee that both source and destination files are
> + *	protected from race with vfs close. The current file is protected
> + *	from such race because verb is executed in a system-call context.
> + *	The other file is protected by 'fget'. This function also ensures
> + *	that ib_uobject identified by the type & handle is locked for read.
> + *
> + *	Callers of this helper must also call ib_uverbs_import_unlock
> + *	to undo any locking performed by this helper.
> + */
> +static int ib_uverbs_import_lock(struct uverbs_attr_bundle *attrs,

This isn't a lock, it is a get

> +				 int fd, u16 type, u32 handle,
> +				 struct ib_uobject **uobj,
> +				 struct file **filep,
> +				 struct ib_uverbs_file **ufile)
> +{
> +	struct ib_uverbs_file *file = attrs->ufile;
> +	struct ib_uverbs_device *dev = file->device;
> +	struct uverbs_attr_bundle fd_attrs;
> +	struct ib_uverbs_device *fd_dev;
> +	int ret = 0;
> +
> +	*filep = fget(fd);
> +	if (!*filep)
> +		return -EINVAL;
> +
> +	/* check uverbs ops exist */
> +	if ((*filep)->f_op != file->filp->f_op) {
> +		ret = -EINVAL;
> +		goto file;
> +	}
> +
> +	*ufile = (*filep)->private_data;
> +	fd_dev = (*ufile)->device;

Most likely all of this should be in the ioctl code as part of some 

> +	/* check that both files belong to same ib_device */
> +	if (dev != fd_dev) {
> +		ret = -EINVAL;
> +		goto file;
> +	}
> +
> +	uverbs_init_attrs_ufile(&fd_attrs, *ufile);

This makes no sense here

> +	*uobj = uobj_get_read(type, handle, &fd_attrs);
> +	if (IS_ERR(*uobj)) {
> +		ret = -EINVAL;
> +		goto file;
> +	}
> +
> +	/* verify ib_object is shareable */
> +	if (!(*uobj)->refcnt) {
> +		ret = -EINVAL;
> +		goto uobj;
> +	}
> +
> +	return 0;
> +uobj:
> +	uobj_put_read(*uobj);
> +file:
> +	fput(*filep);
> +	return ret;
> +}

Most likely most of this belongs to the ioctl path as some new input
attr type 'foreign context'

Jason