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
