Message-ID: <61f734d6-1497-755f-3632-3f261b890846@asahilina.net>
Date:   Sat, 25 Feb 2023 11:43:56 +0900
From:   Asahi Lina <lina@...hilina.net>
To:     Gary Guo <gary@...yguo.net>, Arnd Bergmann <arnd@...db.de>
Cc:     Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
        asahi@...ts.linux.dev, Linux-Arch <linux-arch@...r.kernel.org>
Subject: Re: [PATCH] rust: ioctl: Add ioctl number manipulation functions

On 25/02/2023 11.38, Asahi Lina wrote:
> On 25/02/2023 09.38, Gary Guo wrote:
>> On Fri, 24 Feb 2023 09:43:27 +0100
>> "Arnd Bergmann" <arnd@...db.de> wrote:
>>
>>> On Fri, Feb 24, 2023, at 08:36, Asahi Lina wrote:
>>>> Add simple 1:1 wrappers of the C ioctl number manipulation functions.
>>>> Since these are macros we cannot bindgen them directly, and since they
>>>> should be usable in const context we cannot use helper wrappers, so
>>>> we'll have to reimplement them in Rust. Thankfully, the C headers do
>>>> declare defines for the relevant bitfield positions, so we don't need
>>>> to duplicate that.
>>>>
>>>> Signed-off-by: Asahi Lina <lina@...hilina.net>  
>>>
>>> I don't know much rust yet, but it looks like a correct abstraction
>>> that handles all the corner cases of architectures with unusual
>>> _IOC_*MASK combinations the same way as the C version.
>>>
>>> There is one corner case I'm not sure about:
>>>
>>>> +/// Build an ioctl number, analogous to the C macro of the same name.
>>>> +const fn _IOC(dir: u32, ty: u32, nr: u32, size: usize) -> u32 {
>>>> +    core::assert!(dir <= bindings::_IOC_DIRMASK);
>>>> +    core::assert!(ty <= bindings::_IOC_TYPEMASK);
>>>> +    core::assert!(nr <= bindings::_IOC_NRMASK);
>>>> +    core::assert!(size <= (bindings::_IOC_SIZEMASK as usize));
>>>> +
>>>> +    (dir << bindings::_IOC_DIRSHIFT)
>>>> +        | (ty << bindings::_IOC_TYPESHIFT)
>>>> +        | (nr << bindings::_IOC_NRSHIFT)
>>>> +        | ((size as u32) << bindings::_IOC_SIZESHIFT)
>>>> +}  
>>>
>>> This has the assertions inside of _IOC() while the C version
>>> has them in the outer _IOR()/_IOW() /_IOWR() helpers. This was
>>> intentional since some users of _IOC() pass a variable
>>> length in rather than sizeof(type), and this would cause
>>> a link failure in C.
>>>
>>> How is the _IOC_SIZEMASK assertion evaluated here? It's
>>> probably ok if this is a compile-time assertion that prevents
>>> the variable-length arguments, but it would be bad if this
>>> could lead to a BUG() or panic() in case of a user-supplied
>>> length that is out of range.
>>
>> This is a very good point.
>>
>> The code, as currently written, will cause a compile-time error if
>> `_IOC` is used in const contexts (i.e. used in const generics
>> arguments, or inside a `const {}` block), and it will become a runtime
>> `BUG()` if used elsewhere.
>>
>> We do have a facility to enforce compile-time checks, that's
>> `kernel::build_assert!()`. If runtime values are used and the
>> compiler can't optimise these assertions out, a link failure would
>> be triggered just like how our C code does that.
>>
>> Lina, could you change these `core::assert!` calls to build assert?
> 
> Thanks, I'll do that for v2!

Come to think of it, _IOC() isn't even public right now, so you can only
use it via the typed wrappers that take an actual struct to take the
size of. So it would only (always) panic if someone tried to use it with
a huge structure definition. But anyway, these clearly should be
compile-time assertions, so I'll change it to that. If someone wants to
use dynamic sizes in the future we can refactor it then.


~~ Lina

Powered by blists - more mailing lists