Re: [PATCH v8 3/5] iommufd: Add IOMMU_GET_HW_INFO

From: Nicolin Chen
Date: Thu Aug 17 2023 - 20:22:38 EST

On Thu, Aug 17, 2023 at 05:08:34PM -0700, Nicolin Chen wrote:
> On Fri, Aug 18, 2023 at 12:04:29AM +0000, Liu, Yi L wrote:
>
> > > From: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > > Sent: Friday, August 18, 2023 5:08 AM
> > >
> > > Looks like Yi's latest code has not addressed these comments.
> >
> > Yeah. Not yet. In progress to incorporate them. 😊
>
> I fixed them all in my local tree. I'm finalizing with alloc_user.
> Let me send my branch after that, and you can edit upon :)

https://github.com/nicolinc/iommufd/tree/wip/iommufd_alloc_user-08172023-nic
Attached two sets of git-diff for the updates that I made to the
two series. Note that I didn't make any change to the VT-d patch.

Thanks
Nic
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 36dff7ca3ae4..f5836a5bdbbc 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -1144,7 +1144,7 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
data = ops->hw_info(idev->dev, &data_len, &cmd->out_data_type);
if (IS_ERR(data)) {
rc = PTR_ERR(data);
- goto err_put;
+ goto out_put;
}

