Re: [PATCH v4 2/4] samples: rust: Provide example using the new Rust MiscDevice abstraction

["Arnd Bergmann" <arnd@arndb.de>]

On Fri, Dec 6, 2024, at 11:09, Alice Ryhl wrote:
> On Fri, Dec 6, 2024 at 11:05 AM Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Fri, Dec 6, 2024, at 10:05, Lee Jones wrote:
>> > This sample driver demonstrates the following basic operations:
>> >
>> > * Register a Misc Device
>> > * Create /dev/rust-misc-device
>> > * Provide open call-back for the aforementioned character device
>> > * Operate on the character device via a simple ioctl()
>> > * Provide close call-back for the character device
>> >
>> > Signed-off-by: Lee Jones <lee@kernel.org>
>>
>> Could you include a compat_ioctl() callback in the example?
>> I think it would be good to include it as a reminder for
>> authors of actual drivers that every driver implementing
>> ioctl should also implement compat_ioctl. In C drivers, this
>> can usually be done by pointing .compat_ioctl() to the
>> generic compat_ptr_ioctl() function, which assumes that 'arg'
>> is a pointer disguised as an 'unsigned long'.
>
> The current Rust logic for building the fops table will use
> compat_ptr_ioctl() automatically if you specify ioctl() but don't
> specify compat_ioctl(), so this already uses compat_ptr_ioctl(). But
> maybe that's not what we want?

Ok, got it. It's usually the right thing to do, but it's easy
to get wrong if there is at least one ioctl command that actually
needs an integer argument instead of a pointer.

Almost all command definitions are for either no argument or
a pointer argument, and compat_ptr_ioctl() works fine there, by
doing a conversion from a 32-bit pointer to a 64-bit pointer
by zero-extending the upper 33 (on s390) or 32 bits (everywhere
else). Integer values need to either a 32-bit sign-extension
or a 32-bit zero-extension depending on how the argument is
interpreted on 32-bit architectures.

I wonder if we should change the prototype of the ioctl
callback to always pass a __user pointer and just not allow
the few commands that pass an integer in rust drivers, and
worry about it only when it's absolutely needed.

     Arnd