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

From: Alice Ryhl
Date: Wed Oct 02 2024 - 09:32:21 EST


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.

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

> > 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.
>
> 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(())
}

/// Ioctls that are read/write from the perspective of userspace.
///
/// The kernel will both read from and write to the pointer that
userspace provided to us.
fn ioctl_write_read(
this: ArcBorrow<'_, Process>,
file: &File,
cmd: u32,
data: UserSlice,
) -> Result {
let thread = this.get_current_thread()?;
let blocking = (file.flags() & file::flags::O_NONBLOCK) == 0;
match cmd {
bindings::BINDER_WRITE_READ => thread.write_read(data, blocking)?,
bindings::BINDER_GET_NODE_DEBUG_INFO =>
this.get_node_debug_info(data)?,
bindings::BINDER_GET_NODE_INFO_FOR_REF =>
this.get_node_info_from_ref(data)?,
bindings::BINDER_VERSION => this.version(data)?,
bindings::BINDER_GET_FROZEN_INFO => get_frozen_status(data)?,
bindings::BINDER_GET_EXTENDED_ERROR =>
thread.get_extended_error(data)?,
_ => return Err(EINVAL),
}
Ok(())
}

pub(crate) fn ioctl(this: ArcBorrow<'_, Process>, file: &File,
cmd: u32, arg: usize) -> Result {
use kernel::ioctl::{_IOC_DIR, _IOC_SIZE};
use kernel::uapi::{_IOC_READ, _IOC_WRITE};

crate::trace::trace_ioctl(cmd, arg as usize);

let user_slice = UserSlice::new(arg, _IOC_SIZE(cmd));

const _IOC_READ_WRITE: u32 = _IOC_READ | _IOC_WRITE;

let res = match _IOC_DIR(cmd) {
_IOC_WRITE => Self::ioctl_write_only(this, file, cmd, &mut
user_slice.reader()),
_IOC_READ_WRITE => Self::ioctl_write_read(this, file, cmd,
user_slice),
_ => Err(EINVAL),
};

crate::trace::trace_ioctl_done(res);
res
}
}

Alice