| 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
| ||
|
Message-ID: <CAJ-ks9=2E0XV03ONKaeSnjq5fD=WC3u=nDnkmYK_spv59zVpjw@mail.gmail.com>
Date: Wed, 7 Jan 2026 12:11:32 -0500
From: Tamir Duberstein <tamird@...il.com>
To: Gary Guo <gary@...yguo.net>
Cc: Miguel Ojeda <ojeda@...nel.org>, Boqun Feng <boqun.feng@...il.com>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
Danilo Krummrich <dakr@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, Petr Pavlu <petr.pavlu@...e.com>,
Daniel Gomez <da.gomez@...nel.org>, Sami Tolvanen <samitolvanen@...gle.com>,
Aaron Tomlin <atomlin@...mlin.com>, José Expósito <jose.exposito89@...il.com>,
rust-for-linux@...r.kernel.org, Patrick Miller <paddymills@...ton.me>,
linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org
Subject: Re: [PATCH v2 04/11] rust: macros: use `syn` to parse `module!` macro
On Wed, Jan 7, 2026 at 11:30 AM Gary Guo <gary@...nel.org> wrote:
>
> From: Gary Guo <gary@...yguo.net>
>
> With `syn` being available in the kernel, use it to parse the complex
> custom `module!` macro to replace existing helpers. Only parsing is
> changed in this commit, the code generation is untouched.
>
> This has the benefit of better error message when the macro is used
> incorrectly, as it can point to a concrete span on what's going wrong.
>
> For example, if a field is specified twice, previously it reads:
>
> error: proc macro panicked
> --> samples/rust/rust_minimal.rs:7:1
> |
> 7 | / module! {
> 8 | | type: RustMinimal,
> 9 | | name: "rust_minimal",
> 10 | | author: "Rust for Linux Contributors",
> 11 | | description: "Rust minimal sample",
> 12 | | license: "GPL",
> 13 | | license: "GPL",
> 14 | | }
> | |_^
> |
> = help: message: Duplicated key "license". Keys can only be specified once.
>
> now it reads:
>
> error: duplicated key "license". Keys can only be specified once.
> --> samples/rust/rust_minimal.rs:13:5
> |
> 13 | license: "GPL",
> | ^^^^^^^
>
> Signed-off-by: Gary Guo <gary@...yguo.net>
Reviewed-by: Tamir Duberstein <tamird@...il.com>
> ---
> rust/macros/helpers.rs | 109 ++++--------
> rust/macros/lib.rs | 6 +-
> rust/macros/module.rs | 389 +++++++++++++++++++++++++----------------
> 3 files changed, 277 insertions(+), 227 deletions(-)
>
> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
> index 13fafaba12261..fa66ef6eb0f3d 100644
> --- a/rust/macros/helpers.rs
> +++ b/rust/macros/helpers.rs
> @@ -1,53 +1,21 @@
> // SPDX-License-Identifier: GPL-2.0
>
> -use proc_macro2::{token_stream, Group, Ident, TokenStream, TokenTree};
> -
> -pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
> - if let Some(TokenTree::Ident(ident)) = it.next() {
> - Some(ident.to_string())
> - } else {
> - None
> - }
> -}
> -
> -pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
> - let peek = it.clone().next();
> - match peek {
> - Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
> - let _ = it.next();
> - Some(punct.as_char())
> - }
> - _ => None,
> - }
> -}
> -
> -pub(crate) fn try_literal(it: &mut token_stream::IntoIter) -> Option<String> {
> - if let Some(TokenTree::Literal(literal)) = it.next() {
> - Some(literal.to_string())
> - } else {
> - None
> - }
> -}
> -
> -pub(crate) fn try_string(it: &mut token_stream::IntoIter) -> Option<String> {
> - try_literal(it).and_then(|string| {
> - if string.starts_with('\"') && string.ends_with('\"') {
> - let content = &string[1..string.len() - 1];
> - if content.contains('\\') {
> - panic!("Escape sequences in string literals not yet handled");
> - }
> - Some(content.to_string())
> - } else if string.starts_with("r\"") {
> - panic!("Raw string literals are not yet handled");
> - } else {
> - None
> - }
> - })
> -}
> -
> -pub(crate) fn expect_ident(it: &mut token_stream::IntoIter) -> String {
> - try_ident(it).expect("Expected Ident")
> -}
> +use proc_macro2::{
> + token_stream,
> + Ident,
> + TokenStream,
> + TokenTree, //
> +};
> +use quote::ToTokens;
> +use syn::{
> + parse::{
> + Parse,
> + ParseStream, //
> + },
> + Error,
> + LitStr,
> + Result, //
> +};
>
> pub(crate) fn expect_punct(it: &mut token_stream::IntoIter) -> char {
> if let TokenTree::Punct(punct) = it.next().expect("Reached end of token stream for Punct") {
> @@ -57,27 +25,28 @@ pub(crate) fn expect_punct(it: &mut token_stream::IntoIter) -> char {
> }
> }
>
> -pub(crate) fn expect_string(it: &mut token_stream::IntoIter) -> String {
> - try_string(it).expect("Expected string")
> -}
> +/// A string literal that is required to have ASCII value only.
> +pub(crate) struct AsciiLitStr(LitStr);
>
> -pub(crate) fn expect_string_ascii(it: &mut token_stream::IntoIter) -> String {
> - let string = try_string(it).expect("Expected string");
> - assert!(string.is_ascii(), "Expected ASCII string");
> - string
> +impl Parse for AsciiLitStr {
> + fn parse(input: ParseStream<'_>) -> Result<Self> {
> + let s: LitStr = input.parse()?;
> + if !s.value().is_ascii() {
> + return Err(Error::new_spanned(s, "expected ASCII-only string literal"));
> + }
> + Ok(Self(s))
> + }
> }
>
> -pub(crate) fn expect_group(it: &mut token_stream::IntoIter) -> Group {
> - if let TokenTree::Group(group) = it.next().expect("Reached end of token stream for Group") {
> - group
> - } else {
> - panic!("Expected Group");
> +impl ToTokens for AsciiLitStr {
> + fn to_tokens(&self, ts: &mut TokenStream) {
> + self.0.to_tokens(ts);
> }
> }
>
> -pub(crate) fn expect_end(it: &mut token_stream::IntoIter) {
> - if it.next().is_some() {
> - panic!("Expected end");
> +impl AsciiLitStr {
> + pub(crate) fn value(&self) -> String {
> + self.0.value()
> }
> }
>
> @@ -114,17 +83,3 @@ pub(crate) fn file() -> String {
> proc_macro::Span::call_site().file()
> }
> }
> -
> -/// Parse a token stream of the form `expected_name: "value",` and return the
> -/// string in the position of "value".
> -///
> -/// # Panics
> -///
> -/// - On parse error.
> -pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String {
> - assert_eq!(expect_ident(it), expected_name);
> - assert_eq!(expect_punct(it), ':');
> - let string = expect_string(it);
> - assert_eq!(expect_punct(it), ',');
> - string
> -}
> diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
> index 9955c04dbaae3..c5347127a3a51 100644
> --- a/rust/macros/lib.rs
> +++ b/rust/macros/lib.rs
> @@ -131,8 +131,10 @@
> /// - `firmware`: array of ASCII string literals of the firmware files of
> /// the kernel module.
> #[proc_macro]
> -pub fn module(ts: TokenStream) -> TokenStream {
> - module::module(ts.into()).into()
> +pub fn module(input: TokenStream) -> TokenStream {
> + module::module(parse_macro_input!(input))
> + .unwrap_or_else(|e| e.into_compile_error())
> + .into()
> }
>
> /// Declares or implements a vtable trait.
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index b855a2b586e18..6ad7b411ccde4 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -2,28 +2,30 @@
>
> use std::fmt::Write;
>
> -use proc_macro2::{token_stream, Delimiter, Literal, TokenStream, TokenTree};
> +use proc_macro2::{
> + Literal,
> + TokenStream, //
> +};
> +use quote::ToTokens;
> +use syn::{
> + braced,
> + bracketed,
> + ext::IdentExt,
> + parse::{
> + Parse,
> + ParseStream, //
> + },
> + punctuated::Punctuated,
> + Error,
> + Expr,
> + Ident,
> + LitStr,
> + Result,
> + Token, //
> +};
>
> use crate::helpers::*;
>
> -fn expect_string_array(it: &mut token_stream::IntoIter) -> Vec<String> {
> - let group = expect_group(it);
> - assert_eq!(group.delimiter(), Delimiter::Bracket);
> - let mut values = Vec::new();
> - let mut it = group.stream().into_iter();
> -
> - while let Some(val) = try_string(&mut it) {
> - assert!(val.is_ascii(), "Expected ASCII string");
> - values.push(val);
> - match it.next() {
> - Some(TokenTree::Punct(punct)) => assert_eq!(punct.as_char(), ','),
> - None => break,
> - _ => panic!("Expected ',' or end of array"),
> - }
> - }
> - values
> -}
> -
> struct ModInfoBuilder<'a> {
> module: &'a str,
> counter: usize,
> @@ -113,12 +115,16 @@ fn emit_params(&mut self, info: &ModuleInfo) {
> };
>
> for param in params {
> - let ops = param_ops_path(¶m.ptype);
> + let param_name = param.name.to_string();
> + let param_type = param.ptype.to_string();
> + let param_default = param.default.to_token_stream().to_string();
> +
> + let ops = param_ops_path(¶m_type);
>
> // Note: The spelling of these fields is dictated by the user space
> // tool `modinfo`.
> - self.emit_param("parmtype", ¶m.name, ¶m.ptype);
> - self.emit_param("parm", ¶m.name, ¶m.description);
> + self.emit_param("parmtype", ¶m_name, ¶m_type);
> + self.emit_param("parm", ¶m_name, ¶m.description.value());
>
> write!(
> self.param_buffer,
> @@ -160,10 +166,7 @@ fn emit_params(&mut self, info: &ModuleInfo) {
> );
> }};
> ",
> - module_name = info.name,
> - param_type = param.ptype,
> - param_default = param.default,
> - param_name = param.name,
> + module_name = info.name.value(),
> ops = ops,
> )
> .unwrap();
> @@ -187,127 +190,82 @@ fn param_ops_path(param_type: &str) -> &'static str {
> }
> }
>
> -fn expect_param_default(param_it: &mut token_stream::IntoIter) -> String {
> - assert_eq!(expect_ident(param_it), "default");
> - assert_eq!(expect_punct(param_it), ':');
> - let sign = try_sign(param_it);
> - let default = try_literal(param_it).expect("Expected default param value");
> - assert_eq!(expect_punct(param_it), ',');
> - let mut value = sign.map(String::from).unwrap_or_default();
> - value.push_str(&default);
> - value
> -}
> -
> -#[derive(Debug, Default)]
> -struct ModuleInfo {
> - type_: String,
> - license: String,
> - name: String,
> - authors: Option<Vec<String>>,
> - description: Option<String>,
> - alias: Option<Vec<String>>,
> - firmware: Option<Vec<String>>,
> - imports_ns: Option<Vec<String>>,
> - params: Option<Vec<Parameter>>,
> -}
> -
> -#[derive(Debug)]
> -struct Parameter {
> - name: String,
> - ptype: String,
> - default: String,
> - description: String,
> -}
> -
> -fn expect_params(it: &mut token_stream::IntoIter) -> Vec<Parameter> {
> - let params = expect_group(it);
> - assert_eq!(params.delimiter(), Delimiter::Brace);
> - let mut it = params.stream().into_iter();
> - let mut parsed = Vec::new();
> -
> - loop {
> - let param_name = match it.next() {
> - Some(TokenTree::Ident(ident)) => ident.to_string(),
> - Some(_) => panic!("Expected Ident or end"),
> - None => break,
> - };
> -
> - assert_eq!(expect_punct(&mut it), ':');
> - let param_type = expect_ident(&mut it);
> - let group = expect_group(&mut it);
> - assert_eq!(group.delimiter(), Delimiter::Brace);
> - assert_eq!(expect_punct(&mut it), ',');
> -
> - let mut param_it = group.stream().into_iter();
> - let param_default = expect_param_default(&mut param_it);
> - let param_description = expect_string_field(&mut param_it, "description");
> - expect_end(&mut param_it);
> -
> - parsed.push(Parameter {
> - name: param_name,
> - ptype: param_type,
> - default: param_default,
> - description: param_description,
> - })
> - }
> -
> - parsed
> -}
> -
> -impl ModuleInfo {
> - fn parse(it: &mut token_stream::IntoIter) -> Self {
> - let mut info = ModuleInfo::default();
> -
> - const EXPECTED_KEYS: &[&str] = &[
> - "type",
> - "name",
> - "authors",
> - "description",
> - "license",
> - "alias",
> - "firmware",
> - "imports_ns",
> - "params",
> - ];
> - const REQUIRED_KEYS: &[&str] = &["type", "name", "license"];
> +/// Parse fields that are required to use a specific order.
> +///
> +/// As fields must follow a specific order, we *could* just parse fields one by one by peeking.
> +/// However the error message generated when implementing that way is not very friendly.
> +///
> +/// So instead we parse fields in an arbitrary order, but only enforce the ordering after parsing,
> +/// and if the wrong order is used, the proper order is communicated to the user with error message.
> +///
> +/// Usage looks like this:
> +/// ```ignore
> +/// parse_ordered_fields! {
> +/// from input;
> +///
> +/// // This will extract "foo: <field>" into a variable named "foo".
> +/// // The variable will have type `Option<_>`.
> +/// foo => <expression that parses the field>,
> +///
> +/// // If you need the variable name to be different than the key name.
> +/// // This extracts "baz: <field>" into a variable named "bar".
> +/// // You might want this if "baz" is a keyword.
> +/// baz as bar => <expression that parse the field>,
> +///
> +/// // You can mark a key as required, and the variable will no longer be `Option`.
> +/// // foobar will be of type `Expr` instead of `Option<Expr>`.
> +/// foobar [required] => input.parse::<Expr>()?,
> +/// }
> +/// ```
It's a shame that this macro bails on the first failure rather than
accumulating them all.
> +macro_rules! parse_ordered_fields {
> + (@gen
> + [$input:expr]
> + [$([$name:ident; $key:ident; $parser:expr])*]
> + [$([$req_name:ident; $req_key:ident])*]
> + ) => {
> + $(let mut $name = None;)*
> +
> + const EXPECTED_KEYS: &[&str] = &[$(stringify!($key),)*];
> + const REQUIRED_KEYS: &[&str] = &[$(stringify!($req_key),)*];
> +
> + let span = $input.span();
> let mut seen_keys = Vec::new();
>
> loop {
> - let key = match it.next() {
> - Some(TokenTree::Ident(ident)) => ident.to_string(),
> - Some(_) => panic!("Expected Ident or end"),
> - None => break,
> - };
> + if $input.is_empty() {
> + break;
> + }
`while !$input.is_empty()`?
> +
> + let key = $input.call(Ident::parse_any)?;
>
> if seen_keys.contains(&key) {
> - panic!("Duplicated key \"{key}\". Keys can only be specified once.");
> + Err(Error::new_spanned(
> + &key,
> + format!(r#"duplicated key "{key}". Keys can only be specified once."#),
> + ))?
> }
>
> - assert_eq!(expect_punct(it), ':');
> -
> - match key.as_str() {
> - "type" => info.type_ = expect_ident(it),
> - "name" => info.name = expect_string_ascii(it),
> - "authors" => info.authors = Some(expect_string_array(it)),
> - "description" => info.description = Some(expect_string(it)),
> - "license" => info.license = expect_string_ascii(it),
> - "alias" => info.alias = Some(expect_string_array(it)),
> - "firmware" => info.firmware = Some(expect_string_array(it)),
> - "imports_ns" => info.imports_ns = Some(expect_string_array(it)),
> - "params" => info.params = Some(expect_params(it)),
> - _ => panic!("Unknown key \"{key}\". Valid keys are: {EXPECTED_KEYS:?}."),
> + $input.parse::<Token![:]>()?;
> +
> + match &*key.to_string() {
> + $(
> + stringify!($key) => $name = Some($parser),
> + )*
> + _ => {
> + Err(Error::new_spanned(
> + &key,
> + format!(r#"unknown key "{key}". Valid keys are: {EXPECTED_KEYS:?}."#),
> + ))?
> + }
> }
>
> - assert_eq!(expect_punct(it), ',');
> -
> + $input.parse::<Token![,]>()?;
> seen_keys.push(key);
> }
>
> - expect_end(it);
> -
> for key in REQUIRED_KEYS {
> if !seen_keys.iter().any(|e| e == key) {
> - panic!("Missing required key \"{key}\".");
> + Err(Error::new(span, format!(r#"missing required key "{key}""#)))?
> }
> }
>
> @@ -319,43 +277,178 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
> }
>
> if seen_keys != ordered_keys {
> - panic!("Keys are not ordered as expected. Order them like: {ordered_keys:?}.");
> + Err(Error::new(
> + span,
> + format!(r#"keys are not ordered as expected. Order them like: {ordered_keys:?}."#),
> + ))?
> + }
> +
> + $(let $req_name = $req_name.expect("required field");)*
> + };
> +
> + // Handle required fields.
> + (@gen
> + [$input:expr] [$($tok:tt)*] [$($req:tt)*]
> + $key:ident as $name:ident [required] => $parser:expr,
> + $($rest:tt)*
> + ) => {
> + parse_ordered_fields!(
> + @gen [$input] [$($tok)* [$name; $key; $parser]] [$($req)* [$name; $key]] $($rest)*
> + )
> + };
> + (@gen
> + [$input:expr] [$($tok:tt)*] [$($req:tt)*]
> + $name:ident [required] => $parser:expr,
> + $($rest:tt)*
> + ) => {
> + parse_ordered_fields!(
> + @gen [$input] [$($tok)* [$name; $name; $parser]] [$($req)* [$name; $name]] $($rest)*
> + )
> + };
> +
> + // Handle optional fields.
> + (@gen
> + [$input:expr] [$($tok:tt)*] [$($req:tt)*]
> + $key:ident as $name:ident => $parser:expr,
> + $($rest:tt)*
> + ) => {
> + parse_ordered_fields!(
> + @gen [$input] [$($tok)* [$name; $key; $parser]] [$($req)*] $($rest)*
> + )
> + };
> + (@gen
> + [$input:expr] [$($tok:tt)*] [$($req:tt)*]
> + $name:ident => $parser:expr,
> + $($rest:tt)*
> + ) => {
> + parse_ordered_fields!(
> + @gen [$input] [$($tok)* [$name; $name; $parser]] [$($req)*] $($rest)*
> + )
> + };
It would be nice to avoid the combinatorial explosion here, though I'm
not immediately sure how.
> +
> + (from $input:expr; $($tok:tt)*) => {
> + parse_ordered_fields!(@gen [$input] [] [] $($tok)*)
> + }
> +}
> +
> +struct Parameter {
> + name: Ident,
> + ptype: Ident,
> + default: Expr,
> + description: LitStr,
> +}
> +
> +impl Parse for Parameter {
> + fn parse(input: ParseStream<'_>) -> Result<Self> {
> + let name = input.parse()?;
> + input.parse::<Token![:]>()?;
> + let ptype = input.parse()?;
> +
> + let fields;
> + braced!(fields in input);
> +
> + parse_ordered_fields! {
> + from fields;
> + default [required] => fields.parse()?,
> + description [required] => fields.parse()?,
> }
>
> - info
> + Ok(Self {
> + name,
> + ptype,
> + default,
> + description,
> + })
> }
> }
>
> -pub(crate) fn module(ts: TokenStream) -> TokenStream {
> - let mut it = ts.into_iter();
> +pub(crate) struct ModuleInfo {
> + type_: Ident,
> + license: AsciiLitStr,
> + name: AsciiLitStr,
> + authors: Option<Punctuated<AsciiLitStr, Token![,]>>,
> + description: Option<LitStr>,
> + alias: Option<Punctuated<AsciiLitStr, Token![,]>>,
> + firmware: Option<Punctuated<AsciiLitStr, Token![,]>>,
> + imports_ns: Option<Punctuated<AsciiLitStr, Token![,]>>,
> + params: Option<Punctuated<Parameter, Token![,]>>,
> +}
>
> - let info = ModuleInfo::parse(&mut it);
> +impl Parse for ModuleInfo {
> + fn parse(input: ParseStream<'_>) -> Result<Self> {
> + parse_ordered_fields!(
> + from input;
> + type as type_ [required] => input.parse()?,
> + name [required] => input.parse()?,
> + authors => {
> + let list;
> + bracketed!(list in input);
> + Punctuated::parse_terminated(&list)?
> + },
> + description => input.parse()?,
> + license [required] => input.parse()?,
> + alias => {
> + let list;
> + bracketed!(list in input);
> + Punctuated::parse_terminated(&list)?
> + },
> + firmware => {
> + let list;
> + bracketed!(list in input);
> + Punctuated::parse_terminated(&list)?
> + },
> + imports_ns => {
> + let list;
> + bracketed!(list in input);
> + Punctuated::parse_terminated(&list)?
> + },
> + params => {
> + let list;
> + braced!(list in input);
> + Punctuated::parse_terminated(&list)?
> + },
> + );
> +
> + Ok(ModuleInfo {
> + type_,
> + license,
> + name,
> + authors,
> + description,
> + alias,
> + firmware,
> + imports_ns,
> + params,
> + })
> + }
> +}
>
> +pub(crate) fn module(info: ModuleInfo) -> Result<TokenStream> {
> // Rust does not allow hyphens in identifiers, use underscore instead.
> - let ident = info.name.replace('-', "_");
> + let ident = info.name.value().replace('-', "_");
> let mut modinfo = ModInfoBuilder::new(ident.as_ref());
> if let Some(authors) = &info.authors {
> for author in authors {
> - modinfo.emit("author", author);
> + modinfo.emit("author", &author.value());
> }
> }
> if let Some(description) = &info.description {
> - modinfo.emit("description", description);
> + modinfo.emit("description", &description.value());
> }
> - modinfo.emit("license", &info.license);
> + modinfo.emit("license", &info.license.value());
> if let Some(aliases) = &info.alias {
> for alias in aliases {
> - modinfo.emit("alias", alias);
> + modinfo.emit("alias", &alias.value());
> }
> }
> if let Some(firmware) = &info.firmware {
> for fw in firmware {
> - modinfo.emit("firmware", fw);
> + modinfo.emit("firmware", &fw.value());
> }
> }
> if let Some(imports) = &info.imports_ns {
> for ns in imports {
> - modinfo.emit("import_ns", ns);
> + modinfo.emit("import_ns", &ns.value());
> }
> }
>
> @@ -366,7 +459,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>
> modinfo.emit_params(&info);
>
> - format!(
> + Ok(format!(
> "
> /// The module name.
> ///
> @@ -536,12 +629,12 @@ mod module_parameters {{
> }}
> ",
> type_ = info.type_,
> - name = info.name,
> + name = info.name.value(),
> ident = ident,
> modinfo = modinfo.buffer,
> params = modinfo.param_buffer,
> initcall_section = ".initcall6.init"
> )
> .parse()
> - .expect("Error parsing formatted string into token stream.")
> + .expect("Error parsing formatted string into token stream."))
> }
> --
> 2.51.2
>
Powered by blists - more mailing lists