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

[Alice Ryhl <alice@ryhl.io>]

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