Re: [PATCH RFC 08/11] rust: add support for dynamic debug

[sashiko-bot@kernel.org]

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 <linux@weissschuh.net>

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/20261011-rust-dyndbg-v1-0-832e4039b71f@weissschuh.net?part=8