On Wed, Oct 2, 2024 at 3:24 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > 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. Could you point me at some more complete documentation or example of how to correctly do versioning? Alice