Re: [PATCH V4 2/9] mshv: Add declarations and definitions for VFIO-MSHV bridge device

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The UAPI ioctl command macro for MSHV_CREATE_DEVICE is missing.
- [Medium] The ops structure mshv_vfio_device_ops is not declared as const.
--

--- Patch [2]: [PATCH V4 2/9] mshv: Add declarations and definitions for VFIO-MSHV bridge device ---
commit dbad27877cef927b38632a1b77ab492ceeebae34
Author: Mukesh R <[email protected]>

mshv: Add declarations and definitions for VFIO-MSHV bridge device
    
Add data structs needed by the subsequent patch that introduces a new
module to implement VFIO-MSHV pseudo device.

> diff --git a/drivers/hv/mshv_root.h b/drivers/hv/mshv_root.h
> index a85c24dcc701a..b9880d0bdc4df 100644
> --- a/drivers/hv/mshv_root.h
> +++ b/drivers/hv/mshv_root.h

[ ... ]

> +struct mshv_device_ops {
> +	const char *device_name;
> +	long (*device_create)(struct mshv_device *dev);
> +	void (*device_release)(struct mshv_device *dev);
> +	long (*device_set_attr)(struct mshv_device *dev,
> +				struct mshv_device_attr *attr);
> +	long (*device_has_attr)(struct mshv_device *dev,
> +				struct mshv_device_attr *attr);
> +};
> +
> +extern struct mshv_device_ops mshv_vfio_device_ops;

[Severity: Medium]
Should mshv_vfio_device_ops be declared as const? 

Operations structures containing function pointers are typically placed in
read-only memory as a security hardening measure. Missing the const keyword on
the extern declaration prevents the future definition from being const without
triggering compilation errors.

> diff --git a/include/uapi/linux/mshv.h b/include/uapi/linux/mshv.h
> index 32ff92b6342b2..be6fe3ee87078 100644
> --- a/include/uapi/linux/mshv.h
> +++ b/include/uapi/linux/mshv.h

[ ... ]

> +/* Device passhthru */
> +#define MSHV_CREATE_DEVICE_TEST		1
> +
> +enum {
> +	MSHV_DEV_TYPE_VFIO,
> +	MSHV_DEV_TYPE_MAX,
> +};
> +
> +struct mshv_create_device {
> +	__u32	type;	     /* in: MSHV_DEV_TYPE_xxx */
> +	__u32	fd;	     /* out: device handle */
> +	__u32	flags;	     /* in: MSHV_CREATE_DEVICE_xxx */
> +};
> +
> +#define MSHV_DEV_VFIO_FILE      1
> +#define MSHV_DEV_VFIO_FILE_ADD	1
> +#define MSHV_DEV_VFIO_FILE_DEL	2
> +
> +struct mshv_device_attr {
> +	__u32	flags;		/* no flags currently defined */
> +	__u32	group;		/* device-defined */
> +	__u64	attr;		/* group-defined */
> +	__u64	addr;		/* userspace address of attr data */
> +};
> +
> +/* Device fds created with MSHV_CREATE_DEVICE */
> +#define MSHV_SET_DEVICE_ATTR	_IOW(MSHV_IOCTL, 0x00, struct mshv_device_attr)
> +#define MSHV_HAS_DEVICE_ATTR	_IOW(MSHV_IOCTL, 0x01, struct mshv_device_attr)

[Severity: High]
Is the UAPI ioctl command macro for MSHV_CREATE_DEVICE missing from this
header?

The struct mshv_create_device is defined for the ioctl payload, and the
comments reference MSHV_CREATE_DEVICE, but the actual ioctl command macro
appears to be absent. Without this definition, userspace cannot compile or
invoke the ioctl to create a device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2