Re: [PATCH v4] rust: configfs: add procedural macro for declaring configfs attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The new macro no longer prevents duplicate attribute names at compile time.
- [Low] The macro will panic at compile time if a raw identifier (e.g., `r#type`) is used as an attribute name.

Pre-existing issues:
- [High] The `configfs_attrs!` macro dynamically mutates a `static` array via `AttributeList::add`, causing a data race and undefined behavior when invoked concurrently at runtime.
--

commit eb5933487fd2a34ea26fb0f106c056e03f633059
Author: Malte Wechter <maltewechter@gmail.com>

rust: configfs: add procedural macro for declaring configfs attributes

Implement configfs_attrs! as a procedural macro using syn, this improves
readability and maintainability. Remove the old macro and replace all uses
with the new macro. Add the new macro implementation file to MAINTAINERS.

> diff --git a/rust/macros/configfs_attrs.rs b/rust/macros/configfs_attrs.rs
> new file mode 100644
> index 0000000000000..81037bc38188b
> --- /dev/null
> +++ b/rust/macros/configfs_attrs.rs

[ ... ]

> +    let mut attrs = Vec::new();
> +    for (attr_idx, (name, id)) in cfs_attrs.attributes.iter().enumerate() {
> +        let name_with_attr = format_ident!("{}_ATTR_{}", name.to_string().to_uppercase(), attr_idx);

[Severity: Medium]
Does this change remove compile-time duplicate name checking?

In the old macro, the generated static variables were named based only on
the data type and attribute name (e.g., CONFIGURATION_MESSAGE_ATTR). If a
user accidentally provided duplicate attribute names, the compiler would
catch the duplicate static variable definition.

By appending attr_idx to the identifier here, duplicate attributes will
compile successfully since the variable names are unique, but they will
fail at runtime when the C configfs layer attempts to register them.

[Severity: Low]
Will this format_ident! call panic during compilation if a raw identifier
(like r#type) is used as an attribute name?

When name is a raw identifier, name.to_string() yields "r#type", which
becomes "R#TYPE" after to_uppercase(). Attempting to construct an
identifier containing "R#" is invalid. Using name.unraw() might be needed
to strip the prefix before processing.

> +
> +        let id: u64 = match id.base10_parse::<u64>() {
> +            Ok(v) => v,
> +            Err(_) => {
> +                return syn::Error::new(id.span(), "Could not parse attribute ID as a u64")
> +                    .to_compile_error();
> +            }
> +        };
> +
> +        attrs.push(quote! {
> +        static #name_with_attr: kernel::configfs::Attribute<#id, #data_ty, #data_ty> =
> +            // SAFETY: We are expanding `configfs_attrs`.
> +            unsafe {
> +              kernel::configfs::Attribute::new(kernel::c_str!(::core::stringify!(#name)))
> +            };
> +
> +          // SAFETY: By design of this macro, the name of the variable we
> +          // invoke the `add` method on below, is not visible outside of
> +          // the macro expansion. The macro does not operate concurrently
> +          // on this variable, and thus we have exclusive access to the
> +          // variable.
> +          unsafe { #data_attr_ident.add::<#attr_idx, #id, _>(&#name_with_attr) }

[Severity: High]
This is a pre-existing issue, but does this unsafe block introduce a data
race?

The macro expands to a block containing a static DATA_ATTR_LIST and unsafe
runtime statements (DATA_ATTR_LIST.add(...)) to populate it. Because this
macro is typically evaluated at runtime inside the make_group callback
(which handles mkdir syscalls), concurrent executions by multiple threads
will perform unsynchronized writes to the shared static variable.

Concurrently modifying the same memory via interior mutability is undefined
behavior in Rust.

> +        });
> +    }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619-configfs-syn-v4-1-207c39504c1e@gmail.com?part=1