Re: [PATCH v2 2/2] rust: miscdevice: add base miscdevice abstraction

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

On Tue, Oct 1, 2024, at 08:22, Alice Ryhl wrote:
> +#[cfg(CONFIG_COMPAT)]
> +unsafe extern "C" fn fops_compat_ioctl<T: MiscDevice>(
> +    file: *mut bindings::file,
> +    cmd: c_uint,
> +    arg: c_ulong,
> +) -> c_long {
> +    // SAFETY: The compat ioctl call of a file can access the private 
> data.
> +    let private = unsafe { (*file).private_data };
> +    // SAFETY: Ioctl calls can borrow the private data of the file.
> +    let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) 
> };
> +
> +    match T::compat_ioctl(device, cmd as u32, arg as usize) {
> +        Ok(ret) => ret as c_long,
> +        Err(err) => err.to_errno() as c_long,
> +    }
> +}

I think this works fine as a 1:1 mapping of the C API, so this
is certainly something we can do. On the other hand, it would be
nice to improve the interface in some way and make it better than
the C version.

The changes that I think would be straightforward and helpful are:

- combine native and compat handlers and pass a flag argument
  that the callback can check in case it has to do something
  special for compat mode

- pass the 'arg' value as both a __user pointer and a 'long'
  value to avoid having to cast. This specifically simplifies
  the compat version since that needs different types of
  64-bit extension for incoming 32-bit values.

On top of that, my ideal implementation would significantly
simplify writing safe ioctl handlers by using the information
encoded in the command word:

 - copy the __user data into a kernel buffer for _IOW()
   and back for _IOR() type commands, or both for _IOWR()
 - check that the argument size matches the size of the
   structure it gets assigned to

We have a couple of subsystems in the kernel that already
do something like this, but they all do it differently.
For newly written drivers in rust, we could try to do
this well from the start and only offer a single reliable
way to do it. For drivers implementing existing ioctl
commands, an additional complication is that there are
many command codes that encode incorrect size/direction
data, or none at all.

I don't know if there is a good way to do that last bit
in rust, and even if there is, we may well decide to not
do it at first in order to get something working.

      Arnd