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

[Arnd Bergmann]

On Wed, Oct 2, 2024, at 14:23, Alice Ryhl wrote:
> 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:
>> >>
>> 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?

It was intentionally an example to demonstrate the worst
case one might hit, and I would hope that most drivers end
up not having to worry about them. 

To clarify: the file I mentioned is itself a piece of
infrastructure that is used to make the actual drivers
simpler, in this case by having drivers just fill out
a 'struct v4l2_ioctl_ops' with the command specific callbacks.

This works because video_ioctl2() has a clearly defined set
of commands that is shared by a large number of drivers.
For a generic bit of infrastructure, we obviously wouldn't
do anything that knows about specific commands, but the way
the get_user/put_user part works based on the size can be
quite similar.

There is similar piece of infrastructure in
drivers/gpu/drm/drm_ioctl.c, which is a bit simpler.

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

Ok.

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



     Arnd