[3/5] misc: mlx5ctl: Add info ioctl
Commit Message
From: Saeed Mahameed <saeedm@nvidia.com>
Implement INFO ioctl to return the allocated UID and the capability flags
and some other useful device information such as the underlying ConnectX
device.
Example:
$ mlx5ctl mlx5_core.ctl.0
mlx5dev: 0000:00:04.0
UCTX UID: 1
UCTX CAP: 0x3
DEV UCTX CAP: 0x3
USER CAP: 0x1d
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
.../userspace-api/ioctl/ioctl-number.rst | 1 +
drivers/misc/mlx5ctl/main.c | 72 +++++++++++++++++++
include/uapi/misc/mlx5ctl.h | 24 +++++++
3 files changed, 97 insertions(+)
create mode 100644 include/uapi/misc/mlx5ctl.h
Comments
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
On Wed, Oct 18, 2023 at 11:02:00AM +0200, Arnd Bergmann wrote:
> 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.
Even for new code as well?
Both kernel and userspace code is new, not released yet.
>
> > 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.
UCTX UID is mlx5 hardware index, it is not uid_t.
>
> > + __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?
I think that he needs to delete __u32 reserved2[4]; completely.
Thanks
>
> Arnd
On Wed, Oct 18, 2023, at 12:08, Leon Romanovsky wrote:
> On Wed, Oct 18, 2023 at 11:02:00AM +0200, Arnd Bergmann wrote:
>> 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.
>
> Even for new code as well?
>
> Both kernel and userspace code is new, not released yet.
Yes, the main thing is that both x86 and arm support 32-bit
user space, and there are lots of users that use those in
containers and embedded systems. Even if it's unlikely to be
used in combination with your driver, there really isn't
much reason to be different from other drivers here.
>> I don't know what a UCTX UID is, but if this is related to
>> uid_t, it has to be 32 bit wide.
>
> UCTX UID is mlx5 hardware index, it is not uid_t.
Ok
>>
>> > + __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?
>
> I think that he needs to delete __u32 reserved2[4]; completely.
Removing the extra reserved words and the 'flags' is probably
best here, but you still need one 32-bit word to get to
a multiple of eight bytes to avoid the hole.
Arnd
Hi Saeed,
kernel test robot noticed the following build errors:
[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus soc/for-next rdma/for-next linus/master v6.6-rc6]
[cannot apply to next-20231020]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Saeed-Mahameed/mlx5-Add-aux-dev-for-ctl-interface/20231018-162610
base: char-misc/char-misc-testing
patch link: https://lore.kernel.org/r/20231018081941.475277-4-saeed%40kernel.org
patch subject: [PATCH 3/5] misc: mlx5ctl: Add info ioctl
config: i386-randconfig-011-20231022 (https://download.01.org/0day-ci/archive/20231022/202310220923.Lga6m0Af-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231022/202310220923.Lga6m0Af-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310220923.Lga6m0Af-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from <command-line>:
>> ./usr/include/misc/mlx5ctl.h:8:9: error: unknown type name '__aligned_u64'
8 | __aligned_u64 flags;
| ^~~~~~~~~~~~~
>> ./usr/include/misc/mlx5ctl.h:9:9: error: unknown type name '__u32'
9 | __u32 size;
| ^~~~~
>> ./usr/include/misc/mlx5ctl.h:10:9: error: unknown type name '__u8'
10 | __u8 devname[64]; /* underlaying ConnectX device */
| ^~~~
>> ./usr/include/misc/mlx5ctl.h:11:9: error: unknown type name '__u16'
11 | __u16 uctx_uid; /* current process allocated UCTX UID */
| ^~~~~
./usr/include/misc/mlx5ctl.h:12:9: error: unknown type name '__u16'
12 | __u16 reserved1;
| ^~~~~
./usr/include/misc/mlx5ctl.h:13:9: error: unknown type name '__u32'
13 | __u32 uctx_cap; /* current process effective UCTX cap */
| ^~~~~
./usr/include/misc/mlx5ctl.h:14:9: error: unknown type name '__u32'
14 | __u32 dev_uctx_cap; /* device's UCTX capabilities */
| ^~~~~
./usr/include/misc/mlx5ctl.h:15:9: error: unknown type name '__u32'
15 | __u32 ucap; /* process user capability */
| ^~~~~
./usr/include/misc/mlx5ctl.h:16:9: error: unknown type name '__u32'
16 | __u32 reserved2[4];
| ^~~~~
Hi Saeed,
kernel test robot noticed the following build errors:
[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus soc/for-next rdma/for-next linus/master v6.6-rc6]
[cannot apply to next-20231020]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Saeed-Mahameed/mlx5-Add-aux-dev-for-ctl-interface/20231018-162610
base: char-misc/char-misc-testing
patch link: https://lore.kernel.org/r/20231018081941.475277-4-saeed%40kernel.org
patch subject: [PATCH 3/5] misc: mlx5ctl: Add info ioctl
config: arc-allmodconfig (https://download.01.org/0day-ci/archive/20231022/202310221931.r1vsjtys-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231022/202310221931.r1vsjtys-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310221931.r1vsjtys-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/misc/mlx5ctl/main.c:276:27: error: initialization of 'long int (*)(struct file *, unsigned int, long unsigned int)' from incompatible pointer type 'ssize_t (*)(struct file *, unsigned int, long unsigned int)' {aka 'int (*)(struct file *, unsigned int, long unsigned int)'} [-Werror=incompatible-pointer-types]
276 | .unlocked_ioctl = mlx5ctl_ioctl,
| ^~~~~~~~~~~~~
drivers/misc/mlx5ctl/main.c:276:27: note: (near initialization for 'mlx5ctl_fops.unlocked_ioctl')
cc1: some warnings being treated as errors
vim +276 drivers/misc/mlx5ctl/main.c
271
272 static const struct file_operations mlx5ctl_fops = {
273 .owner = THIS_MODULE,
274 .open = mlx5ctl_open,
275 .release = mlx5ctl_release,
> 276 .unlocked_ioctl = mlx5ctl_ioctl,
277 };
278
@@ -89,6 +89,7 @@ Code Seq# Include File Comments
0x20 all drivers/cdrom/cm206.h
0x22 all scsi/sg.h
0x3E 00-0F linux/counter.h <mailto:linux-iio@vger.kernel.org>
+0x5c all uapi/misc/mlx5ctl.h Nvidia ConnectX control
'!' 00-1F uapi/linux/seccomp.h
'#' 00-3F IEEE 1394 Subsystem
Block for the entire subsystem
@@ -8,6 +8,7 @@
#include <linux/auxiliary_bus.h>
#include <linux/mlx5/device.h>
#include <linux/mlx5/driver.h>
+#include <uapi/misc/mlx5ctl.h>
#include <linux/atomic.h>
#include <linux/refcount.h>
@@ -198,10 +199,81 @@ static int mlx5ctl_release(struct inode *inode, struct file *file)
return 0;
}
+static int mlx5ctl_info_ioctl(struct file *file, void __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;
+ void *kbuff = NULL;
+ size_t ksize = 0;
+ int err = 0;
+
+ ksize = max(sizeof(struct mlx5ctl_info), usize);
+ kbuff = kzalloc(ksize, GFP_KERNEL_ACCOUNT);
+ if (!kbuff)
+ return -ENOMEM;
+
+ info = kbuff;
+
+ info->size = sizeof(struct mlx5ctl_info);
+ info->flags = 0;
+
+ 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, kbuff, usize))
+ err = -EFAULT;
+
+ kfree(kbuff);
+ return err;
+}
+
+static ssize_t 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;
+ break;
+ }
+unlock:
+ up_read(&mcdev->rw_lock);
+ return err;
+}
+
static const struct file_operations mlx5ctl_fops = {
.owner = THIS_MODULE,
.open = mlx5ctl_open,
.release = mlx5ctl_release,
+ .unlocked_ioctl = mlx5ctl_ioctl,
};
static int mlx5ctl_probe(struct auxiliary_device *adev,
new file mode 100644
@@ -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 */
+ __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];
+};
+
+#define MLX5CTL_IOCTL_MAGIC 0x5c
+
+#define MLX5CTL_IOCTL_INFO \
+ _IOR(MLX5CTL_IOCTL_MAGIC, 0x0, struct mlx5ctl_info)
+
+#endif /* __MLX5CTL_IOCTL_H__ */