lkml.org 
[lkml]   [2023]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2023-10-18 11:33    [W:7.307 / U:0.508 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site