Re: [PATCH v8 08/15] iommufd: Add a vDEVICE operation ioctl framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] iommufd: iommufd_vdevice_op_ioctl() returns -EINVAL for non-zero reserved field
--

commit 02d0f32280b8147168fe9f8c2300f3939bdda343
Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

iommufd: Add a vDEVICE operation ioctl framework

This commit introduces the IOMMU_VDEVICE_OP ioctl and its dispatcher
iommufd_vdevice_op_ioctl() to execute operations on a vDEVICE object using
request and response buffers. It adds a supported_ops bitmask to struct
iommufd_vdevice and returns -EOPNOTSUPP by default when no operations are
defined.

> diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
> index 0dbd382eeb981..cdbb597f35888 100644
> --- a/drivers/iommu/iommufd/viommu.c
> +++ b/drivers/iommu/iommufd/viommu.c
[ ... ]
> @@ -121,6 +125,38 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
>  	return rc;
>  }
>  
> +int iommufd_vdevice_op_ioctl(struct iommufd_ucmd *ucmd)
> +{
> +	struct iommu_vdevice_op *cmd = ucmd->cmd;
> +	struct iommufd_object *obj;
> +	struct iommufd_vdevice *vdev;
> +	int ret;
> +
> +	if (cmd->reserved)
> +		return -EINVAL;

[Severity: Medium]
Should iommufd_vdevice_op_ioctl() return -EOPNOTSUPP instead of -EINVAL when
cmd->reserved is non-zero?

The general ioctl format documentation in include/uapi/linux/iommufd.h
specifies that -EOPNOTSUPP is returned when a known field has a value the
kernel does not understand or support, whereas -EINVAL indicates a field is
not correct:

include/uapi/linux/iommufd.h:
 *  - EOPNOTSUPP: The IOCTL number is supported, and the structure is
 *    understood, however a known field has a value the kernel does not
 *    understand or support.
 *  - EINVAL: Everything about the IOCTL was understood, but a field is not
 *    correct.

All other ioctl handlers across drivers/iommu/iommufd/ return -EOPNOTSUPP when
a reserved field is non-zero so userspace can detect whether a future kernel
extension repurposing that field is supported.

Additionally, in include/uapi/linux/iommufd.h, struct iommu_vdevice_op names
this member reserved:

struct iommu_vdevice_op {
	...
	__u32 reserved;
	...
};

Should this member be named __reserved to match the other UAPI structures in
include/uapi/linux/iommufd.h?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104161632.3547423-1-aneesh.kumar@kernel.org?part=8