Re: [PATCH 5/5] misc: mlx5ctl: Add umem reg/unreg ioctl

["Arnd Bergmann" <arnd@arndb.de>]

On Wed, Oct 18, 2023, at 10:19, Saeed Mahameed wrote:
> From: Saeed Mahameed <saeedm@nvidia.com>

>
> To do so this patch introduces two ioctls:
>
> MLX5CTL_IOCTL_UMEM_REG(va_address, size):
>  - calculate page fragments from the user provided virtual address
>  - pin the pages, and allocate a sg list
>  - dma map the sg list
>  - create a UMEM device object that points to the dma addresses
>  - add a driver umem object to an xarray data base for bookkeeping
>  - return UMEM ID to user so it can be used in subsequent rpcs
>
> MLX5CTL_IOCTL_UMEM_UNREG(umem_id):
>  - user provides a pre allocated umem ID
>  - unwinds the above
>

> +static ssize_t mlx5ctl_ioctl_umem_reg(struct file *file, unsigned long 
> arg)
> +{
> +	struct mlx5ctl_fd *mfd = file->private_data;
> +	struct mlx5ctl_umem_reg umem_reg;
> +	int umem_id;
> +
> +	if (copy_from_user(&umem_reg, (void __user *)arg, sizeof(umem_reg)))
> +		return -EFAULT;

Instead of an in-place cast, the usual way to do it is to
have a local pointer of the correct type
(struct mlx5ctl_umem_reg __iomem *) as the function argument.

> +
> +	umem_id = mlx5ctl_umem_reg(mfd->umem_db, (unsigned 
> long)umem_reg.addr, umem_reg.len);

umem_reg.addr seems to be a user space address, so I would
suggest consistently passing it as a 'void __user *' instead
of casting to (unsigned long) here. You can use u64_to_user_ptr()
to handle the pointer conversion correctly across all
architectures that way, and get better type checking.

> @@ -24,6 +24,14 @@ struct mlx5ctl_cmdrpc {
>  	__aligned_u64 flags;
>  };
> 
> +struct mlx5ctl_umem_reg {
> +	__aligned_u64 addr; /* user address */
> +	__aligned_u64 len; /* user buffer length */
> +	__aligned_u64 flags;
> +	__u32 umem_id; /* returned device's umem ID */
> +	__u32 reserved[7];
> +};
> +

You have a 'flags' argument that is never accessed and can
probably be removed. If the intention was to make the ioctl
extensible for the future, this doesn't work unless you
ensure that only known flags (i.e. none at this point)
are set, and it's probably a bad idea anyway, compared
to creating a new ioctl command with new semantics.

Same for the 'reserved' fields except as needed for padding.

> +#define MLX5CTL_IOCTL_UMEM_UNREG \
> +	_IOWR(MLX5CTL_IOCTL_MAGIC, 0x3, unsigned long)

The use of 'unsigned long' here is wrong, this just makes
the command incompatible with compat mode, since
that type has different sizes. It also doesn't
match what the implementation uses, as that does
not try to read and write an 'unsigned long'
from user memory but instead takes the argument
itself as an integer. If you want to keep the use
of direct integer arguments (instead of pointer to
__u32 or __u64) here, this would have to be

   _IO(MLX5CTL_IOCTL_MAGIC, 0x3)

     Arnd