Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] printk: dynamic debug formatting prefixes are silently ignored in Rust - [Low] rust: data race due to mixed atomic/non-atomic access on C bitfield flags --
commit 62390fa941837d8feaa389e3a6ea73a463056133 Author: Thomas Weißschuh <[email protected]> rust: add support for dynamic debug This commit adds the Rust infrastructure to integrate with the dynamic debug framework. It introduces a descriptor wrapper and macros to wire up the conditional debug calls, with static keys and custom classes planned for the future. > diff --git a/rust/kernel/dynamic_debug.rs b/rust/kernel/dynamic_debug.rs > new file mode 100644 > index 0000000000000..2fdc691eaac94 > --- /dev/null > +++ b/rust/kernel/dynamic_debug.rs [ ... ] > + #[inline(always)] > + pub unsafe fn branch_and_stack<F: FnOnce()>(desc: *mut > Descriptor<'static>, func: F) { > + // TODO: Use static keys. > + > + let flags_ptr = project!(mut desc, .0.flags); > + // SAFETY: As per the function safety requirement desc is valid, and > properly aligned, > + // so flags_ptr is so, too. Updates from C are atomic. > + let flags = unsafe { atomic::atomic_load(flags_ptr, > atomic::ordering::Relaxed) }; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: Low] Does this atomic load in Rust race with non-atomic bitfield updates in C? In the C core, the flags field is defined as an unsigned int bitfield sharing a 32-bit memory location with lineno and class_id: include/linux/dynamic_debug.h:struct _ddebug { ... unsigned int flags:8; ... } The C code modifies flags using non-atomic, unprotected read-modify-write operations under a mutex: lib/dynamic_debug.c:ddebug_change() { ... dp->flags = newflags; ... } Mixing atomic loads in Rust with non-atomic bitfield updates in C results in a data race. While practically benign as word tearing on 32-bit aligned words is rare, the assumption in the comments that C updates are atomic or that flags is an independent byte appears factually incorrect. > + > + if flags & flags::PRINT != 0 { > + func(); ^^^^^^ [Severity: Medium] Does this parameterless closure execution prevent formatting prefixes? In C, dynamic debug formats optional prefixes such as module name, function name, line number, or thread ID via dynamic_emit_prefix() using the _ddebug descriptor. Because func() does not receive the descriptor, the underlying closure will invoke printk directly without retrieving the formatted prefix. As a result, all dynamic debug formatting flags (+p, +m, +f, +l, +t) configured via debugfs appear to be silently ignored for Rust logs, degrading expected kernel observability features. [ ... ] > +macro_rules! dynamic_func_call_cls { > + ($cls:expr, $fmt:expr, $func:expr) => {{ > + const default_enabled: bool = cfg!(debug_assertions); > + > + let descriptor = > $crate::dynamic_debug_metadata_cls!(default_enabled, $cls, $fmt); > + > + // SAFETY: descriptor has static lifetime, so it is always valid. > + unsafe { > + $crate::dynamic_debug::Descriptor::branch_and_stack(descriptor, > $func); ^^^^^ [Severity: Medium] Will this macro need to pass the descriptor into the func closure so that the underlying logging macro can use it for dynamic prefix formatting? > + } > + }}; > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
