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

[Saeed Mahameed]

On 28 Nov 09:13, Greg Kroah-Hartman wrote:
> On Mon, Nov 27, 2023 at 12:39:22PM -0800, Saeed Mahameed wrote:
> > On 27 Nov 19:09, Greg Kroah-Hartman wrote:
> > > 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?
> > >
> >
> > new user-space old kernel, the driver would allocate the usiae and make
> > sure to clear all the buffer with 0's, then fill in what the kernel
> > understands and send the whole buffer back to user with trailer always
> > zeroed out.
>
> No, at that point you know something is wrong and you need to just abort
> and return -EINVAL as the structure sizes do not match.
>
> If you need to "extend" the structure to include more information, do so
> in a new ioctl.

Ack, will remove these fields.

> > > > --- /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?
> > >
> >
> > no, not yet, but it is good for future extendibility and compatibility
> > checking.
>
> But you are not checking anything now, so please don't include something
> that will not work in the future.

Ack, will remove.

> > > > +	__u32 size;
> > > > +	__u8 devname[64]; /* underlaying ConnectX device */
> > >
> > > 64 should be a define somewhere, right?  And why 64?
> > >
> >
> > It is usually the kobj->name of the underlying device, I will have to
> > define this in the uAPI. 64 seemed large enough, any other suggestion ?
>
> What happens if the names get bigger?
>
> > This field is informational only for the user to have an idea which is the
> > underlying physical device, it's ok if in odd situation the name has to be
> > truncated to fit into the uAPI buffer.
>
> As the truncation will happen on the right side of the string, usually
> the actual device id or unique identifier, that's not going to help out
> much to drop that portion :(

Right :/, it's an assumption that mlx5 devices can either be a pci device
or an auxiliary device in case of a mlx5-subfunction, so i don't expect the
names to get larger and can easily fit in 64B string, but you are right, I
shouldn't make such assumption in an IOCTL, I will figure out something or
completely drop this field in V4.

> > > > +	__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?
> > >
> >
> > I forgot to add the checks in the info ioctl path, will add that.
> > Isn't it an unwritten rule that reserved fields has to be always 0 ?
> > Do I really need to document this ?
>
> It is a written rule that reserved fields must be 0, please see the
> documentation for how to write an ioctl.

Ack, will document.

> thanks,
>
> greg k-h