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

[Christian Brauner <brauner@kernel.org>]

On Wed, Oct 02, 2024 at 12:48:12PM GMT, Arnd Bergmann wrote:
> 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

- Handle versioning by size for ioctl()s correctly so stuff like:

        /* extensible ioctls */
        switch (_IOC_NR(ioctl)) {
        case _IOC_NR(NS_MNT_GET_INFO): {
                struct mnt_ns_info kinfo = {};
                struct mnt_ns_info __user *uinfo = (struct mnt_ns_info __user *)arg;
                size_t usize = _IOC_SIZE(ioctl);

                if (ns->ops->type != CLONE_NEWNS)
                        return -EINVAL;

                if (!uinfo)
                        return -EINVAL;

                if (usize < MNT_NS_INFO_SIZE_VER0)
                        return -EINVAL;

                return copy_ns_info_to_user(to_mnt_ns(ns), uinfo, usize, &kinfo);
        }

This is not well-known and noone versions ioctl()s correctly and if they
do it's their own hand-rolled thing. Ideally, this would be a first
class concept with Rust bindings and versioning like this would be
universally enforced.

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