From: Jason Gunthorpe <jgg@nvidia.com>
Sent: Wednesday, September 22, 2021 1:15 AM

On Sun, Sep 19, 2021 at 02:38:35PM +0800, Liu Yi L wrote:

quoted

+/*
+ * A iommufd_device object represents the binding relationship
+ * between iommufd and device. It is created per a successful
+ * binding request from device driver. The bound device must be
+ * a physical device so far. Subdevice will be supported later
+ * (with additional PASID information). An user-assigned cookie
+ * is also recorded to mark the device in the /dev/iommu uAPI.
+ */
+struct iommufd_device {
+	unsigned int id;
+	struct iommufd_ctx *ictx;
+	struct device *dev; /* always be the physical device */
+	u64 dev_cookie;
 };

 static int iommufd_fops_open(struct inode *inode, struct file *filep)

@@ -32,15 +52,58 @@ static int iommufd_fops_open(struct inode *inode,

struct file *filep)

quoted

 		return -ENOMEM;

 	refcount_set(&ictx->refs, 1);
+	mutex_init(&ictx->lock);
+	xa_init_flags(&ictx->device_xa, XA_FLAGS_ALLOC);
 	filep->private_data = ictx;

 	return ret;
 }

+static void iommufd_ctx_get(struct iommufd_ctx *ictx)
+{
+	refcount_inc(&ictx->refs);
+}

See my earlier remarks about how to structure the lifetime logic, this
ref isn't necessary.

quoted

+static const struct file_operations iommufd_fops;
+
+/**
+ * iommufd_ctx_fdget - Acquires a reference to the internal iommufd

context.

quoted

+ * @fd: [in] iommufd file descriptor.
+ *
+ * Returns a pointer to the iommufd context, otherwise NULL;
+ *
+ */
+static struct iommufd_ctx *iommufd_ctx_fdget(int fd)
+{
+	struct fd f = fdget(fd);
+	struct file *file = f.file;
+	struct iommufd_ctx *ictx;
+
+	if (!file)
+		return NULL;
+
+	if (file->f_op != &iommufd_fops)
+		return NULL;

Leaks the fdget

quoted

+
+	ictx = file->private_data;
+	if (ictx)
+		iommufd_ctx_get(ictx);

Use success oriented flow

quoted

+	fdput(f);
+	return ictx;
+}

quoted

+ */
+struct iommufd_device *iommufd_bind_device(int fd, struct device *dev,
+					   u64 dev_cookie)
+{
+	struct iommufd_ctx *ictx;
+	struct iommufd_device *idev;
+	unsigned long index;
+	unsigned int id;
+	int ret;
+
+	ictx = iommufd_ctx_fdget(fd);
+	if (!ictx)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&ictx->lock);
+
+	/* check duplicate registration */
+	xa_for_each(&ictx->device_xa, index, idev) {
+		if (idev->dev == dev || idev->dev_cookie == dev_cookie) {
+			idev = ERR_PTR(-EBUSY);
+			goto out_unlock;
+		}

I can't think of a reason why this expensive check is needed.

quoted

+	}
+
+	idev = kzalloc(sizeof(*idev), GFP_KERNEL);
+	if (!idev) {
+		ret = -ENOMEM;
+		goto out_unlock;
+	}
+
+	/* Establish the security context */
+	ret = iommu_device_init_user_dma(dev, (unsigned long)ictx);
+	if (ret)
+		goto out_free;
+
+	ret = xa_alloc(&ictx->device_xa, &id, idev,
+		       XA_LIMIT(IOMMUFD_DEVID_MIN,

IOMMUFD_DEVID_MAX),

quoted

+		       GFP_KERNEL);

idev should be fully initialized before being placed in the xarray, so
this should be the last thing done.

all good suggestions above. thanks for catching them.

Why not just use the standard xa_limit_32b instead of special single
use constants?

yeah. should use xa_limit_32b.

Regards,
Yi Liu

Jason