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: <2024082229-elevation-emporium-8118@gregkh>
Date: Thu, 22 Aug 2024 07:29:48 +0800
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: Matthew Maurer <mmaurer@...gle.com>,
	Sami Tolvanen <samitolvanen@...gle.com>,
	Masahiro Yamada <masahiroy@...nel.org>,
	Luis Chamberlain <mcgrof@...nel.org>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Gary Guo <gary@...yguo.net>, Petr Pavlu <petr.pavlu@...e.com>,
	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

On Wed, Aug 21, 2024 at 11:31:25AM +0000, Benno Lossin wrote:
> On 20.08.24 22:03, Matthew Maurer wrote:
> >>> The way `KAbiReserved` is implemented is via a `union` (maybe a bit
> >>> ironic, considering what I said in my other replies, but in this case,
> >>> we would provide a safe abstraction over this `union`, thus avoiding
> >>> exposing users of this type to `unsafe`):
> >>>
> >>>     #[repr(C)]
> >>>     pub union KAbiReserved<T, R> {
> >>>         value: T,
> >>>         _reserved: R,
> >>>     }
> >>
> >> I like this approach even better, assuming any remaining issues with
> >> ownership etc. can be sorted out. This would also look identical to
> >> the C version in DWARF if you rename _reserved in the union to
> >> __kabi_reserved. Of course, we can always change gendwarfksyms to
> >> support a different scheme for Rust code if a better solution comes
> >> along later.
> 
> Yeah sure, that should also then work directly with this patch, right?
> 
> >> Sami
> > 
> > Agreement here - this seems like a good approach to representing
> > reserved in Rust code. A few minor adjustments we discussed off-list
> > which aren't required for gendwarfksyms to know about:
> > 1. Types being added to reserved fields have to be `Copy`, e.g. they
> > must be `!Drop`.
> > 2. Types being added to reserved fields must be legal to be
> > represented by all zeroes.
> > 3. Reserved fields need to be initialized to zero before having their
> > union set to the provided value when constructing them.
> > 4. It may be helpful to have delegating trait implementations to avoid
> > the couple places where autoderef won't handle the conversion.
> > 
> > While I think this is the right solution, esp. since it can share a
> > representation with C, I wanted to call out one minor shortfall - a
> > reserved field can only be replaced by one type. We could still
> > indicate a replacement by two fields the same as in C, by using a
> > tuple which will look like an anonymous struct. The limitation will be
> > that if two or more new fields were introduced, we'd need to edit the
> > patches accessing them to do foo.x.y and foo.x.z for their accesses
> > instead of simply foo.y and foo.z - the autoref trick only works for a
> > single type.
> 
> We will have to see how often multiple fields are added to a struct. If
> they are infrequent and it's fine for those patches to then touch the
> field accesses, then I think we can just stick with this approach.
> If there are problems with that, we can also try the following:
> all fields of kABI structs must be private and must only be accessed
> through setters/getters. We can then modify the body the setters/getters
> to handle the additional indirection.

That's just not going to work, sorry.  Remember, the goal here is to
keep the code that comes from kernel.org identical to what you have in
your "enterprise" kernel tree, with the exception of the few extra
"padding" fields you have added to allow for changes in the future in
the kernel.org versions.

Requiring all kernel.org changes that add a new field to a structure to
only do so with a settter/getter is going to just not fly at all as they
will not care one bit.

Or, we can just forget about "abi stability" for rust code entirely,
which I am totally fine with.  It's something that managers seem to like
for a "check box" but in reality, no one really needs it (hint, vendors
rebuild their code anyway.)

thanks,

greg k-h