rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rust: Add `container_of` and `offset_of` macros
@ 2024-02-17 15:32 ` Maíra Canal
  2024-02-18 17:10   ` Martin Rodriguez Reboredo
                     ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Maíra Canal @ 2024-02-17 15:32 UTC (permalink / raw)
  To: Asahi Lina, Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Greg Kroah-Hartman, Matt Gilbride,
	Lyude Paul, Danilo Krummrich
  Cc: rust-for-linux, kernel-dev, Léo Lanteri Thauvin,
	Wedson Almeida Filho, Maíra Canal

From: Asahi Lina <lina@asahilina.net>

Add Rust counterparts to these C macros. `container_of` is useful for C
struct subtyping, to recover the original pointer to the container
structure. `offset_of` is useful for struct-relative addressing.

Co-authored-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Co-authored:by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
Signed-off-by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
Co-authored-by: Wedson Almeida Filho <wedsonaf@google.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
Signed-off-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---

Hi,

`container_of` is a essential macro to build any sort of driver in the
Linux Kernel. Therefore, we need a good and reliable implementation of
this macro for the Rust abstractions.

Although I checked the version sent by Matt [1], I don't really like the
fact that such an essential part of the foundation of any driver would
be an unsafe macro. Therefore, I propose this alternative to Matt's
implementation, which is Lina's implementation that it is actually safe.

`container_of` is a dependency for the rustgem driver and this is part
of my current efforts to upstream the driver.

[1] https://lore.kernel.org/rust-for-linux/20240205-b4-rbtree-v1-1-995e3eee38c0@google.com/

Best Regards,
- Maíra

 rust/kernel/lib.rs | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 5a8594d2f5db..7ae89c676800 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -103,3 +103,72 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
     // SAFETY: FFI call.
     unsafe { bindings::BUG() };
 }
+
+/// Calculates the offset of a field from the beginning of the struct it belongs to.
+///
+/// # Examples
+///
+/// ```rust
+/// use kernel::prelude::*;
+/// use kernel::offset_of;
+/// struct Test {
+///     a: u64,
+///     b: u32,
+/// }
+///
+/// assert_eq!(offset_of!(Test, b), 8);
+/// ```
+#[macro_export]
+macro_rules! offset_of {
+    ($type:ty, $($f:tt)*) => {{
+        let tmp = core::mem::MaybeUninit::<$type>::uninit();
+        let outer = tmp.as_ptr();
+        // To avoid warnings when nesting `unsafe` blocks.
+        #[allow(unused_unsafe)]
+        // SAFETY: The pointer is valid and aligned, just not initialised; `addr_of` ensures that
+        // we don't actually read from `outer` (which would be UB) nor create an intermediate
+        // reference.
+        let inner = unsafe { core::ptr::addr_of!((*outer).$($f)*) } as *const u8;
+        // To avoid warnings when nesting `unsafe` blocks.
+        #[allow(unused_unsafe)]
+        // SAFETY: The two pointers are within the same allocation block.
+        unsafe { inner.offset_from(outer as *const u8) }
+    }}
+}
+
+/// Produces a pointer to an object from a pointer to one of its fields.
+///
+/// # Safety
+///
+/// Callers must ensure that the pointer to the field is in fact a pointer to the specified field,
+/// as opposed to a pointer to another object of the same type. If this condition is not met,
+/// any dereference of the resulting pointer is UB.
+///
+/// # Examples
+///
+/// ```rust
+/// use kernel::container_of;
+/// struct Test {
+///     a: u64,
+///     b: u32,
+/// }
+///
+/// let test = Test { a: 10, b: 20 };
+/// let b_ptr = &test.b;
+/// let test_alias = container_of!(b_ptr, Test, b);
+/// assert!(core::ptr::eq(&test, test_alias));
+/// ```
+#[macro_export]
+macro_rules! container_of {
+    ($ptr:expr, $type:ty, $($f:tt)*) => {{
+        let ptr = $ptr as *const _ as *const u8;
+        let offset = $crate::offset_of!($type, $($f)*);
+        let outer = ptr.wrapping_offset(-offset) as *const $type;
+        // SAFETY: The pointer is valid and aligned, just not initialised; `addr_of` ensures that
+        // we don't actually read from `outer` (which would be UB) nor create an intermediate
+        // reference. The two pointers are within the same allocation block.
+        let inner = unsafe { core::ptr::addr_of!((*outer).$($f)*) };
+        build_assert!(inner == $ptr);
+        outer
+    }}
+}
--
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] rust: Add `container_of` and `offset_of` macros
  2024-02-17 15:32 ` [PATCH] rust: Add `container_of` and `offset_of` macros Maíra Canal
@ 2024-02-18 17:10   ` Martin Rodriguez Reboredo
  2024-02-19  9:49   ` Alice Ryhl
  2024-02-19  9:51   ` Andreas Hindborg
  2 siblings, 0 replies; 9+ messages in thread
