On Wed, Oct 2, 2024 at 2:48 PM Arnd Bergmann <arnd@xxxxxxxx> 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
>
> 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.
A quick sketch.
One option is to do something along these lines:
struct IoctlParams {
pub cmd: u32,
pub arg: usize,
}
impl IoctlParams {
fn user_slice(&self) -> IoctlUser {
let userslice = UserSlice::new(self.arg, _IOC_SIZE(self.cmd));
match _IOC_DIR(self.cmd) {
_IOC_READ => IoctlParams::Read(userslice.reader()),
_IOC_WRITE => IoctlParams::Write(userslice.writer()),
_IOC_READ|_IOC_WRITE => IoctlParams::WriteRead(userslice),
_ => unreachable!(),
}
}
}
enum IoctlUser {
Read(UserSliceReader),
Write(UserSliceWriter),
WriteRead(UserSlice),
}
Then ioctl implementations can use a match statement like this:
match ioc_params.user_slice() {
IoctlUser::Read(slice) => {},
IoctlUser::Write(slice) => {},
IoctlUser::WriteRead(slice) => {},
}
Where each branch of the match handles that case.
Alice