/*
@@ -1154,18 +1154,17 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
if (WARN_ON_ONCE(cmd->out_data_type ==
IOMMU_HW_INFO_TYPE_NONE)) {
rc = -ENODEV;
- goto out;
+ goto out_free;
}
} else {
cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE;
data_len = 0;
- data = NULL;
}

copy_len = min(cmd->data_len, data_len);
if (copy_to_user(user_ptr, data, copy_len)) {
rc = -EFAULT;
- goto out;
+ goto out_free;
}

/*
@@ -1175,7 +1174,7 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
if (copy_len < cmd->data_len) {
if (clear_user(user_ptr + copy_len, cmd->data_len - copy_len)) {
rc = -EFAULT;
- goto out;
+ goto out_free;
}
}

@@ -1186,9 +1185,9 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
cmd->data_len = data_len;

rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
-out:
+out_free:
kfree(data);
-err_put:
+out_put:
iommufd_put_object(&idev->obj);
return rc;
}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f2d6a3989713..bd6a1110b294 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -228,14 +228,10 @@ struct iommu_iotlb_gather {
/**
* struct iommu_ops - iommu ops and capabilities
* @capable: check capability
- * @hw_info: IOMMU hardware information. The type of the returned data is
- * marked by the output type of this op. Type is one of
- * enum iommu_hw_info_type defined in include/uapi/linux/iommufd.h.
- * The drivers that support this op should define a unique type
- * in include/uapi/linux/iommufd.h. The data buffer returned by this
- * op is allocated in the IOMMU driver and the caller should free it
- * after use. Return the data buffer if success, or ERR_PTR on
- * failure.
+ * @hw_info: report iommu hardware information. The data buffer returned by this
+ * op is allocated in the iommu driver and freed by the caller after
+ * use. The information type is one of enum iommu_hw_info_type defined
+ * in include/uapi/linux/iommufd.h.
* @domain_alloc: allocate iommu domain
* @probe_device: Add device to iommu driver handling
* @release_device: Remove device from iommu driver handling
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 42f024c07006..b4ba0c0cbab6 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -429,10 +429,7 @@ enum iommu_hw_info_type {
* fills the buffer using the given length in @data_len. If the ioctl succeeds,
* @data_len will be updated to the length that kernel actually supports,
* @out_data_type will be filled to decode the data filled in the buffer
- * pointed by @data_uptr. Input @data_len == zero is allowed, no information
- * data will be filled to user, but user space could get the iommu_hw_info_type
- * filled in @out_data_type and the iommu hardware information data length
- * supported by kernel filled in @data_len.
+ * pointed by @data_uptr. Input @data_len == zero is allowed.
*/
struct iommu_hw_info {
__u32 size;
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index a068bfbe9f83..33d08600be13 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -295,10 +295,13 @@ TEST_F(iommufd_ioas, ioas_area_auto_destroy)
TEST_F(iommufd_ioas, get_hw_info)
{
struct iommu_test_hw_info buffer_exact;
- struct iommu_test_hw_info_buffer {
+ struct iommu_test_hw_info_buffer_larger {
struct iommu_test_hw_info info;
uint64_t trailing_bytes;
} buffer_larger;
+ struct iommu_test_hw_info_buffer_smaller {
+ __u32 flags;
+ } buffer_smaller;

if (self->device_id) {
/* Provide a zero-size user_buffer */
@@ -310,6 +313,11 @@ TEST_F(iommufd_ioas, get_hw_info)
* kernel zero the trailing bytes.
*/
test_cmd_get_hw_info(self->device_id, &buffer_larger, sizeof(buffer_larger));
+ /*
+ * Provide a user_buffer with size smaller than the exact size to check if
+ * the fields within the size range still gets updated.
+ */
+ test_cmd_get_hw_info(self->device_id, &buffer_smaller, sizeof(buffer_smaller));
} else {
test_err_get_hw_info(ENOENT, self->device_id,
&buffer_exact, sizeof(buffer_exact));
diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
index 097418f2fdb3..e0753d03ecaa 100644
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -21,6 +21,10 @@ static unsigned long BUFFER_SIZE;

static unsigned long PAGE_SIZE;

+#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
+#define offsetofend(TYPE, MEMBER) \
+ (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
+
/*
* Have the kernel check the refcount on pages. I don't know why a freshly
* mmap'd anon non-compound page starts out with a ref of 3
@@ -353,13 +357,13 @@ static void teardown_iommufd(int fd, struct __test_metadata *_metadata)
static int _test_cmd_get_hw_info(int fd, __u32 device_id,
void *data, size_t data_len)
{
+ struct iommu_test_hw_info *info = (struct iommu_test_hw_info *)data;
struct iommu_hw_info cmd = {
.size = sizeof(cmd),
.dev_id = device_id,
.data_len = data_len,
.data_uptr = (uint64_t)data,
};
- struct iommu_test_hw_info *info = (struct iommu_test_hw_info *)data;
int ret;

ret = ioctl(fd, IOMMU_GET_HW_INFO, &cmd);
@@ -389,8 +393,10 @@ static int _test_cmd_get_hw_info(int fd, __u32 device_id,
}

if (info) {
- assert(info->test_reg == IOMMU_HW_INFO_SELFTEST_REGVAL);
- assert(!info->flags);
+ if (data_len >= offsetofend(struct iommu_test_hw_info, test_reg))
+ assert(info->test_reg == IOMMU_HW_INFO_SELFTEST_REGVAL);
+ if (data_len >= offsetofend(struct iommu_test_hw_info, flags))
+ assert(!info->flags);
}

return 0;
diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index 1d9ba7454605..3f3644375bf1 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -109,18 +109,4 @@ struct iommu_test_hw_info {
__u32 test_reg;
};

-/* Should not be equal to any defined value in enum iommu_hwpt_type */
-#define IOMMU_HWPT_TYPE_SELFTEST 0xdead
-
-/**
- * struct iommu_hwpt_selftest
- *
- * @flags: page table entry attributes, must be 0
- * @test_type: can be either IOMMU_HW_INFO_TYPE_NONE or IOMMU_HWPT_TYPE_SELFTEST
- */
-struct iommu_hwpt_selftest {
- __u64 flags;
- __u32 test_type;
-};
-
#endif
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 3cd1b17638bc..0bd7534be257 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -88,8 +88,6 @@ void iommufd_test_syz_conv_iova_id(struct iommufd_ucmd *ucmd,
struct mock_iommu_domain {
struct iommu_domain domain;
struct xarray pfns;
- /* mock domain test data */
- enum iommu_hwpt_type hwpt_type;
};

enum selftest_obj_type {
@@ -150,10 +148,17 @@ static void *mock_domain_hw_info(struct device *dev, u32 *length, u32 *type)

static const struct iommu_ops mock_ops;

+union mock_domain_alloc_data {
+ struct iommu_hwpt_default default_data;
+};
+
static struct iommu_domain *
-__mock_domain_alloc(unsigned int iommu_domain_type,
- const struct iommu_hwpt_selftest *user_cfg)
+__mock_domain_alloc_default(unsigned int iommu_domain_type,
+ const union mock_domain_alloc_data *data)
{
+ const struct iommu_hwpt_default *user_cfg =
+ (const struct iommu_hwpt_default *)data;
+ dma_addr_t aperture_end = MOCK_APERTURE_LAST;
struct mock_iommu_domain *mock;

if (iommu_domain_type == IOMMU_DOMAIN_BLOCKED)
@@ -162,16 +167,21 @@ __mock_domain_alloc(unsigned int iommu_domain_type,
if (iommu_domain_type != IOMMU_DOMAIN_UNMANAGED)
return NULL;

+ if (user_cfg) {
+ if (user_cfg->max_addr > MOCK_APERTURE_LAST ||
+ user_cfg->max_addr <= MOCK_APERTURE_START)
+ return ERR_PTR(-EINVAL);
+ aperture_end = user_cfg->max_addr;
+ }
+
mock = kzalloc(sizeof(*mock), GFP_KERNEL);
if (!mock)
return ERR_PTR(-ENOMEM);
mock->domain.type = iommu_domain_type;
mock->domain.geometry.aperture_start = MOCK_APERTURE_START;
- mock->domain.geometry.aperture_end = MOCK_APERTURE_LAST;
+ mock->domain.geometry.aperture_end = aperture_end;
mock->domain.ops = mock_ops.default_domain_ops;
mock->domain.pgsize_bitmap = MOCK_IO_PAGE_SIZE;
- if (user_cfg)
- mock->hwpt_type = user_cfg->test_type;
xa_init(&mock->pfns);
return &mock->domain;
}
@@ -183,7 +193,7 @@ static struct iommu_domain *mock_domain_alloc(unsigned int iommu_domain_type)
if (iommu_domain_type != IOMMU_DOMAIN_BLOCKED &&
iommu_domain_type != IOMMU_DOMAIN_UNMANAGED)
return NULL;
- domain = __mock_domain_alloc(iommu_domain_type, NULL);
+ domain = __mock_domain_alloc_default(iommu_domain_type, NULL);
if (IS_ERR(domain))
domain = NULL;
return domain;
@@ -193,27 +203,31 @@ static struct iommu_domain *mock_domain_alloc_user(struct device *dev,
enum iommu_hwpt_type hwpt_type,
const struct iommu_user_data *user_data)
{
- const size_t min_len =
- offsetofend(struct iommu_hwpt_selftest, test_type);
+ struct iommu_domain *(*alloc_fn)(unsigned int iommu_domain_type,
+ const union mock_domain_alloc_data *data);
unsigned int iommu_domain_type = IOMMU_DOMAIN_UNMANAGED;
- struct iommu_hwpt_selftest data, *user_cfg = NULL;
-
- if (hwpt_type != IOMMU_HWPT_TYPE_DEFAULT &&
- hwpt_type != IOMMU_HWPT_TYPE_SELFTEST)
+ union mock_domain_alloc_data data, *user_cfg = NULL;
+ size_t data_len, min_len;
+
+ switch (hwpt_type) {
+ case IOMMU_HWPT_TYPE_DEFAULT:
+ alloc_fn = __mock_domain_alloc_default;
+ data_len = sizeof(struct iommu_hwpt_default);
+ min_len = offsetofend(struct iommu_hwpt_default, max_addr);
+ break;
+ default:
return ERR_PTR(-EINVAL);
+ }

if (user_data) {
int rc = iommu_copy_user_data(&data, user_data,
- sizeof(data), min_len);
+ data_len, min_len);
if (rc)
return ERR_PTR(rc);
- /* Expecting test program to pass hwpt_type in test data too */
- if (data.test_type != hwpt_type)
- return ERR_PTR(rc);
user_cfg = &data;
}

- return __mock_domain_alloc(iommu_domain_type, user_cfg);
+ return alloc_fn(iommu_domain_type, user_cfg);
}

static void mock_domain_free(struct iommu_domain *domain)
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index 7eb7b88459bd..2cbe47c80c09 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -274,20 +274,31 @@ TEST_F(iommufd_ioas, ioas_destroy)
TEST_F(iommufd_ioas, hwpt_alloc)
{
const uint32_t min_data_len =
- offsetofend(struct iommu_hwpt_selftest, test_type);
- struct iommu_hwpt_selftest data = {
+ offsetofend(struct iommu_hwpt_default, max_addr);
+ struct iommu_hwpt_default data = {
.flags = 0,
- .test_type = IOMMU_HWPT_TYPE_SELFTEST,
};
uint32_t new_hwpt_id[2] = {};

if (self->stdev_id && self->device_id) {
test_cmd_hwpt_alloc(self->device_id,
self->ioas_id, &new_hwpt_id[0]);
+ /* Try alloc_user w/o data */
+ test_err_cmd_hwpt_alloc_user(EINVAL,
+ self->device_id, self->ioas_id,
+ &new_hwpt_id[1],
+ IOMMU_HWPT_TYPE_DEFAULT,
+ 0, &data);
+ test_err_cmd_hwpt_alloc_user(EINVAL,
+ self->device_id, self->ioas_id,
+ &new_hwpt_id[1],
+ IOMMU_HWPT_TYPE_DEFAULT,
+ min_data_len, NULL);
test_cmd_hwpt_alloc_user(self->device_id, self->ioas_id,
&new_hwpt_id[1],
- IOMMU_HWPT_TYPE_SELFTEST,
- min_data_len, &data);
+ IOMMU_HWPT_TYPE_DEFAULT, 0, NULL);
+
+ /* Replace the auto domain with new_hwpt_id[0] */
test_cmd_mock_domain_replace(self->stdev_id, new_hwpt_id[0]);
/* hw_pagetable cannot be freed if a device is attached to it */
EXPECT_ERRNO(EBUSY,
@@ -302,6 +313,25 @@ TEST_F(iommufd_ioas, hwpt_alloc)
/* Detach from the new hw_pagetable[1] and try again */
test_cmd_mock_domain_replace(self->stdev_id, self->ioas_id);
test_ioctl_destroy(new_hwpt_id[1]);
+
+ /* Try alloc_user with data */
+ data.max_addr = MOCK_APERTURE_START - 1;
+ test_err_cmd_hwpt_alloc_user(EINVAL,
+ self->device_id, self->ioas_id,
+ &new_hwpt_id[1],
+ IOMMU_HWPT_TYPE_DEFAULT,
+ min_data_len, &data);
+ data.max_addr = (uint64_t)MOCK_APERTURE_LAST + 1;
+ test_err_cmd_hwpt_alloc_user(EINVAL,
+ self->device_id, self->ioas_id,
+ &new_hwpt_id[1],
+ IOMMU_HWPT_TYPE_DEFAULT,
+ min_data_len, &data);
+ data.max_addr = MOCK_APERTURE_LAST;
+ test_cmd_hwpt_alloc_user(self->device_id, self->ioas_id,
+ &new_hwpt_id[1],
+ IOMMU_HWPT_TYPE_DEFAULT,
+ min_data_len, &data);
} else {
test_err_cmd_hwpt_alloc(ENOENT,
self->device_id, self->ioas_id,
@@ -309,7 +339,7 @@ TEST_F(iommufd_ioas, hwpt_alloc)
test_err_cmd_hwpt_alloc_user(ENOENT,
self->device_id, self->ioas_id,
&new_hwpt_id[1],
- IOMMU_HWPT_TYPE_SELFTEST,
+ IOMMU_HWPT_TYPE_DEFAULT,
min_data_len, &data);
test_err_mock_domain_replace(ENOENT,
self->stdev_id, new_hwpt_id[0]);