Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: staging/rust-pci
Are you sure you want to change the base?
rust: hrtimer: introduce hrtimer support #1061
Changes from 1 commit
ec475a6
aeae371
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rustfmt suggests:
Weird that it doesn't group the two
atomic
imports.pr_info
should be in the preludeThere was a problem hiding this comment.
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 withbut no success.
There was a problem hiding this comment.
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 withT
".There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 theTimer
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.
There was a problem hiding this comment.
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 aDrop
impl for whatever type it wraps?This kind of comes back to the
container_of
problem.There was a problem hiding this comment.
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 thefollowing 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 thestruct request
and immediatelyafter that is a driver private area that the driver can use to store things:
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:
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:
Box
,Arc
,Aref
.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 therequest 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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have an idea, but it is rather complex and needs a lot more thought:
Deconstruct
trait withfn deconstruct(&self)
that is called at most once (done by making itunsafe
) and guaranteed to be called before anydrop
functions from objects higher in the "contains" hierarchy are calledbindings::hrtimer_cancel
inside ofTimer::deconstruct
Timer
where the above invariant can be upheldwe 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timer_offset
(without theget
) might be more in line with namingThere was a problem hiding this comment.
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
There was a problem hiding this comment.
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)