linux-kernel - Re: [PATCH v8 02/14] rust: hrtimer: introduce hrtimer support

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]
Message-ID: <CAJ-ks9=P2W9+qk216N5HnTvW+yvfPfGbJf87cFjhv8SuayyW6w@mail.gmail.com>
Date: Fri, 21 Feb 2025 09:19:14 -0500
From: Tamir Duberstein <tamird@...il.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Anna-Maria Behnsen <anna-maria@...utronix.de>, 
	Frederic Weisbecker <frederic@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Danilo Krummrich <dakr@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Alice Ryhl <aliceryhl@...gle.com>, 
	Trevor Gross <tmgross@...ch.edu>, Lyude Paul <lyude@...hat.com>, Guangbo Cui <2407018371@...com>, 
	Dirk Behme <dirk.behme@...il.com>, Daniel Almeida <daniel.almeida@...labora.com>, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 02/14] rust: hrtimer: introduce hrtimer support

On Fri, Feb 21, 2025 at 8:17 AM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>
> "Tamir Duberstein" <tamird@...il.com> writes:
>
> > On Fri, Feb 21, 2025 at 3:20 AM Andreas Hindborg <a.hindborg@...nel.org> wrote:
> >>
> >> "Tamir Duberstein" <tamird@...il.com> writes:
> >>
> >> > On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@...nel.org> wrote:
> >>
> >> [...]
> >>
> >> >> >> +pub unsafe trait HrTimerHandle {
> >> >> >> +    /// Cancel the timer, if it is running. If the timer handler is running, block
> >> >> >> +    /// till the handler has finished.
> >> >> >> +    fn cancel(&mut self) -> bool;
> >> >> >> +}
> >> >> >> +
> >> >> >> +/// Implemented by structs that contain timer nodes.
> >> >> >> +///
> >> >> >> +/// Clients of the timer API would usually safely implement this trait by using
> >> >> >> +/// the [`crate::impl_has_hr_timer`] macro.
> >> >> >> +///
> >> >> >> +/// # Safety
> >> >> >> +///
> >> >> >> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
> >> >> >> +/// field at the offset specified by `OFFSET` and that all trait methods are
> >> >> >> +/// implemented according to their documentation.
> >> >> >> +///
> >> >> >> +/// [`impl_has_timer`]: crate::impl_has_timer
> >> >> >> +pub unsafe trait HasHrTimer<T> {
> >> >> >> +    /// Offset of the [`HrTimer`] field within `Self`
> >> >> >> +    const OFFSET: usize;
> >> >> >
> >> >> > Does this need to be part of the trait? As an alternative the provided
> >> >> > methods could be generated in the macro below and reduce the
> >> >> > opportunity to implement this trait incorrectly.
> >> >>
> >> >> There is no risk of implementing the trait wrong, because it is usually
> >> >> derived by a macro.
> >> >
> >> > There's no risk when it's implemented by the macro, but you used the
> >> > word usually,  which means there is a risk.
> >> >
> >> >> We need at least one of the methods to be able to have the type system
> >> >> verify that the type for which we implement `HasHrTImer` actually has a
> >> >> field with the name we specify, and that this field has the right type.
> >> >> And to have that, we need the OFFSET.
> >> >
> >> > I don't follow this logic. OFFSET is calculated in the body of the
> >> > macro. I'm suggesting that the macro generate the method
> >> > implementations (which would no longer be provided). In effect I'm
> >> > saying: keep OFFSET private.
> >> >
> >> > I'm also noticing now that the macro generates an implementation of
> >> > raw_get_timer *in addition to* the provided implementation. Why are
> >> > both needed?
> >>
> >> HasHrTimer is unsafe, because it would be unsound to implement, if the
> >> type it is implemented on does not have a `Timer` at the specified
> >> offset.
> >>
> >> To be able to implement it safely with a macro, the macro must verify
> >> that the type we implement the trait on satisfies the safety
> >> requirement. That is, we have to have the macro verify that the type
> >> indeed has a field of type `Timer` with the given name. If that is the
> >> case, the macro can calculate OFFSET.
> >>
> >> The way we achieve this is we re-implement on of the trait methods in
> >> such a way that it only compiles if the type we reimplement trait
> >> on actually have the field of the right type.
> >>
> >> I want to generate as little code as possible in the macro, and I would
> >> rather rely on the default implementations given in the trait, than have
> >> the macro generate implementations for all the methods. Generated code
> >> are more difficult to reason about.
> >
> > Again, I don't follow. The provided implementation of raw_get_timer is
> > either not used (in the presence of the macro) or it relies on the
> > implementer correctly setting OFFSET, which the compiler cannot check
> > and which can break at a distance.
> >
> > Wouldn't it be simpler to just generate both functions that rely on
> > OFFSET? They're both one-liners that delegate to other existing
> > macros.
>
> No, I would rather generate as little code as possible. The only reason
> I am generating `raw_get_timer` is to be able to type check that the
> field name given to the macro has the right type.

At a minimum this code is inconsistent because it includes a provided
implementation which is never used.

But I continue to disagree that generating OFFSET and then doing raw
pointer manipulation is better than just generating a one-line method.
Perhaps someone else could weigh in.