Message-ID: <2024082257-refrain-subsector-b6c4@gregkh>
Date: Thu, 22 Aug 2024 15:29:39 +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 Thu, Aug 22, 2024 at 05:55:32AM +0000, Benno Lossin wrote:
> On 22.08.24 01:29, Greg Kroah-Hartman wrote:
> > 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.
> 
> Yeah, that's what I thought.
> 
> > 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.)
> 
> The approach already works for a adding a single field and I got from
> the discussions with Matthew and Sami that that is the most common case.
> We will reach out to the Rust folks and see what we can do about the
> multiple field case.

No, single field is NOT the common case, the common case is reserving
multiple padding variables in a structure as lots of things can change
of the long lifetimes of some of these kernel trees.  Look at the
changes in the Android or SLES or RHEL kernels for specifics.

Here's one example in the android tree where 4 64bit fields are reserved
for future abi changes:
	https://android.googlesource.com/kernel/common/+/refs/heads/android12-5.10/include/linux/fs.h#421

And here's a different place where a field is being used with many
remaining for future use:
	https://android.googlesource.com/kernel/common/+/refs/heads/android12-5.10/include/linux/sched.h#1379

And also, we want/need lots of other space reservation at times, look at
how "Others" can get access to reserved areas in structures that need to
be done in an abi-safe way:
	https://android.googlesource.com/kernel/common/+/refs/heads/android12-5.10/include/linux/sched.h#1375

All of this also needs to be possible in any structures that are
exported by rust code if vendors want to have a way to track and ensure
that abis do not change over time, just like they can today in C code.

Or if not possible, just don't export any rust structures at all :)

hope this helps,

greg k-h

Powered by blists - more mailing lists