rust: hrtimer: introduce hrtimer support #1061

metaspace · 2024-02-09T13:54:43Z

This commit adds support for intrusive use of the hrtimer system. For now, only one timer can be embedded in a host struct.

The hrtimer Rust API is based on the intrusive style pattern introduced by the Rust workqueue API.

This commit adds support for intrusive use of the hrtimer system. For now, only one timer can be embedded in a host struct. The hrtimer Rust API is based on the intrusive style pattern introduced by the Rust workqueue API. Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>

rust/kernel/hrtimer.rs

Darksonn · 2024-02-09T13:58:15Z

rust/kernel/hrtimer.rs

+/// # Invariants
+///
+/// * `self.timer` is initialized by `bindings::hrtimer_init`.
+///


You probably need an invariant along the lines of "the function is T::Receiver::run" or "the function is compatible with T".

Why do I need this invariant?

Darksonn · 2024-02-09T13:59:56Z

rust/kernel/hrtimer.rs

+#[pinned_drop]
+impl<T> PinnedDrop for Timer<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY: By struct invariant `self.timer` was initialized by
+        // `hrtimer_init` so by C API contract it is safe to call
+        // `hrtimer_cancel`.
+        unsafe {
+            bindings::hrtimer_cancel(self.timer.get());
+        }
+    }
+}


In the workqueue, ownership makes it impossible to run the destructor while the item is registered with a workqueue. Is that not the same here?

I'm concerned about this because if this is not the first field (in declaration order! not memory layout order), then other fields may already have been dropped, so by the time we call this method, triggering the timer is already a UAF.

I think you are right. The timer handler will hold a reference to the struct that is embedding the Timer, and if the Timer is not the first field, some of the fields could be dropped while the handler is holding this reference.

I'll try to think of a solution, thanks.

If being the first field is a solution, impl_has_timer could become an attribute macro that also adds this field. That's not foolproof and adds proc macro complexity.

Maybe impl_has_timer could add a Drop impl for whatever type it wraps?

This kind of comes back to the container_of problem.

The reason for using Pin<&T> to point to the target is that I have the
following arrangement:

In blk-mq, requests are handed off to the driver as a pointer to a struct request. The first part of the pointee is the struct request and immediately
after that is a driver private area that the driver can use to store things:

+-----------------------+ | | | | | Request | | | | | +-----------------------+ | | | Private | | | | | +-----------------------+

The memory for these is pre allocated and initialized before the driver starts
accepting requests. The null block driver keeps the following in the private
area:

#[pin_data] struct Pdu { #[pin] timer: kernel::hrtimer::Timer<Self>, }

When the driver is handling a request, it will obtain a reference to this place:
Pin<&Pdu> that is guaranteed to be valid until the request is completed.

There are three options I think:

Use an owned pointer such as Box, Arc, Aref.

Somehow implement drop on the container struct, for instance with a proc macro.

Use a custom pointer type for this special use case - not sure how yet.

I would rather not go with 1 for my use case, because that would mean an extra
layer of indirection. I would have to store an Arc in my private area of the
request structure. For other uses it might be fine and the hrtimer
abstractions would work as they are. Just don't implement the pointer traits for
references.

For 2, we would loose the ability to eventually embed more than one timer in a
structure. I would like to keep the option to extend this patch like workqueue
to use an ID to distinguish between fields. Also, it might interfere with user
drop impls?

3 would mean unsafe code in the driver, and I am not sure how to exactly do it
yet. But maybe it could be the way.

Any input greatly appreciated :)

If being the first field is a solution, impl_has_timer could become an attribute macro that also adds this field. That's not foolproof and adds proc macro complexity.

Maybe impl_has_timer could add a Drop impl for whatever type it wraps?

This kind of comes back to the container_of problem.

I think that it is not sufficient, if we want to support custom drop implementations, since one could have an invariant on the struct that is violated after drop is called, then when dropping even the first field, the invariant is violated.

Any input greatly appreciated :)

I think I have an idea, but it is rather complex and needs a lot more thought:

have a Deconstruct trait with fn deconstruct(&self) that is called at most once (done by making it unsafe) and guaranteed to be called before any drop functions from objects higher in the "contains" hierarchy are called

call bindings::hrtimer_cancel inside of Timer::deconstruct

somehow only allow constructions of Timer where the above invariant can be upheld

we might need a new smart pointer for this.

This only works if the request struct that you mentioned is declared on the rust side though.

This only works if the request struct that you mentioned is declared on the rust side though.

It's a C side structure that is initialized and destroyed from C. In this case, the private part is initialized and destroyed by calling into a vtable where Rust pointers are installed.

The following is not directly related to this PR, but it clarifies my rationale for wanting to implement the timer trait on references. I think this whole situation actually comes from me trying to achieve something that is not possible. struct request is actually reference counted. But when the requests are issued to the drivers, the block layer holds a reference to the request, and thus they are guaranteed to not disappear until they are completed. So for performance reasons C drivers do not take out a ref count on the structure. They just promise to only access requests that are in flight (not completed). To do this they must trust drivers to never deliver invalid completions, because at some point in the process a driver will have to turn a completion id into a request reference. If the hardware delivers an invalid completion id, you would get a reference to the wrong request and possibly have a data race.

