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

[Arnd Bergmann]

On Wed, Oct 2, 2024, at 13:31, Alice Ryhl wrote:
> On Wed, Oct 2, 2024 at 3:25 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>>
>> On Wed, Oct 2, 2024, at 12:58, Alice Ryhl wrote:
>> > On Wed, Oct 2, 2024 at 2:48 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> > A quick sketch.
>> >
>> > One option is to do something along these lines:
>>
>> This does seem promising, at least if I read your sketch
>> correctly. I'd probably need a more concrete example to
>> understand better how this would be used in a driver.
>
> Could you point me at a driver that uses all of the features we want
> to support? Then I can try to sketch it.

drivers/media/v4l2-core/v4l2-ioctl.c probably has all of the
things we want here, plus more. This is a big ugly for having
to pass a function pointer into the video_usercopy() function
and then have both functions know about particular commands.

You can also see the effects of the compat handlers there,
e.g. VIDIOC_QUERYBUF has three possible sizes associated
with it, depending on sizeof(long) and sizeof(time_t).

There is a small optimization for buffers up to 128 bytes
to avoid the dynamic allocation, and this is likely a good
idea elsewhere as well.

>> > 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!(),
>>
>> Does the unreachable() here mean that something bad happens
>> if userspace passes something other than one of the three,
>> or are the 'cmd' values here in-kernel constants that are
>> always valid?
>
> The unreachable!() macro is equivalent to a call to BUG() .. we
> probably need to handle the fourth case too so that userspace can't
> trigger it ... but _IOC_DIR only has 4 possible return values.

As a small complication, _IOC_DIR is architecture specific,
and sometimes uses three bits that lead to four additional values
that are all invalid but could be passed by userspace.

>>
>> This is where I fail to see how that would fit in. If there
>> is a match statement in a driver, I would assume that it would
>> always match on the entire cmd code, but never have a command
>> that could with more than one _IOC_DIR type.
>
> Here's what Rust Binder does today:
>
> /// The ioctl handler.
> impl Process {
>     /// Ioctls that are write-only from the perspective of userspace.
>     ///
>     /// The kernel will only read from the pointer that userspace
> provided to us.
>     fn ioctl_write_only(
>         this: ArcBorrow<'_, Process>,
>         _file: &File,
>         cmd: u32,
>         reader: &mut UserSliceReader,
>     ) -> Result {
>         let thread = this.get_current_thread()?;
>         match cmd {
>             bindings::BINDER_SET_MAX_THREADS =>
> this.set_max_threads(reader.read()?),
>             bindings::BINDER_THREAD_EXIT => this.remove_thread(thread),
>             bindings::BINDER_SET_CONTEXT_MGR =>
> this.set_as_manager(None, &thread)?,
>             bindings::BINDER_SET_CONTEXT_MGR_EXT => {
>                 this.set_as_manager(Some(reader.read()?), &thread)?
>             }
>             bindings::BINDER_ENABLE_ONEWAY_SPAM_DETECTION => {
>                 this.set_oneway_spam_detection_enabled(reader.read()?)
>             }
>             bindings::BINDER_FREEZE => ioctl_freeze(reader)?,
>             _ => return Err(EINVAL),
>         }
>         Ok(())
>     }

I see. So the 'match cmd' bit is what we want to have
for certain, this is a sensible way to structure things.

Having the split into none/read/write/readwrite functions
feels odd to me, and this means we can't group a pair of
get/set commands together in one place, but I can also see
how this makes sense from the perspective of writing the
output buffer back to userspace.

It seems like it should be possible to validate the size of
the argument against _IOC_SIZE(cmd) at compile time, but this
is not currently done, right?

     Arnd