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

[Greg Kroah-Hartman]

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.

> > > --- /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.

> > > +	__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 :(

> > > +	__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.

thanks,

greg k-h