[PATCH] rust: ioctl: Add ioctl number manipulation functions

[PATCH] rust: ioctl: Add ioctl number manipulation functions

[Asahi Lina]

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@asahilina.net>
---
 rust/bindings/bindings_helper.h |  3 +-
 rust/kernel/ioctl.rs            | 64 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |  1 +
 3 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 75d85bd6c592..aef60f300be0 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -6,8 +6,9 @@
  * Sorted alphabetically.
  */
 
-#include <linux/slab.h>
+#include <linux/ioctl.h>
 #include <linux/refcount.h>
+#include <linux/slab.h>
 
 /* `bindgen` gets confused at certain things. */
 const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
diff --git a/rust/kernel/ioctl.rs b/rust/kernel/ioctl.rs
new file mode 100644
index 000000000000..6cd8e5738b91
--- /dev/null
+++ b/rust/kernel/ioctl.rs
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0
+#![allow(non_snake_case)]
+
+//! ioctl() number definitions
+//!
+//! C header: [`include/asm-generic/ioctl.h`](../../../../include/asm-generic/ioctl.h)
+
+/// 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)
+}
+
+/// Build an ioctl number for an argumentless ioctl.
+pub const fn _IO(ty: u32, nr: u32) -> u32 {
+    _IOC(bindings::_IOC_NONE, ty, nr, 0)
+}
+
+/// Build an ioctl number for an read-only ioctl.
+pub const fn _IOR<T>(ty: u32, nr: u32) -> u32 {
+    _IOC(bindings::_IOC_READ, ty, nr, core::mem::size_of::<T>())
+}
+
+/// Build an ioctl number for an write-only ioctl.
+pub const fn _IOW<T>(ty: u32, nr: u32) -> u32 {
+    _IOC(bindings::_IOC_WRITE, ty, nr, core::mem::size_of::<T>())
+}
+
+/// Build an ioctl number for a read-write ioctl.
+pub const fn _IOWR<T>(ty: u32, nr: u32) -> u32 {
+    _IOC(
+        bindings::_IOC_READ | bindings::_IOC_WRITE,
+        ty,
+        nr,
+        core::mem::size_of::<T>(),
+    )
+}
+
+/// Get the ioctl direction from an ioctl number.
+pub const fn _IOC_DIR(nr: u32) -> u32 {
+    (nr >> bindings::_IOC_DIRSHIFT) & bindings::_IOC_DIRMASK
+}
+
+/// Get the ioctl type from an ioctl number.
+pub const fn _IOC_TYPE(nr: u32) -> u32 {
+    (nr >> bindings::_IOC_TYPESHIFT) & bindings::_IOC_TYPEMASK
+}
+
+/// Get the ioctl number from an ioctl number.
+pub const fn _IOC_NR(nr: u32) -> u32 {
+    (nr >> bindings::_IOC_NRSHIFT) & bindings::_IOC_NRMASK
+}
+
+/// Get the ioctl size from an ioctl number.
+pub const fn _IOC_SIZE(nr: u32) -> usize {
+    ((nr >> bindings::_IOC_SIZESHIFT) & bindings::_IOC_SIZEMASK) as usize
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 223564f9f0cc..7610b18ee642 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -30,6 +30,7 @@ compile_error!("Missing kernel configuration for conditional compilation");
 mod allocator;
 mod build_assert;
 pub mod error;
+pub mod ioctl;
 pub mod prelude;
 pub mod print;
 mod static_assert;

---
base-commit: 83f978b63fa7ad474ca22d7e2772c5988101c9bd
change-id: 20230224-rust-ioctl-a520f3eb3aa8

Thank you,
~~ Lina

Re: [PATCH] rust: ioctl: Add ioctl number manipulation functions

[Arnd Bergmann]

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

     Arnd

Re: [PATCH] rust: ioctl: Add ioctl number manipulation functions

[Gary Guo]

On Fri, 24 Feb 2023 09:43:27 +0100
"Arnd Bergmann" <arnd@arndb.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@asahilina.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?

Best,
Gary

Re: [PATCH] rust: ioctl: Add ioctl number manipulation functions

[Asahi Lina]

On 25/02/2023 09.38, Gary Guo wrote:
> On Fri, 24 Feb 2023 09:43:27 +0100
> "Arnd Bergmann" <arnd@arndb.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@asahilina.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!

~~ Lina

Re: [PATCH] rust: ioctl: Add ioctl number manipulation functions

[Asahi Lina]

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@arndb.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@asahilina.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