From: Martin Rodriguez Reboredo @ 2024-02-18 17:10 UTC (permalink / raw)
  To: Maíra Canal, Asahi Lina, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Greg Kroah-Hartman,
	Matt Gilbride, Lyude Paul, Danilo Krummrich
  Cc: rust-for-linux, kernel-dev, Léo Lanteri Thauvin,
	Wedson Almeida Filho

On 2/17/24 12:32, Maíra Canal wrote:
> From: Asahi Lina <lina@asahilina.net>
> 
> Add Rust counterparts to these C macros. `container_of` is useful for C
> struct subtyping, to recover the original pointer to the container
> structure. `offset_of` is useful for struct-relative addressing.
> 
> Co-authored-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Co-authored:by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
> Signed-off-by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
> Co-authored-by: Wedson Almeida Filho <wedsonaf@google.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>

A handful of authors? So then, it would be sensible to list their
contributions to the patch, though, it could be done when merged.

> [...]
> +/// Calculates the offset of a field from the beginning of the struct it belongs to.
> +///
> +/// # Examples
> +///
> +/// ```rust
> +/// use kernel::prelude::*;
> +/// use kernel::offset_of;
> +/// struct Test {
> +///     a: u64,
> +///     b: u32,
> +/// }
> +///
> +/// assert_eq!(offset_of!(Test, b), 8);

Would've added a check for field `a`, but that's my personal preference.

