Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rust: hrtimer: introduce hrtimer support #1061

Draft
wants to merge 2 commits into
base: staging/rust-pci
Choose a base branch
from

Conversation

metaspace
Copy link
Collaborator

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 <[email protected]>
rust/kernel/hrtimer.rs Outdated Show resolved Hide resolved
Comment on lines 60 to 63
/// # Invariants
///
/// * `self.timer` is initialized by `bindings::hrtimer_init`.
///
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do I need this invariant?

Comment on lines +108 to +118
#[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());
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Use an owned pointer such as Box, Arc, Aref.
  2. Somehow implement drop on the container struct, for instance with a proc macro.
  3. 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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rust/kernel/hrtimer.rs Outdated Show resolved Hide resolved
rust/kernel/hrtimer.rs Outdated Show resolved Hide resolved
rust/kernel/hrtimer.rs Show resolved Hide resolved
Comment on lines 120 to 121
/// Implemented by pointer types to structs that embed a [`Timer`] to allow
/// queueing the timer through the pointer.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify which pointer this is?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this function because it is not necessary

rust/kernel/hrtimer.rs Outdated Show resolved Hide resolved
/// 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines 12 to 14
//! 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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +108 to +118
#[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());
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants