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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, Aug 16, 2024 at 08:50:53AM -0700, Sami Tolvanen wrote:
> Hi Greg,
> 
> On Fri, Aug 16, 2024 at 12:20 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Aug 15, 2024 at 05:39:20PM +0000, Sami Tolvanen wrote:
> > > Distributions that want to maintain a stable kABI need the ability to
> > > add reserved fields to kernel data structures that they anticipate
> > > will be modified during the ABI support timeframe, either by LTS
> > > updates or backports.
> > >
> > > With genksyms, developers would typically hide changes to the reserved
> > > fields from version calculation with #ifndef __GENKSYMS__, which would
> > > result in the symbol version not changing even though the actual type
> > > of the reserved field changes. When we process precompiled object
> > > files, this is again not an option.
> > >
> > > To support stable symbol versions for reserved fields, change the
> > > union type processing to recognize field name prefixes, and if the
> > > union contains a field name that starts with __kabi_reserved, only use
> > > the type of that field for computing symbol versions. In other words,
> > > let's assume we have a structure where we want to reserve space for
> > > future changes:
> > >
> > >   struct struct1 {
> > >     long a;
> > >     long __kabi_reserved_0; /* reserved for future use */
> > >   };
> > >   struct struct1 exported;
> > >
> > > gendwarfksyms --debug produces the following output:
> > >
> > >   variable structure_type struct1 {
> > >     member base_type long int byte_size(8) encoding(5) data_member_location(0),
> > >     member base_type long int byte_size(8) encoding(5) data_member_location(8),
> > >   } byte_size(16);
> > >   #SYMVER exported 0x67997f89
> > >
> > > To take the reserved field into use, a distribution would replace it
> > > with a union, with one of the fields keeping the __kabi_reserved name
> > > prefix for the original type:
> > >
> > >   struct struct1 {
> > >     long a;
> > >     union {
> > >       long __kabi_reserved_0;
> > >       struct {
> > >           int b;
> > >           int v;
> > >       };
> > >     };
> > >
> >
> > Ah, ignore my previous email, here's the --stable stuff.
> >
> > But this all needs to go into some documentation somewhere, trying to
> > dig it out of a changelog is going to be impossible to point people at.
> 
> I agree, which is why I included the details in the comments too.
> There's also an example file if you scroll down a bit further, but I
> can certainly add some actual documentation too. Since the --stable
> bits are not really needed in the mainline kernel, do you prefer a
> file in Documentation/ or is it sufficient to expand the example files
> to include any missing details?

Ah, I missed the examples, I thought that was a test for the feature :)

Yes, it needs to be documented somewhere, and usually documentation is
in Documentation/ so that it shows up on the web and everywhere else.

> > > +/* See dwarf.c:process_reserved */
> > > +#define RESERVED_PREFIX "__kabi_reserved"
> >
> > Seems semi-sane, I can live with this.
> 
> Is there something you'd change to make this more than semi-sane?

I can't think of it, but perhaps we need a check somewhere to ensure
that these symbol names do NOT end up in the main kernel tree?

Or just keep this whole patch as an add-on on the end that is only
applied by the distro kernels and is not merged into mainline at all?

> > I don't know if you want to take the next step and provide examples of
> > how to use this in "easy to use macros" for it all, but if so, that
> > might be nice.
> 
> This should already work with the macros Android uses, for example,
> with minor changes. The current example file doesn't include macro
> wrappers, but I can add them in the next version.

The Android macros are a copy of what SLES and RHEL does so that's good.

And yes, an example macro would be nice so we all don't have to reinvent
it yet-again like we have done already.  Consolidation is nice.

> > Especially as I have no idea how you are going to do
> > this with the rust side of things, this all will work for any structures
> > defined in .rs code, right?
> 
> Yes, Rust structures can use the same scheme. Accessing union members
> might be less convenient than in C, but can presumably be wrapped in
> helper macros if needed.

That feels ripe for problems for any rust code as forcing a helper macro
for a "normal" access to a structure field is going to be a lot of churn
over time.  Is the need for a macro due to the fact that accessing a
union is always considered "unsafe" in rust?  If that's the case, ick,
this is going to get even messier even faster as the need for sprinkling
unsafe accesses everywhere for what used to be a normal/safe one will
cause people to get nervous...

thanks,

greg k-h