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
