Re: [PATCH V3 3/5] misc: mlx5ctl: Add info ioctl

[Greg Kroah-Hartman]

On Mon, Nov 20, 2023 at 11:06:17PM -0800, Saeed Mahameed wrote:
> +static int mlx5ctl_info_ioctl(struct file *file,
> +			      struct mlx5ctl_info __user *arg,
> +			      size_t usize)
> +{
> +	struct mlx5ctl_fd *mfd = file->private_data;
> +	struct mlx5ctl_dev *mcdev = mfd->mcdev;
> +	struct mlx5_core_dev *mdev = mcdev->mdev;
> +	struct mlx5ctl_info *info;
> +	size_t ksize = 0;
> +	int err = 0;
> +
> +	ksize = max(sizeof(struct mlx5ctl_info), usize);

Why / How can usize be larger than the structure size and you still want
to allocate a memory chunk that big?  Shouldn't the size always match?

And what if it's too small?

> +	info = kzalloc(ksize, GFP_KERNEL_ACCOUNT);

Why account as it will go away almost instantly?

> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->size = sizeof(struct mlx5ctl_info);
> +
> +	info->dev_uctx_cap = MLX5_CAP_GEN(mdev, uctx_cap);
> +	info->uctx_cap = mfd->uctx_cap;
> +	info->uctx_uid = mfd->uctx_uid;
> +	info->ucap = mfd->ucap;
> +
> +	strscpy(info->devname, dev_name(&mdev->pdev->dev),
> +		sizeof(info->devname));
> +
> +	if (copy_to_user(arg, info, usize))
> +		err = -EFAULT;

So if usize is smaller than the structure you don't copy it all?

What am I missing here?

> +
> +	kfree(info);
> +	return err;
> +}
> +
> +static long mlx5ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	struct mlx5ctl_fd *mfd = file->private_data;
> +	struct mlx5ctl_dev *mcdev = mfd->mcdev;
> +	void __user *argp = (void __user *)arg;
> +	size_t size = _IOC_SIZE(cmd);
> +	int err = 0;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	mlx5ctl_dbg(mcdev, "ioctl 0x%x type/nr: %d/%d size: %d DIR:%d\n", cmd,
> +		    _IOC_TYPE(cmd), _IOC_NR(cmd), _IOC_SIZE(cmd), _IOC_DIR(cmd));
> +
> +	down_read(&mcdev->rw_lock);
> +	if (!mcdev->mdev) {
> +		err = -ENODEV;
> +		goto unlock;
> +	}
> +
> +	switch (cmd) {
> +	case MLX5CTL_IOCTL_INFO:
> +		err = mlx5ctl_info_ioctl(file, argp, size);
> +		break;
> +
> +	default:
> +		mlx5ctl_dbg(mcdev, "Unknown ioctl %x\n", cmd);
> +		err = -ENOIOCTLCMD;

-ENOTTY is the correct error.

> --- /dev/null
> +++ b/include/uapi/misc/mlx5ctl.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 WITH Linux-syscall-note */
> +/* Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
> +
> +#ifndef __MLX5CTL_IOCTL_H__
> +#define __MLX5CTL_IOCTL_H__
> +
> +struct mlx5ctl_info {
> +	__aligned_u64 flags;

Is this used?

> +	__u32 size;
> +	__u8 devname[64]; /* underlaying ConnectX device */

64 should be a define somewhere, right?  And why 64?

> +	__u16 uctx_uid; /* current process allocated UCTX UID */
> +	__u16 reserved1;

Where is this checked to be always 0?  Well it's a read so I guess where
is the documentation saying it will always be set to 0?

> +	__u32 uctx_cap; /* current process effective UCTX cap */
> +	__u32 dev_uctx_cap; /* device's UCTX capabilities */
> +	__u32 ucap; /* process user capability */
> +	__u32 reserved2;

Same here.

And why reserve anything?  What does that help with?

thanks,

greg k-h