Messages in this thread | | | Date | Wed, 18 Oct 2023 13:08:23 +0300 | From | Leon Romanovsky <> | Subject | Re: [PATCH 3/5] misc: mlx5ctl: Add info ioctl |
| |
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
| |