Message-ID: <77e8e20c-8ca1-4df7-a4d7-ed77454f1754@proton.me>
Date: Thu, 22 Aug 2024 12:00:15 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
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 22.08.24 09:29, Greg Kroah-Hartman wrote:
> 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.

Thanks for letting me know.

> 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

Let me correct myself, it's only possible to replace one `KAbiReserved`
by one new field. You can have as many fields of type `KAbiReserved` as
you want. The thing that you can't do is replace a single `KAbiReserved`
field by multiple (well you can, but then you have to change the sites
that use it).

> 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.

All of those structs need to be `repr(C)`, otherwise they don't
have a stable layout to begin with. Other than that, only autoderef
might be missing. But we would have to see if that even comes up.

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

I think that we should try to get this working.

---
Cheers,
Benno

Powered by blists - more mailing lists