Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-1019-ged83ad8595-fm-20231002.001-ged83ad85
MIME-Version: 1.0
Message-Id: <6fb88a6a-dd66-4368-8da9-596f384fe5bc@app.fastmail.com>
In-Reply-To: <20231018081941.475277-4-saeed@kernel.org>
References: <20231018081941.475277-1-saeed@kernel.org>
 <20231018081941.475277-4-saeed@kernel.org>
Date:   Wed, 18 Oct 2023 11:02:00 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Saeed Mahameed" <saeed@kernel.org>,
        "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org,
        "Leon Romanovsky" <leonro@nvidia.com>,
        "Jason Gunthorpe" <jgg@nvidia.com>, "Jiri Pirko" <jiri@nvidia.com>,
        "Saeed Mahameed" <saeedm@nvidia.com>
Subject: Re: [PATCH 3/5] misc: mlx5ctl: Add info ioctl
Content-Type: text/plain
Precedence: bulk

On Wed, Oct 18, 2023, at 10:19, Saeed Mahameed wrote:

> Implement INFO ioctl to return the allocated UID and the capability flags
> and some other useful device information such as the underlying ConnectX
> device.

I'm commenting on the ABI here, ignoring everything for the moment.

>  static const struct file_operations mlx5ctl_fops = {
>  	.owner = THIS_MODULE,
>  	.open = mlx5ctl_open,
>  	.release = mlx5ctl_release,
> +	.unlocked_ioctl = mlx5ctl_ioctl,
>  };

There should be a .compat_ioctl entry as well, to allow 32-bit
tasks to use the same interface.
 
>  static int mlx5ctl_probe(struct auxiliary_device *adev,
> diff --git a/include/uapi/misc/mlx5ctl.h b/include/uapi/misc/mlx5ctl.h
> new file mode 100644
> index 000000000000..81d89cd285fc
> --- /dev/null
> +++ b/include/uapi/misc/mlx5ctl.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB 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;
> +	__u32 size;
> +	__u8 devname[64]; /* underlaying ConnectX device */
> +	__u16 uctx_uid; /* current process allocated UCTX UID */

I don't know what a UCTX UID is, but if this is related to
uid_t, it has to be 32 bit wide.

> +	__u16 reserved1;
> +	__u32 uctx_cap; /* current process effective UCTX cap */
> +	__u32 dev_uctx_cap; /* device's UCTX capabilities */
> +	__u32 ucap; /* process user capability */
> +	__u32 reserved2[4];
> +};

If I'm counting right, this structure has a size of
108 bytes but an alignment of 8 bytes, so you end up with
a 4-byte hole at the end, which introduces a risk of
leaking kernel data. Maybe give it an extra reserved word?

     Arnd