Re: [PATCH v4 01/17] iommu: Add hwpt_type with user_data for domain_alloc_user op

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, Sep 21, 2023 at 08:10:58PM +0800, Baolu Lu wrote:
> On 2023/9/21 15:51, Yi Liu wrote:
> > +/**
> > + * iommu_copy_user_data - Copy iommu driver specific user space data
> > + * @dst_data: Pointer to an iommu driver specific user data that is defined in
> > + *            include/uapi/linux/iommufd.h
> > + * @src_data: Pointer to a struct iommu_user_data for user space data info
> > + * @data_len: Length of current user data structure, i.e. sizeof(struct _dst)
> > + * @min_len: Initial length of user data structure for backward compatibility.
> > + *           This should be offsetofend using the last member in the user data
> > + *           struct that was initially added to include/uapi/linux/iommufd.h
> > + */
> > +static inline int iommu_copy_user_data(void *dst_data,
> > +                                    const struct iommu_user_data *src_data,
> > +                                    size_t data_len, size_t min_len)
> > +{
> > +     if (WARN_ON(!dst_data || !src_data))
> > +             return -EINVAL;
> > +     if (src_data->len < min_len || data_len < src_data->len)
> > +             return -EINVAL;
> > +     return copy_struct_from_user(dst_data, data_len,
> > +                                  src_data->uptr, src_data->len);
> > +}
> 
> I am not sure that I understand the purpose of "min_len" correctly. It
> seems like it would always be equal to data_len?
> 
> Or, it means the minimal data length that the iommu driver requires?

Hmm, I thought I had made it quite clear with the kdoc that it's
the "initial" length (min_len) v.s. "current" length (data_len),
i.e. min_len was set when the structure was introduced and would
never change while data_len can grow if the structure introduces
a new member. E.g. after this series struct iommu_hwpt_alloc has
a min_len fixed to offsetofend(..., __reserved) but its data_len
is actually increased to offsetofend(..., data_uptr).

Perhaps we could put all min_len defines in uAPI header, like:
include/uapi/linux/gfs2_ondisk.h:442:#define LH_V1_SIZE (offsetofend(struct gfs2_log_header, lh_hash))
In this way, drivers won't need to deal with that nor have risks
of breaking ABI by changing a min_len.

Also, if we go a bit further to ease the drivers, we could do:

========================================================================================
diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index 65a363f5e81e..13234e67409c 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -147,6 +147,9 @@ struct iommu_hwpt_selftest {
 	__u32 iotlb;
 };
 
+#define iommu_hwpt_selftest_min_len \
+	(offsetofend(struct iommu_hwpt_selftest, iotlb))
+
 /**
  * struct iommu_hwpt_invalidate_selftest
  *
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 117776d236dc..2cc3a8a3355b 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -263,8 +263,8 @@ mock_domain_alloc_user(struct device *dev, u32 flags,
 	}
 
 	if (user_data) {
-		int rc = iommu_copy_user_data(&data, user_data,
-					      data_len, min_len);
+		int rc = iommu_copy_user_data2(iommu_hwpt_selftest, &data,
+					       user_data);
 		if (rc)
 			return ERR_PTR(rc);
 		user_cfg = &data;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fb2febe7b8d8..db82803b026f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -282,6 +282,10 @@ static inline int iommu_copy_user_data(void *dst_data,
 				     src_data->uptr, src_data->len);
 }
 
+#define iommu_copy_user_data2(dst_struct, dst, src)               \
+	iommu_copy_user_data(dst, src, sizeof(struct dst_struct), \
+			     dst_struct##_min_len)
+
 /**
  * iommu_copy_user_data_from_array - Copy iommu driver specific user space data
  *                                   from an iommu_user_data_array input
========================================================================================

Surely, the end product of the test code above can do:
	iommu_copy_user_data = > __iommu_copy_user_data
	iommu_copy_user_data2 = > iommu_copy_user_data

Thanks
Nicolin