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?

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:

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 :)

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.

Could you clarify which pointer this is?

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

I removed this function because it is not necessary

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)

Rustfmt suggests:

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.

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.