Messages in this thread | | | Date | Wed, 18 Oct 2023 11:30:24 +0200 | From | "Arnd Bergmann" <> | Subject | Re: [PATCH 5/5] misc: mlx5ctl: Add umem reg/unreg ioctl |
| |
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
| |