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

[Saeed Mahameed <saeed@kernel.org>]

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