[PATCH] rust: Add `container_of` and `offset_of` macros

[PATCH] rust: Add `container_of` and `offset_of` macros

[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

Re: [PATCH] rust: Add `container_of` and `offset_of` macros

[Martin Rodriguez Reboredo]

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>

Re: [PATCH] rust: Add `container_of` and `offset_of` macros

[Alice Ryhl]

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

Re: [PATCH] rust: Add `container_of` and `offset_of` macros

[Maíra Canal]

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

Re: [PATCH] rust: Add `container_of` and `offset_of` macros

[Alice Ryhl]

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

Re: [PATCH] rust: Add `container_of` and `offset_of` macros

[Miguel Ojeda]

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

Re: [PATCH] rust: Add `container_of` and `offset_of` macros

[Maíra Canal]

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

Re: [PATCH] rust: Add `container_of` and `offset_of` macros

[Miguel Ojeda]

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

Re: [PATCH] rust: Add `container_of` and `offset_of` macros

[Andreas Hindborg]

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