linux-kernel - Re: [PATCH v2 5/8] 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:   Fri,  2 Jun 2023 09:37:41 +0000
From:   Alice Ryhl <aliceryhl@...gle.com>
To:     boqun.feng@...il.com
Cc:     alex.gaynor@...il.com, aliceryhl@...gle.com,
        benno.lossin@...ton.me, bjorn3_gh@...tonmail.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 v2 5/8] rust: workqueue: add helper for defining
 work_struct fields

Boqun Feng <boqun.feng@...il.com> writes:
> On Thu, Jun 01, 2023 at 01:49:43PM +0000, Alice Ryhl wrote:
>> +/// Defines the method that should be called directly when a work item is executed.
>> +///
>> +/// Typically you would implement [`WorkItem`] instead. The `run` method on this trait will
>> +/// usually just perform the appropriate `container_of` translation and then call into the `run`
>> +/// method from the [`WorkItem`] trait.
>> +///
>> +/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
>> +///
>> +/// # Safety
>> +///
>> +/// Implementers must ensure that [`__enqueue`] uses a `work_struct` initialized with the [`run`]
>> +/// method of this trait as the function pointer.
>> +///
>> +/// [`__enqueue`]: RawWorkItem::__enqueue
>> +/// [`run`]: WorkItemPointer::run
>> +pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> {
>> +    /// Run this work item.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The provided `work_struct` pointer must originate from a previous call to `__enqueue` where
>> +    /// the `queue_work_on` closure returned true, and the pointer must still be valid.
>> +    unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
>> +}
>> +
>> +/// Defines the method that should be called when this work item is executed.
>> +///
>> +/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
>> +pub trait WorkItem<const ID: u64 = 0> {
>> +    /// The pointer type that this struct is wrapped in. This will typically be `Arc<Self>` or
>> +    /// `Pin<Box<Self>>`.
>> +    type Pointer: WorkItemPointer<ID>;
> 
> This being an associate type makes me wonder how do we want to support
> the following (totally made-up by me, but I think it makes sense)?:
> 
> Say we have a struct
> 
> 	pub struct Foo {
> 		work: Work<Foo>,
> 		data: Data,
> 	}
> 
> 	impl Foo {
> 		pub fn do_sth(&self) {
> 			...
> 		}
> 	}
> 
> and we want to queue both Pin<Box<Foo>> and Arc<Foo> as work items, but
> the following doesn't work:
> 
> 	// Pin<Box<Foo>> can be queued.
> 	impl WorkItem for Foo {
> 		type Pointer = Pin<Box<Foo>>;
> 		fn run(ptr: Self::Pointer) {
> 			ptr.do_sth();
> 		}
> 	}
> 
> 	// Arc<Foo> can also be queued.
> 	impl WorkItem for Foo {
> 		type Pointer = Arc<Foo>;
> 		fn run(ptr: Self::Pointer) {
> 			ptr.do_sth();
> 		}
> 	}
> 
> Of course, we can use new type idiom, but that's not really great, and
> we may have more smart pointer types in the future.
> 
> Am I missing something here?

Basically, you're asking "is it possible to use the same `work_struct`
field for several different pointer types"?

When creating the function pointer to store in the `work_struct`, the
function pointer _must_ know what the pointer type is. Otherwise it
cannot call the right `WorkItem::run` method or perform the correct
`container_of` operation. (E.g. an `Arc` embeds a `refcount_t` before
the struct, but a `Box` does not.)

With an associated type there is no problem with that. Associated types
force you to make a choice, which means that the `work_struct` knows
what the pointer type is when you create it. Supporting what you suggest
means that we must be able to change the function pointer stored in the
`work_struct` after initializing it.

This is rather tricky because you can call `enqueue` from several
threads in parallel; just setting the function pointer before you call
`queue_work_on` would be a data race. I suppose you could do it by
implementing our own `queue_work_on` that sets the function pointer
_after_ successfully setting the `WORK_STRUCT_PENDING_BIT`, but I don't
think this patchset should do that.

Alice