linux-kernel - Re: [PATCH v2 16/19] gendwarfksyms: Add support for reserved structure fields

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: <CABCJKufJD6Zea7P_aPHNQQgCMgqkw98-XcvW8hRaTx6kcg4vUw@mail.gmail.com>
Date: Thu, 12 Sep 2024 15:37:45 -0700
From: Sami Tolvanen <samitolvanen@...gle.com>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: Petr Pavlu <petr.pavlu@...e.com>, Miroslav Benes <mbenes@...e.cz>, 
	Masahiro Yamada <masahiroy@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, 
	Miguel Ojeda <ojeda@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Matthew Maurer <mmaurer@...gle.com>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, Neal Gompa <neal@...pa.dev>, 
	Hector Martin <marcan@...can.st>, Janne Grunau <j@...nau.net>, Asahi Linux <asahi@...ts.linux.dev>, 
	linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-modules@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 16/19] gendwarfksyms: Add support for reserved
 structure fields

Hi,

On Thu, Sep 12, 2024 at 2:58 PM Benno Lossin <benno.lossin@...ton.me> wrote:
>
> On 12.09.24 22:58, Sami Tolvanen wrote:
> > That's an interesting point. Is the problem that you cannot assign
> > arbitrary values to the Rust enum that bindgen generates, or is using
> > a #define the problem? We could probably just make the hidden enum
> > values visible to bindgen only if needed.
>
> So if I take your example from above add it to our bindgen input, then I
> get the following output:
>
>     pub const e_A: my_own_test_enum = 0;
>     pub const e_B: my_own_test_enum = 1;
>     pub type e_enum = core::ffi::c_uint;
>
> So it doesn't pick up the other constants at all. That is probably
> because we haven't enabled the bindgen flag that adds support for
> function-like macros. If I enable that flag (`--clang-macro-fallback`,
> then the output becomes:
>
>     pub const C: u32 = 2;
>     pub const D: u32 = 3;
>     pub const e_A: e = 0;
>     pub const e_B: e = 1;
>     pub type e = ::std::os::raw::c_uint;
>
> So it doesn't really work as we would like it to (ie missing e_ prefix).

If defines are a problem, we can always use a const int instead. It
doesn't have to be defined inside the enum either, and probably we can
add a prefix too.

> But even if bindgen were to start supporting `#define` inside of the
> enum. It might still have a problem with the `#define`: there is the
> `--rustified-enum <REGEX>` option for bindgen that would change the
> output to this:
>
>     #[repr(u32)]
>     #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
>     pub enum e {
>         A = 0,
>         B = 1,
>     }
>
> Which makes using the values on the Rust side a lot easier, since you
> get exhaustiveness checks when using `match`. Adding the
> `--clang-macro-fallback` flag, I get:
>
>     pub const C: u32 = 2;
>     pub const D: u32 = 3;
>     #[repr(u32)]
>     #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
>     pub enum e {
>         A = 0,
>         B = 1,
>     }
>
> Which is a big problem, because the enum `e` won't have 2 or 3 as valid
> values (it will be UB to write them to a variable of type `e`).

Yes, I sort of thought that this might be an issue. I don't see this
in bindgen flags right now, are you planning on switching the kernel
bindgen to use --rustified-enum?

If you do plan to use --rustified-enum, we could just use #ifdef
__BINDGEN__ to hide the fields from everyone else, but I think we
might actually need a more generic solution after all. I'll think
about it a bit more.

> Would you add conditions to the `#define`? For example checking for the
> version of kABI? (or how would it work?)

Perhaps the folks maintaining distros can chime in, but I suspect
there's typically one kABI version per branch, so there should be no
need to maintain multiple kABI versions in the same source file.

Sami