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-ks9mfe3JBVALO6RJPie6Ae1afH7jsE5jR1jWo_sK266_XPQ@mail.gmail.com>
Date: Fri, 21 Feb 2025 08:04:40 -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 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.