Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.160 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.160; helo=mail.nvidia.com; pr=C
Date:   Wed, 18 Oct 2023 13:08:23 +0300
From:   Leon Romanovsky <leonro@nvidia.com>
To:     Arnd Bergmann <arnd@arndb.de>
CC:     Saeed Mahameed <saeed@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        <linux-kernel@vger.kernel.org>, Jason Gunthorpe <jgg@nvidia.com>,
        Jiri Pirko <jiri@nvidia.com>,
        Saeed Mahameed <saeedm@nvidia.com>
Subject: Re: [PATCH 3/5] misc: mlx5ctl: Add info ioctl
Message-ID: <20231018100823.GH5392@unreal>
References: <20231018081941.475277-1-saeed@kernel.org>
 <20231018081941.475277-4-saeed@kernel.org>
 <6fb88a6a-dd66-4368-8da9-596f384fe5bc@app.fastmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6fb88a6a-dd66-4368-8da9-596f384fe5bc@app.fastmail.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Oct 2023 10:08:37.3597
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 06876105-a76a-4ea6-d93d-08dbcfc23225
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.160];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: DS1PEPF0001709C.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6043
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Wed, 18 Oct 2023 03:08:46 -0700 (PDT)

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