Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-1019-ged83ad8595-fm-20231002.001-ged83ad85
MIME-Version: 1.0
Message-Id: <e8426444-0463-426a-892c-9493ba0cb543@app.fastmail.com>
In-Reply-To: <20231018100823.GH5392@unreal>
References: <20231018081941.475277-1-saeed@kernel.org>
 <20231018081941.475277-4-saeed@kernel.org>
 <6fb88a6a-dd66-4368-8da9-596f384fe5bc@app.fastmail.com>
 <20231018100823.GH5392@unreal>
Date:   Wed, 18 Oct 2023 13:02:49 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Leon Romanovsky" <leonro@nvidia.com>
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
Content-Type: text/plain
Precedence: bulk

On Wed, Oct 18, 2023, at 12:08, Leon Romanovsky wrote:
> 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.

Yes, the main thing is that both x86 and arm support 32-bit
user space, and there are lots of users that use those in
containers and embedded systems. Even if it's unlikely to be
used in combination with your driver, there really isn't
much reason to be different from other drivers here.


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

Ok

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

Removing the extra reserved words and the 'flags' is probably
best here, but you still need one 32-bit word to get to
a multiple of eight bytes to avoid the hole.

    Arnd