linux-kernel - Re: [PATCH v1 5/7] rust: workqueue: add helper for defining work

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 31 May 2023 12:18:47 +0200
From:   Andreas Hindborg <nmi@...aspace.dk>
To:     Alice Ryhl <aliceryhl@...gle.com>
Cc:     alex.gaynor@...il.com, benno.lossin@...ton.me,
        bjorn3_gh@...tonmail.com, boqun.feng@...il.com, gary@...yguo.net,
        jiangshanlai@...il.com, linux-kernel@...r.kernel.org,
        ojeda@...nel.org, patches@...ts.linux.dev,
        rust-for-linux@...r.kernel.org, tj@...nel.org, wedsonaf@...il.com
Subject: Re: [PATCH v1 5/7] rust: workqueue: add helper for defining
 work_struct fields


Alice Ryhl <aliceryhl@...gle.com> writes:

> Andreas Hindborg <nmi@...aspace.dk> writes:
>> Alice Ryhl <aliceryhl@...gle.com> writes:
>>> +/// Used to safely implement the [`HasWork<T>`] trait.
>>> +///
>>> +/// # Examples
>>> +///
>>> +/// ```
>>> +/// use kernel::sync::Arc;
>>> +///
>>> +/// struct MyStruct {
>>> +///     work_field: Work<Arc<MyStruct>>,
>>> +/// }
>>> +///
>>> +/// impl_has_work! {
>>> +///     impl HasWork<Arc<MyStruct>> for MyStruct { self.work_field }
>>> +/// }
>>> +/// ```
>>> +///
>>> +/// [`HasWork<T>`]: HasWork
>>> +#[macro_export]
>>> +macro_rules! impl_has_work {
>>> +    ($(impl$(<$($implarg:ident),*>)?
>>> +       HasWork<$work_type:ty>
>>> +       for $self:ident $(<$($selfarg:ident),*>)?
>>> +       { self.$field:ident }
>>> +    )*) => {$(
>>> +        // SAFETY: The implementation of `raw_get_work` only compiles if the field has the right
>>> +        // type.
>>> +        unsafe impl$(<$($implarg),*>)? $crate::workqueue::HasWork<$work_type> for $self $(<$($selfarg),*>)? {
>>> +            const OFFSET: usize = $crate::offset_of!(Self, $field) as usize;
>>> +
>>> +            #[inline]
>>> +            unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type> {
>>> +                // SAFETY: The caller promises that the pointer is not dangling.
>>> +                unsafe {
>>> +                    ::core::ptr::addr_of_mut!((*ptr).$field)
>>> +                }
>>> +            }
>> 
>> What is the reason for overriding the default implementation of `raw_get_work()`?
>> 
>> BR Andreas
>
> That's how the macro checks that the field actually has the type you
> claim it has. If you lie about the type, then `raw_get_work` will not
> compile. (See the safety comment on the impl block.)

Got it 👍

I was thinking we could do the type check without redefining the method,
but that blows up complexity wise fast, since we need a trait to do it
to support `Self` in `$work_type`. It strikes me as a bit of a hack to
overwrite an otherwise fine implementation, but I guess it is the least
complex way.

Also I am a bit annoyed that we need to state the `$work_type` type at
all, since it is available in `work_field`. But I can see no way around
that.

BR Andreas