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

[Alice Ryhl]

On Wed, Oct 2, 2024 at 3:59 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> 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.

Oh, my. That seems like a rather sophisticated ioctl handler.

Do we want all new ioctl handlers to work along those 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!(),
> >>
> >> 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.

Interesting. I did not know that.

> >> 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's the most convenient way to do it without having any
infrastructure for helping with writing ioctls. I imagine that adding
something to help with that could eliminate the reason for matching
twice in this way.

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

No, right now that validation happens at runtime. The ioctl handler
tries to use the UserSliceReader to read a struct, which fails if the
struct is too large.

I wonder if we could go for something more comprehensive than the
super simple thing I just put together. I'm sure we can validate more
things at compile time.

Alice