linux-kernel - Re: [PATCH v8 3/5] rust: samples: add tracepoint to Rust sample

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: <ZsdzNqUuXSXaGZqt@boqun-archlinux>
Date: Thu, 22 Aug 2024 10:19:50 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Josh Poimboeuf <jpoimboe@...nel.org>,	Jason Baron <jbaron@...mai.com>,
 Ard Biesheuvel <ardb@...nel.org>,	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...sung.com>,
	linux-trace-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	linux-arch@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>,
	Sean Christopherson <seanjc@...gle.com>,	Uros Bizjak <ubizjak@...il.com>,
	Catalin Marinas <catalin.marinas@....com>,	Will Deacon <will@...nel.org>,
 Marc Zyngier <maz@...nel.org>,	Oliver Upton <oliver.upton@...ux.dev>,
	Mark Rutland <mark.rutland@....com>,	Ryan Roberts <ryan.roberts@....com>,
 Fuad Tabba <tabba@...gle.com>,	linux-arm-kernel@...ts.infradead.org,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,	Albert Ou <aou@...s.berkeley.edu>,
	Anup Patel <apatel@...tanamicro.com>,
	Andrew Jones <ajones@...tanamicro.com>,
	Alexandre Ghiti <alexghiti@...osinc.com>,
	Conor Dooley <conor.dooley@...rochip.com>,
	Samuel Holland <samuel.holland@...ive.com>,
	linux-riscv@...ts.infradead.org,	Huacai Chen <chenhuacai@...nel.org>,
	WANG Xuerui <kernel@...0n.name>, Bibo Mao <maobibo@...ngson.cn>,
	Tiezhu Yang <yangtiezhu@...ngson.cn>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Tianrui Zhao <zhaotianrui@...ngson.cn>, loongarch@...ts.linux.dev
Subject: Re: [PATCH v8 3/5] rust: samples: add tracepoint to Rust sample

On Thu, Aug 22, 2024 at 12:04:15PM +0000, Alice Ryhl wrote:
> This updates the Rust printing sample to invoke a tracepoint. This
> ensures that we have a user in-tree from the get-go even though the
> patch is being merged before its real user.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
>  MAINTAINERS                        |  1 +
>  include/trace/events/rust_sample.h | 31 +++++++++++++++++++++++++++++++
>  rust/bindings/bindings_helper.h    |  1 +
>  samples/rust/Makefile              |  3 ++-
>  samples/rust/rust_print.rs         | 18 ++++++++++++++++++
>  samples/rust/rust_print_events.c   |  8 ++++++++
>  6 files changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f328373463b0..1acf5bfddfc4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19922,6 +19922,7 @@ C:	zulip://rust-for-linux.zulipchat.com
>  P:	https://rust-for-linux.com/contributing
>  T:	git https://github.com/Rust-for-Linux/linux.git rust-next
>  F:	Documentation/rust/
> +F:	include/trace/events/rust_sample.h
>  F:	rust/
>  F:	samples/rust/
>  F:	scripts/*rust*
> diff --git a/include/trace/events/rust_sample.h b/include/trace/events/rust_sample.h
> new file mode 100644
> index 000000000000..dbc80ca2e465
> --- /dev/null
> +++ b/include/trace/events/rust_sample.h

Is it possible to make this a header file inside sample/rust/? Given
this is just an example, I feel it's better if we could avoid making
this "public", but maybe I'm missing some constraints of tracepoints.

(Oh, I just remember the problem while I was writting this: we need the
header file here because this is now how bindgen generates bindings, so
moving it to sample/rust/ requires we have "per-module" or
"per-subsystem" bindgen feature)

Anyway this is not a big deal to me. We can move it later if possible.
So:

Reviewed-by: Boqun Feng <boqun.feng@...il.com>

Regards,
Boqun

> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Tracepoints for `samples/rust/rust_print.rs`.
> + *
> + * Copyright (C) 2024 Google, Inc.
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM rust_sample
> +
> +#if !defined(_RUST_SAMPLE_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _RUST_SAMPLE_TRACE_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(rust_sample_loaded,
> +	TP_PROTO(int magic_number),
> +	TP_ARGS(magic_number),
> +	TP_STRUCT__entry(
> +		__field(int, magic_number)
> +	),
> +	TP_fast_assign(
> +		__entry->magic_number = magic_number;
> +	),
> +	TP_printk("magic=%d", __entry->magic_number)
> +);
> +
> +#endif /* _RUST_SAMPLE_TRACE_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index fc6f94729789..fe97256afe65 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -23,6 +23,7 @@
>  #include <linux/tracepoint.h>
>  #include <linux/wait.h>
>  #include <linux/workqueue.h>
> +#include <trace/events/rust_sample.h>
>  
>  /* `bindgen` gets confused at certain things. */
>  const size_t RUST_CONST_HELPER_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN;
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index 03086dabbea4..f29280ec4820 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
> +ccflags-y += -I$(src)				# needed for trace events
>  
>  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> -obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
> +obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o rust_print_events.o
>  
>  subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS)		+= hostprogs
> diff --git a/samples/rust/rust_print.rs b/samples/rust/rust_print.rs
> index 6eabb0d79ea3..6d14b08cac1c 100644
> --- a/samples/rust/rust_print.rs
> +++ b/samples/rust/rust_print.rs
> @@ -69,6 +69,8 @@ fn init(_module: &'static ThisModule) -> Result<Self> {
>  
>          arc_print()?;
>  
> +        trace::trace_rust_sample_loaded(42);
> +
>          Ok(RustPrint)
>      }
>  }
> @@ -78,3 +80,19 @@ fn drop(&mut self) {
>          pr_info!("Rust printing macros sample (exit)\n");
>      }
>  }
> +
> +mod trace {
> +    use core::ffi::c_int;
> +
> +    kernel::declare_trace! {
> +        /// # Safety
> +        ///
> +        /// Always safe to call.
> +        unsafe fn rust_sample_loaded(magic: c_int);
> +    }
> +
> +    pub(crate) fn trace_rust_sample_loaded(magic: i32) {
> +        // SAFETY: Always safe to call.
> +        unsafe { rust_sample_loaded(magic as c_int) }
> +    }
> +}
> diff --git a/samples/rust/rust_print_events.c b/samples/rust/rust_print_events.c
> new file mode 100644
> index 000000000000..a9169ff0edf1
> --- /dev/null
> +++ b/samples/rust/rust_print_events.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Google LLC
> + */
> +
> +#define CREATE_TRACE_POINTS
> +#define CREATE_RUST_TRACE_POINTS
> +#include <trace/events/rust_sample.h>
> 
> -- 
> 2.46.0.184.g6999bdac58-goog
>