> +/// ```
> +#[macro_export]
> +macro_rules! offset_of {
> +    ($type:ty, $($f:tt)*) => {{
> +        let tmp = core::mem::MaybeUninit::<$type>::uninit();
> +        let outer = tmp.as_ptr();
> +        // To avoid warnings when nesting `unsafe` blocks.
> +        #[allow(unused_unsafe)]
> +        // SAFETY: The pointer is valid and aligned, just not initialised; `addr_of` ensures that
> +        // we don't actually read from `outer` (which would be UB) nor create an intermediate
> +        // reference.
> +        let inner = unsafe { core::ptr::addr_of!((*outer).$($f)*) } as *const u8;

Curiously enough, you can get the offset of a member of a field struct
per what `addr_of!` does, i.e. if we have a type `Foo` that contains a
struct field `Bar` which has member `qux`, then
`offset_of!(Foo, bar.qux)` will give out an offset too. I don't know if
this applies for `container_of!`. Just saying.

> [...]

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rust: Add `container_of` and `offset_of` macros
  2024-02-17 15:32 ` [PATCH] rust: Add `container_of` and `offset_of` macros Maíra Canal
  2024-02-18 17:10   ` Martin Rodriguez Reboredo
@ 2024-02-19  9:49   ` Alice Ryhl
  2024-02-19 21:57     ` Maíra Canal
  2024-02-19  9:51   ` Andreas Hindborg
  2 siblings, 1 reply; 9+ messages in thread
From: Alice Ryhl @ 2024-02-19  9:49 UTC (permalink / raw)
  To: Maíra Canal
  Cc: Asahi Lina, Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Greg Kroah-Hartman, Matt Gilbride, Lyude Paul,
	Danilo Krummrich, rust-for-linux, kernel-dev,
	Léo Lanteri Thauvin, Wedson Almeida Filho

On Sat, Feb 17, 2024 at 4:33 PM Maíra Canal <mcanal@igalia.com> wrote:
>
> From: Asahi Lina <lina@asahilina.net>
>
> Add Rust counterparts to these C macros. `container_of` is useful for C
> struct subtyping, to recover the original pointer to the container
> structure. `offset_of` is useful for struct-relative addressing.
>
> Co-authored-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Co-authored:by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
> Signed-off-by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
> Co-authored-by: Wedson Almeida Filho <wedsonaf@google.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>
> Hi,
>
> `container_of` is a essential macro to build any sort of driver in the
> Linux Kernel. Therefore, we need a good and reliable implementation of
> this macro for the Rust abstractions.
>
> Although I checked the version sent by Matt [1], I don't really like the
> fact that such an essential part of the foundation of any driver would
> be an unsafe macro. Therefore, I propose this alternative to Matt's
> implementation, which is Lina's implementation that it is actually safe.

Well, the addr_of macro in the standard library, which performs the
opposite operation is also unsafe. So I don't think that having it be
unsafe is that unreasonable.

> `container_of` is a dependency for the rustgem driver and this is part
> of my current efforts to upstream the driver.
>
> [1] https://lore.kernel.org/rust-for-linux/20240205-b4-rbtree-v1-1-995e3eee38c0@google.com/
>
> Best Regards,
> - Maíra
>
>  rust/kernel/lib.rs | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 5a8594d2f5db..7ae89c676800 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -103,3 +103,72 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>      // SAFETY: FFI call.
>      unsafe { bindings::BUG() };
>  }
> +
> +/// Calculates the offset of a field from the beginning of the struct it belongs to.
> +///
> +/// # Examples
> +///
> +/// ```rust
> +/// use kernel::prelude::*;
> +/// use kernel::offset_of;
> +/// struct Test {
> +///     a: u64,
> +///     b: u32,
> +/// }
> +///
> +/// assert_eq!(offset_of!(Test, b), 8);
> +/// ```
> +#[macro_export]
> +macro_rules! offset_of {
> +    ($type:ty, $($f:tt)*) => {{
> +        let tmp = core::mem::MaybeUninit::<$type>::uninit();
> +        let outer = tmp.as_ptr();
> +        // To avoid warnings when nesting `unsafe` blocks.
> +        #[allow(unused_unsafe)]
> +        // SAFETY: The pointer is valid and aligned, just not initialised; `addr_of` ensures that
> +        // we don't actually read from `outer` (which would be UB) nor create an intermediate
> +        // reference.
> +        let inner = unsafe { core::ptr::addr_of!((*outer).$($f)*) } as *const u8;
> +        // To avoid warnings when nesting `unsafe` blocks.
> +        #[allow(unused_unsafe)]
> +        // SAFETY: The two pointers are within the same allocation block.
> +        unsafe { inner.offset_from(outer as *const u8) }
> +    }}
> +}

The offset_of macro is already available through the standard library.
You do not need to add it here.

> +/// Produces a pointer to an object from a pointer to one of its fields.
> +///
> +/// # Safety
> +///
> +/// Callers must ensure that the pointer to the field is in fact a pointer to the specified field,
> +/// as opposed to a pointer to another object of the same type. If this condition is not met,
> +/// any dereference of the resulting pointer is UB.
> +///
> +/// # Examples
> +///
> +/// ```rust
> +/// use kernel::container_of;
> +/// struct Test {
> +///     a: u64,
> +///     b: u32,
> +/// }
> +///
> +/// let test = Test { a: 10, b: 20 };
> +/// let b_ptr = &test.b;
> +/// let test_alias = container_of!(b_ptr, Test, b);
> +/// assert!(core::ptr::eq(&test, test_alias));
> +/// ```
> +#[macro_export]
> +macro_rules! container_of {
> +    ($ptr:expr, $type:ty, $($f:tt)*) => {{
> +        let ptr = $ptr as *const _ as *const u8;
> +        let offset = $crate::offset_of!($type, $($f)*);
> +        let outer = ptr.wrapping_offset(-offset) as *const $type;
> +        // SAFETY: The pointer is valid and aligned, just not initialised; `addr_of` ensures that
> +        // we don't actually read from `outer` (which would be UB) nor create an intermediate
> +        // reference. The two pointers are within the same allocation block.
> +        let inner = unsafe { core::ptr::addr_of!((*outer).$($f)*) };

If your macro is not unsafe, then this line is incorrect. I could call
`container_of!` with some random dangling pointer, and then it would
use addr_of! on that pointer, which is not okay because addr_of
assumes that the pointer is in-bounds of an allocation both before and
after the offset operation.

Alice

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rust: Add `container_of` and `offset_of` macros
  2024-02-17 15:32 ` [PATCH] rust: Add `container_of` and `offset_of` macros Maíra Canal
  2024-02-18 17:10   ` Martin Rodriguez Reboredo
  2024-02-19  9:49   ` Alice Ryhl
@ 2024-02-19  9:51   ` Andreas Hindborg
  2 siblings, 0 replies; 9+ messages in thread
From: Andreas Hindborg @ 2024-02-19  9:51 UTC (permalink / raw)
  To: Maíra Canal
  Cc: Asahi Lina, Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Alice Ryhl, Greg Kroah-Hartman, Matt Gilbride, Lyude Paul,
	Danilo Krummrich, rust-for-linux, kernel-dev,
	Léo Lanteri Thauvin, Wedson Almeida Filho


Hi Maira,

Maíra Canal <mcanal@igalia.com> writes:

> From: Asahi Lina <lina@asahilina.net>
>
> Add Rust counterparts to these C macros. `container_of` is useful for C
> struct subtyping, to recover the original pointer to the container
> structure. `offset_of` is useful for struct-relative addressing.
>
> Co-authored-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Co-authored:by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
> Signed-off-by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
> Co-authored-by: Wedson Almeida Filho <wedsonaf@google.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>
> Hi,
>
> `container_of` is a essential macro to build any sort of driver in the
> Linux Kernel. Therefore, we need a good and reliable implementation of
> this macro for the Rust abstractions.
>
> Although I checked the version sent by Matt [1], I don't really like the
> fact that such an essential part of the foundation of any driver would
> be an unsafe macro. Therefore, I propose this alternative to Matt's
> implementation, which is Lina's implementation that it is actually safe.
>
> `container_of` is a dependency for the rustgem driver and this is part
> of my current efforts to upstream the driver.
>

`offset_of!()` is available as `core::mem::offset_of!()` under unstable
feature gate, which is currently enabled and used in the workqueue
abstractions. So at least that macro is not required.

Best regards,
Andreas

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rust: Add `container_of` and `offset_of` macros
  2024-02-19  9:49   ` Alice Ryhl
@ 2024-02-19 21:57     ` Maíra Canal
  2024-02-19 22:04       ` Alice Ryhl
  2024-02-20 15:54       ` Miguel Ojeda
  0 siblings, 2 replies; 9+ messages in thread
From: Maíra Canal @ 2024-02-19 21:57 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Asahi Lina, Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Greg Kroah-Hartman, Matt Gilbride, Lyude Paul,
	Danilo Krummrich, rust-for-linux, kernel-dev,
	Léo Lanteri Thauvin, Wedson Almeida Filho

Hi Alice,

On 2/19/24 06:49, Alice Ryhl wrote:
> On Sat, Feb 17, 2024 at 4:33 PM Maíra Canal <mcanal@igalia.com> wrote:
>>
>> From: Asahi Lina <lina@asahilina.net>
>>
>> Add Rust counterparts to these C macros. `container_of` is useful for C
>> struct subtyping, to recover the original pointer to the container
>> structure. `offset_of` is useful for struct-relative addressing.
>>
>> Co-authored-by: Miguel Ojeda <ojeda@kernel.org>
>> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
>> Co-authored:by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
>> Signed-off-by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
>> Co-authored-by: Wedson Almeida Filho <wedsonaf@google.com>
>> Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
>> Signed-off-by: Asahi Lina <lina@asahilina.net>
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>
>> Hi,
>>
>> `container_of` is a essential macro to build any sort of driver in the
>> Linux Kernel. Therefore, we need a good and reliable implementation of
>> this macro for the Rust abstractions.
>>
>> Although I checked the version sent by Matt [1], I don't really like the
>> fact that such an essential part of the foundation of any driver would
>> be an unsafe macro. Therefore, I propose this alternative to Matt's
>> implementation, which is Lina's implementation that it is actually safe.
> 
> Well, the addr_of macro in the standard library, which performs the
> opposite operation is also unsafe. So I don't think that having it be
> unsafe is that unreasonable.
> 
>> `container_of` is a dependency for the rustgem driver and this is part
>> of my current efforts to upstream the driver.
>>
>> [1] https://lore.kernel.org/rust-for-linux/20240205-b4-rbtree-v1-1-995e3eee38c0@google.com/
>>
>> Best Regards,
>> - Maíra
>>
>>   rust/kernel/lib.rs | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 69 insertions(+)
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 5a8594d2f5db..7ae89c676800 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -103,3 +103,72 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>>       // SAFETY: FFI call.
>>       unsafe { bindings::BUG() };
>>   }
>> +
>> +/// Calculates the offset of a field from the beginning of the struct it belongs to.
>> +///
>> +/// # Examples
>> +///
>> +/// ```rust
>> +/// use kernel::prelude::*;
>> +/// use kernel::offset_of;
>> +/// struct Test {
>> +///     a: u64,
>> +///     b: u32,
>> +/// }
>> +///
>> +/// assert_eq!(offset_of!(Test, b), 8);
>> +/// ```
>> +#[macro_export]
>> +macro_rules! offset_of {
>> +    ($type:ty, $($f:tt)*) => {{
>> +        let tmp = core::mem::MaybeUninit::<$type>::uninit();
>> +        let outer = tmp.as_ptr();
>> +        // To avoid warnings when nesting `unsafe` blocks.
>> +        #[allow(unused_unsafe)]
>> +        // SAFETY: The pointer is valid and aligned, just not initialised; `addr_of` ensures that
>> +        // we don't actually read from `outer` (which would be UB) nor create an intermediate
>> +        // reference.
>> +        let inner = unsafe { core::ptr::addr_of!((*outer).$($f)*) } as *const u8;
>> +        // To avoid warnings when nesting `unsafe` blocks.
>> +        #[allow(unused_unsafe)]
>> +        // SAFETY: The two pointers are within the same allocation block.
>> +        unsafe { inner.offset_from(outer as *const u8) }
>> +    }}
>> +}
> 
> The offset_of macro is already available through the standard library.
> You do not need to add it here.
> 
>> +/// Produces a pointer to an object from a pointer to one of its fields.
>> +///
>> +/// # Safety
>> +///
>> +/// Callers must ensure that the pointer to the field is in fact a pointer to the specified field,
>> +/// as opposed to a pointer to another object of the same type. If this condition is not met,
>> +/// any dereference of the resulting pointer is UB.
>> +///
>> +/// # Examples
>> +///
>> +/// ```rust
>> +/// use kernel::container_of;
>> +/// struct Test {
>> +///     a: u64,
>> +///     b: u32,
>> +/// }
>> +///
>> +/// let test = Test { a: 10, b: 20 };
>> +/// let b_ptr = &test.b;
>> +/// let test_alias = container_of!(b_ptr, Test, b);
>> +/// assert!(core::ptr::eq(&test, test_alias));
>> +/// ```
>> +#[macro_export]
>> +macro_rules! container_of {
>> +    ($ptr:expr, $type:ty, $($f:tt)*) => {{
>> +        let ptr = $ptr as *const _ as *const u8;
>> +        let offset = $crate::offset_of!($type, $($f)*);
>> +        let outer = ptr.wrapping_offset(-offset) as *const $type;
>> +        // SAFETY: The pointer is valid and aligned, just not initialised; `addr_of` ensures that
>> +        // we don't actually read from `outer` (which would be UB) nor create an intermediate
>> +        // reference. The two pointers are within the same allocation block.
>> +        let inner = unsafe { core::ptr::addr_of!((*outer).$($f)*) };
> 
> If your macro is not unsafe, then this line is incorrect. I could call
> `container_of!` with some random dangling pointer, and then it would
> use addr_of! on that pointer, which is not okay because addr_of
> assumes that the pointer is in-bounds of an allocation both before and
> after the offset operation.
> 

Thanks for your explanation, I haven't thought through this point of
view. Anyway, any chance we could land the `container_of!` patch as soon
as possible? It would be nice to have a common understanding of this 
fundamental macro. I would like to send some patches with the DMA fences
abstraction, but I would need `container_of!`.

Best Regards,
- Maíra

> Alice

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rust: Add `container_of` and `offset_of` macros
  2024-02-19 21:57     ` Maíra Canal
@ 2024-02-19 22:04       ` Alice Ryhl
  2024-02-20 15:54       ` Miguel Ojeda
  1 sibling, 0 replies; 9+ messages in thread
From: Alice Ryhl @ 2024-02-19 22:04 UTC (permalink / raw)
  To: Maíra Canal, Alice Ryhl
  Cc: Asahi Lina, Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Greg Kroah-Hartman, Matt Gilbride, Lyude Paul,
	Danilo Krummrich, rust-for-linux, kernel-dev,
	Léo Lanteri Thauvin, Wedson Almeida Filho

On 2/19/24 22:57, Maíra Canal wrote:
> Hi Alice,
> 
> On 2/19/24 06:49, Alice Ryhl wrote:
>> On Sat, Feb 17, 2024 at 4:33 PM Maíra Canal <mcanal@igalia.com> wrote:
>>>
>>> From: Asahi Lina <lina@asahilina.net>
>>>
>>> Add Rust counterparts to these C macros. `container_of` is useful for C
>>> struct subtyping, to recover the original pointer to the container
>>> structure. `offset_of` is useful for struct-relative addressing.
>>>
>>> Co-authored-by: Miguel Ojeda <ojeda@kernel.org>
>>> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
>>> Co-authored:by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
>>> Signed-off-by: Léo Lanteri Thauvin <leseulartichaut@gmail.com>
>>> Co-authored-by: Wedson Almeida Filho <wedsonaf@google.com>
>>> Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
>>> Signed-off-by: Asahi Lina <lina@asahilina.net>
>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>>> ---
>>>
>>> Hi,
>>>
>>> `container_of` is a essential macro to build any sort of driver in the
>>> Linux Kernel. Therefore, we need a good and reliable implementation of
>>> this macro for the Rust abstractions.
>>>
>>> Although I checked the version sent by Matt [1], I don't really like the
>>> fact that such an essential part of the foundation of any driver would
>>> be an unsafe macro. Therefore, I propose this alternative to Matt's
>>> implementation, which is Lina's implementation that it is actually safe.
>>
>> Well, the addr_of macro in the standard library, which performs the
>> opposite operation is also unsafe. So I don't think that having it be
>> unsafe is that unreasonable.
>>
>>> `container_of` is a dependency for the rustgem driver and this is part
>>> of my current efforts to upstream the driver.
>>>
>>> [1] 
>>> https://lore.kernel.org/rust-for-linux/20240205-b4-rbtree-v1-1-995e3eee38c0@google.com/
>>>
>>> Best Regards,
>>> - Maíra
>>>
>>>   rust/kernel/lib.rs | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 69 insertions(+)
>>>
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index 5a8594d2f5db..7ae89c676800 100644
>>> --- a/rust/kernel/lib.rs
>>> +++ b/rust/kernel/lib.rs
>>> @@ -103,3 +103,72 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>>>       // SAFETY: FFI call.
>>>       unsafe { bindings::BUG() };
>>>   }
>>> +
>>> +/// Calculates the offset of a field from the beginning of the 
>>> struct it belongs to.
>>> +///
>>> +/// # Examples
>>> +///
>>> +/// ```rust
>>> +/// use kernel::prelude::*;
>>> +/// use kernel::offset_of;
>>> +/// struct Test {
>>> +///     a: u64,
>>> +///     b: u32,
>>> +/// }
>>> +///
>>> +/// assert_eq!(offset_of!(Test, b), 8);
>>> +/// ```
>>> +#[macro_export]
>>> +macro_rules! offset_of {
>>> +    ($type:ty, $($f:tt)*) => {{
>>> +        let tmp = core::mem::MaybeUninit::<$type>::uninit();
>>> +        let outer = tmp.as_ptr();
>>> +        // To avoid warnings when nesting `unsafe` blocks.
>>> +        #[allow(unused_unsafe)]
>>> +        // SAFETY: The pointer is valid and aligned, just not 
>>> initialised; `addr_of` ensures that
>>> +        // we don't actually read from `outer` (which would be UB) 
>>> nor create an intermediate
>>> +        // reference.
>>> +        let inner = unsafe { core::ptr::addr_of!((*outer).$($f)*) } 
>>> as *const u8;
>>> +        // To avoid warnings when nesting `unsafe` blocks.
>>> +        #[allow(unused_unsafe)]
>>> +        // SAFETY: The two pointers are within the same allocation 
>>> block.
>>> +        unsafe { inner.offset_from(outer as *const u8) }
>>> +    }}
>>> +}
>>
>> The offset_of macro is already available through the standard library.
>> You do not need to add it here.
>>
>>> +/// Produces a pointer to an object from a pointer to one of its 
>>> fields.
>>> +///
>>> +/// # Safety
>>> +///
>>> +/// Callers must ensure that the pointer to the field is in fact a 
>>> pointer to the specified field,
>>> +/// as opposed to a pointer to another object of the same type. If 
>>> this condition is not met,
>>> +/// any dereference of the resulting pointer is UB.
>>> +///
>>> +/// # Examples
>>> +///
>>> +/// ```rust
>>> +/// use kernel::container_of;
>>> +/// struct Test {
>>> +///     a: u64,
>>> +///     b: u32,
>>> +/// }
>>> +///
>>> +/// let test = Test { a: 10, b: 20 };
>>> +/// let b_ptr = &test.b;
>>> +/// let test_alias = container_of!(b_ptr, Test, b);
>>> +/// assert!(core::ptr::eq(&test, test_alias));
>>> +/// ```
>>> +#[macro_export]
>>> +macro_rules! container_of {
>>> +    ($ptr:expr, $type:ty, $($f:tt)*) => {{
>>> +        let ptr = $ptr as *const _ as *const u8;
>>> +        let offset = $crate::offset_of!($type, $($f)*);
>>> +        let outer = ptr.wrapping_offset(-offset) as *const $type;
>>> +        // SAFETY: The pointer is valid and aligned, just not 
>>> initialised; `addr_of` ensures that
>>> +        // we don't actually read from `outer` (which would be UB) 
>>> nor create an intermediate
>>> +        // reference. The two pointers are within the same 
>>> allocation block.
>>> +        let inner = unsafe { core::ptr::addr_of!((*outer).$($f)*) };
>>
>> If your macro is not unsafe, then this line is incorrect. I could call
>> `container_of!` with some random dangling pointer, and then it would
>> use addr_of! on that pointer, which is not okay because addr_of
>> assumes that the pointer is in-bounds of an allocation both before and
>> after the offset operation.
>>
> 
> Thanks for your explanation, I haven't thought through this point of
> view. Anyway, any chance we could land the `container_of!` patch as soon
> as possible? It would be nice to have a common understanding of this 
> fundamental macro. I would like to send some patches with the DMA fences
> abstraction, but I would need `container_of!`.

When you send a patchset, you can just state in the coverletter that it 
depends on the container_of patch in the rbtree patchset. Then Miguel 
will know that he needs to take that before he can take your patchset.

Of course, if Miguel can take it soon, that's even better.

Alice

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rust: Add `container_of` and `offset_of` macros
  2024-02-19 21:57     ` Maíra Canal
  2024-02-19 22:04       ` Alice Ryhl
@ 2024-02-20 15:54       ` Miguel Ojeda
  2024-02-21 11:27         ` Maíra Canal
  1 sibling, 1 reply; 9+ messages in thread
From: Miguel Ojeda @ 2024-02-20 15:54 UTC (permalink / raw)
  To: Maíra Canal
  Cc: Alice Ryhl, Asahi Lina, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Greg Kroah-Hartman,
	Matt Gilbride, Lyude Paul, Danilo Krummrich, rust-for-linux,
	kernel-dev, Léo Lanteri Thauvin, Wedson Almeida Filho

On Mon, Feb 19, 2024 at 10:57 PM Maíra Canal <mcanal@igalia.com> wrote:
>
> Thanks for your explanation, I haven't thought through this point of
> view. Anyway, any chance we could land the `container_of!` patch as soon
> as possible? It would be nice to have a common understanding of this
> fundamental macro. I would like to send some patches with the DMA fences
> abstraction, but I would need `container_of!`.

Would you be OK with their version? If so, I could take that one; and
if not, we can discuss what to do.

But, in any case, as Alice mentioned, please feel free to send your
series even before that. It is anyway a very small dependency that
will soon go in.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rust: Add `container_of` and `offset_of` macros
  2024-02-20 15:54       ` Miguel Ojeda
@ 2024-02-21 11:27         ` Maíra Canal
  2024-02-25 20:30           ` Miguel Ojeda
  0 siblings, 1 reply; 9+ messages in thread
From: Maíra Canal @ 2024-02-21 11:27 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Alice Ryhl, Asahi Lina, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Greg Kroah-Hartman,
	Matt Gilbride, Lyude Paul, Danilo Krummrich, rust-for-linux,
	kernel-dev, Léo Lanteri Thauvin, Wedson Almeida Filho

Hi Miguel,

On 2/20/24 12:54, Miguel Ojeda wrote:
> On Mon, Feb 19, 2024 at 10:57 PM Maíra Canal <mcanal@igalia.com> wrote:
>>
>> Thanks for your explanation, I haven't thought through this point of
>> view. Anyway, any chance we could land the `container_of!` patch as soon
>> as possible? It would be nice to have a common understanding of this
>> fundamental macro. I would like to send some patches with the DMA fences
>> abstraction, but I would need `container_of!`.
> 
> Would you be OK with their version? If so, I could take that one; and
> if not, we can discuss what to do.

Considering Alice's points, I'm OK with their version.

Best Regards,
- Maíra

> 
> But, in any case, as Alice mentioned, please feel free to send your
> series even before that. It is anyway a very small dependency that
> will soon go in.
> 
> Cheers,
> Miguel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rust: Add `container_of` and `offset_of` macros
  2024-02-21 11:27         ` Maíra Canal
@ 2024-02-25 20:30           ` Miguel Ojeda
  0 siblings, 0 replies; 9+ messages in thread
From: Miguel Ojeda @ 2024-02-25 20:30 UTC (permalink / raw)
  To: Maíra Canal
  Cc: Alice Ryhl, Asahi Lina, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Greg Kroah-Hartman,
	Matt Gilbride, Lyude Paul, Danilo Krummrich, rust-for-linux,
	kernel-dev, Léo Lanteri Thauvin, Wedson Almeida Filho

On Wed, Feb 21, 2024 at 12:27 PM Maíra Canal <mcanal@igalia.com> wrote:
>
> Considering Alice's points, I'm OK with their version.

Thanks! Their version is in `rust-next` now.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-02-25 20:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20240217153356eucas1p17c926b49da33a438d286ebb5c1a918c9@eucas1p1.samsung.com>
2024-02-17 15:32 ` [PATCH] rust: Add `container_of` and `offset_of` macros Maíra Canal
2024-02-18 17:10   ` Martin Rodriguez Reboredo
2024-02-19  9:49   ` Alice Ryhl
2024-02-19 21:57     ` Maíra Canal
2024-02-19 22:04       ` Alice Ryhl
2024-02-20 15:54       ` Miguel Ojeda
2024-02-21 11:27         ` Maíra Canal
2024-02-25 20:30           ` Miguel Ojeda
2024-02-19  9:51   ` Andreas Hindborg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).