In Rust this is a problem because we have to provide a function that turns an integer into a request reference (for getting to the request in interrupt context when hardware reports the operation done). If you provide an invalid integer (tag) here, you would get a reference that violate invariants for shared references. In Rust it is not enough to say "we trust the driver to deliver correct completion ids", because it is still possible to write safe code that has UB by simply asking for a reference for a tag that you should not be able to get.

So, I either have to make that particular function (turning an id into a request reference) unsafe, and each driver must suffer a line of unsafe code to call this, or I would have to take a reference on the request to make sure it cannot go away. The latter has the side effect of making this timer issue disappear as well, but it might hurt performance.

In the interest of making this patch less convoluted, I think I will benchmark the cost of taking out a ref count on the request and see if that works out. Then we can skip this dance and implement the timer pointer trait for a reference counted smart pointer, probably ARef.

I solved this by using an owned reference for my use case.

rust/kernel/hrtimer.rs

tgross35 · 2024-02-09T18:50:01Z

rust/kernel/hrtimer.rs

+/// Implemented by pointer types to structs that embed a [`Timer`] to allow
+/// queueing the timer through the pointer.


Could you clarify which pointer this is?

tgross35 · 2024-02-09T18:55:25Z

rust/kernel/hrtimer.rs

+    /// Returns offset of the [`Timer`] struct within `Self`.
+    ///
+    /// Required because [`OFFSET`] cannot be accessed when `Self` is `!Sized`.
+    fn get_timer_offset(&self) -> usize {


timer_offset (without the get) might be more in line with naming

I removed this function because it is not necessary

rust/kernel/hrtimer.rs

tgross35 · 2024-02-09T19:00:31Z

rust/kernel/hrtimer.rs

+/// Derive the [`HasTimer`] trait for a struct that contains a field of type
+/// [`Timer`]. See the module level documentation for an example.
+#[macro_export]
+macro_rules! impl_has_timer {


A small example here would be good, or link to the module docs for more information (since the macro will show up in the top-level docs)

tgross35 · 2024-02-09T19:03:41Z

rust/kernel/hrtimer.rs

+//!    use kernel::{hrtimer::{RawTimer, Timer, TimerCallback}, impl_has_timer, pr_info, prelude::*, stack_pin_init};
+//!    use core::sync::atomic::AtomicBool;
+//!    use core::sync::atomic::Ordering;


Rustfmt suggests:

Suggested change

//! use kernel::{hrtimer::{RawTimer, Timer, TimerCallback}, impl_has_timer, pr_info, prelude::*, stack_pin_init};

//! use core::sync::atomic::AtomicBool;

//! use core::sync::atomic::Ordering;

use core::sync::atomic::AtomicBool;

use core::sync::atomic::Ordering;

use kernel::{

hrtimer::{RawTimer, Timer, TimerCallback},

impl_has_timer, pr_info,

prelude::*,

stack_pin_init,

};

Weird that it doesn't group the two atomic imports. pr_info should be in the prelude

How did you get rustfmt to format the example for you? I tried with

rustfmt +nightly --config format_code_in_doc_comments=true rust/kernel/hrtimer.rs

but no success.

tgross35 · 2024-02-09T19:17:49Z

rust/kernel/hrtimer.rs

+#[pinned_drop]
+impl<T> PinnedDrop for Timer<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY: By struct invariant `self.timer` was initialized by
+        // `hrtimer_init` so by C API contract it is safe to call
+        // `hrtimer_cancel`.
+        unsafe {
+            bindings::hrtimer_cancel(self.timer.get());
+        }
+    }
+}


If being the first field is a solution, impl_has_timer could become an attribute macro that also adds this field. That's not foolproof and adds proc macro complexity.

Maybe impl_has_timer could add a Drop impl for whatever type it wraps?

This kind of comes back to the container_of problem.

Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>

Darksonn reviewed Feb 9, 2024

View reviewed changes

tgross35 reviewed Feb 9, 2024

View reviewed changes

hrtimer: address review comments

aeae371

Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>

metaspace force-pushed the rust-pci branch from 2e951c5 to 7b72ead Compare March 4, 2024 12:07

metaspace force-pushed the rust-pci branch from 7b72ead to 0bde2cc Compare March 14, 2024 13:55

metaspace force-pushed the staging/rust-pci branch from 0bde2cc to 75baacb Compare April 16, 2024 11:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rust: hrtimer: introduce hrtimer support #1061

rust: hrtimer: introduce hrtimer support #1061

metaspace commented Feb 9, 2024

Darksonn Feb 9, 2024

metaspace Feb 23, 2024

Darksonn Feb 9, 2024

metaspace Feb 9, 2024

tgross35 Feb 9, 2024

metaspace Feb 10, 2024

y86-dev Feb 10, 2024

y86-dev Feb 10, 2024

metaspace Feb 12, 2024

metaspace Feb 23, 2024

tgross35 Feb 9, 2024

tgross35 Feb 9, 2024

metaspace Feb 23, 2024

tgross35 Feb 9, 2024

tgross35 Feb 9, 2024

metaspace Feb 23, 2024

tgross35 Feb 9, 2024

		/// Implemented by pointer types to structs that embed a [`Timer`] to allow
		/// queueing the timer through the pointer.

rust: hrtimer: introduce hrtimer support #1061

Are you sure you want to change the base?

rust: hrtimer: introduce hrtimer support #1061

Conversation

metaspace commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment