diff mbox series

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

Message ID 20240815173903.4172139-37-samitolvanen@google.com (mailing list archive)
State New
Headers show
Series Implement DWARF modversions | expand

Commit Message

Sami Tolvanen Aug. 15, 2024, 5:39 p.m. UTC
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;
      };
    };

gendwarfksyms --debug now produces the following output:

  variable structure_type struct1 {
    member base_type long int byte_size(8) encoding(5) data_member_location(0),
    member union_type <unnamed> {
      member base_type long int byte_size(8) encoding(5),
      member structure_type <unnamed> {
        member base_type int byte_size(4) encoding(5) data_member_location(0),
        member base_type int byte_size(4) encoding(5) data_member_location(4),
      } byte_size(8),
    } byte_size(8) data_member_location(8),
  } byte_size(16);
  #SYMVER exported 0x66916c41

But with --stable, gendwarfksyms only uses the reserved field for the
version calculation, thus leaving the symbol version unchanged:

  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

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 scripts/gendwarfksyms/dwarf.c             | 148 +++++++++++++++++++++-
 scripts/gendwarfksyms/examples/reserved.c |  66 ++++++++++
 scripts/gendwarfksyms/gendwarfksyms.h     |  18 +++
 3 files changed, 229 insertions(+), 3 deletions(-)
 create mode 100644 scripts/gendwarfksyms/examples/reserved.c

Comments

Greg KH Aug. 16, 2024, 7:20 a.m. UTC | #1
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.

> +/* See dwarf.c:process_reserved */
> +#define RESERVED_PREFIX "__kabi_reserved"

Seems semi-sane, I can live with this.

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

thanks,

greg k-h
Sami Tolvanen Aug. 16, 2024, 3:50 p.m. UTC | #2
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?

> > +/* 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 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.

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

Sami
Greg KH Aug. 17, 2024, 7:41 a.m. UTC | #3
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
Benno Lossin Aug. 17, 2024, 1:19 p.m. UTC | #4
On 17.08.24 09:41, Greg Kroah-Hartman wrote:
> On Fri, Aug 16, 2024 at 08:50:53AM -0700, Sami Tolvanen wrote:
>> 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:
>>> 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...

The reason for union field access being unsafe in Rust is that you can
easily shoot yourself in the foot. For example:

    union Foo {
        a: bool,
        b: i32,
    }

    let foo = Foo { b: 3 };
    println!("{}", unsafe { foo.a });

This is UB, since `3` is of course not a valid value for `bool`. With
unions the compiler doesn't know which variant is active.

Since unions are unsafe in Rust, we don't really use them directly (in
the `kernel` crate, we have 0 union definitions). Instead we use certain
unions from the stdlib such as `MaybeUninit`. But the fields of that
union are private and never accessed.

In general, unions in Rust are very important primitive types, but they
are seldomly used directly. Instead enums are used a lot more, since you
don't need to roll your own tagged unions.

For this use-case (the one in the patch), I don't really know if we want
to copy the approach from C. Do we even support exporting kABI from
Rust? If yes, then we I would recommend we tag it in the source code
instead of using a union. Here the example from the patch adapted:

    #[repr(C)] // needed for layout stability
    pub struct Struct1 {
        a: u64,
        #[kabi_reserved(u64)] // this marker is new
        _reserved: u64,
    }

And then to use the reserved field, you would do this:
    
    #[repr(C)]
    pub struct Struct1 {
        a: u64,
        #[kabi_reserved(u64)]
        b: Struct2,
    }

    #[repr(C)]
    pub struct Struct2 {
        b: i32,
        v: i32,
    }

The attribute would check that the size of the two types match and
gendwarfksyms would use the type given in "()" instead of the actual
type.

---
Cheers,
Benno
Greg KH Aug. 19, 2024, 6:25 p.m. UTC | #5
On Sat, Aug 17, 2024 at 01:19:55PM +0000, Benno Lossin wrote:
> On 17.08.24 09:41, Greg Kroah-Hartman wrote:
> > On Fri, Aug 16, 2024 at 08:50:53AM -0700, Sami Tolvanen wrote:
> >> 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:
> >>> 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...
> 
> The reason for union field access being unsafe in Rust is that you can
> easily shoot yourself in the foot. For example:
> 
>     union Foo {
>         a: bool,
>         b: i32,
>     }
> 
>     let foo = Foo { b: 3 };
>     println!("{}", unsafe { foo.a });
> 
> This is UB, since `3` is of course not a valid value for `bool`. With
> unions the compiler doesn't know which variant is active.

Understood, then why attempt to use a union for this type of "abi safe
padding"?

> Since unions are unsafe in Rust, we don't really use them directly (in
> the `kernel` crate, we have 0 union definitions). Instead we use certain
> unions from the stdlib such as `MaybeUninit`. But the fields of that
> union are private and never accessed.
> 
> In general, unions in Rust are very important primitive types, but they
> are seldomly used directly. Instead enums are used a lot more, since you
> don't need to roll your own tagged unions.
> 
> For this use-case (the one in the patch), I don't really know if we want
> to copy the approach from C. Do we even support exporting kABI from
> Rust?

That's the goal here, you want to create an abi that can change over
time without "breaking" the abi.  Usually this is just adding additional
padding in structures to have room for new additions.

> If yes, then we I would recommend we tag it in the source code
> instead of using a union. Here the example from the patch adapted:
> 
>     #[repr(C)] // needed for layout stability
>     pub struct Struct1 {
>         a: u64,
>         #[kabi_reserved(u64)] // this marker is new
>         _reserved: u64,
>     }
> 
> And then to use the reserved field, you would do this:
>     
>     #[repr(C)]
>     pub struct Struct1 {
>         a: u64,
>         #[kabi_reserved(u64)]
>         b: Struct2,
>     }
> 
>     #[repr(C)]
>     pub struct Struct2 {
>         b: i32,
>         v: i32,
>     }
> 
> The attribute would check that the size of the two types match and
> gendwarfksyms would use the type given in "()" instead of the actual
> type.

Remember the "goal" here is to NOT have to modify the places in the
kernel that use the new field in the structure, but for that to "just
work".  Your change here wouldn't allow that as any use of the new "b"
field would have to be through something in "Struct2", not directly in
Struct1, right?

We can mess with the structure definitions but we should not have to
touch the places where the structure fields are used at all.  If that's
going to be a requirement (as it sounds like it would with the use of
unsafe in the union), then this is not going to be a solution at all.

thanks,

greg k-h
Sami Tolvanen Aug. 19, 2024, 7:38 p.m. UTC | #6
Hi Benno,

On Sat, Aug 17, 2024 at 01:19:55PM +0000, Benno Lossin wrote:
> 
> For this use-case (the one in the patch), I don't really know if we want
> to copy the approach from C. Do we even support exporting kABI from
> Rust? If yes, then we I would recommend we tag it in the source code
> instead of using a union. Here the example from the patch adapted:
> 
>     #[repr(C)] // needed for layout stability
>     pub struct Struct1 {
>         a: u64,
>         #[kabi_reserved(u64)] // this marker is new
>         _reserved: u64,
>     }
> 
> And then to use the reserved field, you would do this:
>     
>     #[repr(C)]
>     pub struct Struct1 {
>         a: u64,
>         #[kabi_reserved(u64)]
>         b: Struct2,
>     }
> 
>     #[repr(C)]
>     pub struct Struct2 {
>         b: i32,
>         v: i32,
>     }
> 
> The attribute would check that the size of the two types match and
> gendwarfksyms would use the type given in "()" instead of the actual
> type.

This definitely looks cleaner than unions in Rust, but how would this
scheme be visible in DWARF? You might also need to expand the annotation
to allow replacing one reserved field with multiple smaller ones without
using structs.

Sami
Benno Lossin Aug. 19, 2024, 9:46 p.m. UTC | #7
On 19.08.24 20:25, Greg Kroah-Hartman wrote:
> On Sat, Aug 17, 2024 at 01:19:55PM +0000, Benno Lossin wrote:
>> On 17.08.24 09:41, Greg Kroah-Hartman wrote:
>>> On Fri, Aug 16, 2024 at 08:50:53AM -0700, Sami Tolvanen wrote:
>>>> 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:
>>>>> 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...
>>
>> The reason for union field access being unsafe in Rust is that you can
>> easily shoot yourself in the foot. For example:
>>
>>     union Foo {
>>         a: bool,
>>         b: i32,
>>     }
>>
>>     let foo = Foo { b: 3 };
>>     println!("{}", unsafe { foo.a });
>>
>> This is UB, since `3` is of course not a valid value for `bool`. With
>> unions the compiler doesn't know which variant is active.
> 
> Understood, then why attempt to use a union for this type of "abi safe
> padding"?

I don't follow, I thought this was the idea from the thread above. (ie
just do what C does)

>> Since unions are unsafe in Rust, we don't really use them directly (in
>> the `kernel` crate, we have 0 union definitions). Instead we use certain
>> unions from the stdlib such as `MaybeUninit`. But the fields of that
>> union are private and never accessed.
>>
>> In general, unions in Rust are very important primitive types, but they
>> are seldomly used directly. Instead enums are used a lot more, since you
>> don't need to roll your own tagged unions.
>>
>> For this use-case (the one in the patch), I don't really know if we want
>> to copy the approach from C. Do we even support exporting kABI from
>> Rust?
> 
> That's the goal here, you want to create an abi that can change over
> time without "breaking" the abi.  Usually this is just adding additional
> padding in structures to have room for new additions.
> 
>> If yes, then we I would recommend we tag it in the source code
>> instead of using a union. Here the example from the patch adapted:
>>
>>     #[repr(C)] // needed for layout stability
>>     pub struct Struct1 {
>>         a: u64,
>>         #[kabi_reserved(u64)] // this marker is new
>>         _reserved: u64,
>>     }
>>
>> And then to use the reserved field, you would do this:
>>
>>     #[repr(C)]
>>     pub struct Struct1 {
>>         a: u64,
>>         #[kabi_reserved(u64)]
>>         b: Struct2,
>>     }
>>
>>     #[repr(C)]
>>     pub struct Struct2 {
>>         b: i32,
>>         v: i32,
>>     }
>>
>> The attribute would check that the size of the two types match and
>> gendwarfksyms would use the type given in "()" instead of the actual
>> type.
> 
> Remember the "goal" here is to NOT have to modify the places in the
> kernel that use the new field in the structure, but for that to "just
> work".  Your change here wouldn't allow that as any use of the new "b"
> field would have to be through something in "Struct2", not directly in
> Struct1, right?

This confuses me, since I thought that in C you would need to use the
new fields. So for example we have

    void increment(struct struct1 *x)
    {
        x->a += 1;
    }

and then we do the extension and also want to increment `b`, then we
have to do this:

    void increment(struct struct1 *x)
    {
        x->a += 1;
        x->b += 1;
    }

I am not 100% sure if you meant the following problem: if a user uses
the `increment` function like this:

    struct struct1 x = { .a = 0, .__kabi_reserved_0 = 0 };
    increment(&x);

Then in the C example they don't need to change their usage. In Rust,
they would, but we shouldn't make the struct fields public, then they
can't create the struct using the initializer syntax, but instead we
would provide stable initialization functions. Translating the entire
example to Rust:

    impl Struct1 {
        pub fn new(a: u64) -> Self {
            Self { a, _reserved: 0 }
        }

        pub fn increment(&mut self) {
            self.a += 1;
        }
    }

Then after adding the new field, this becomes:

    impl Struct1 {
        pub fn new(a: u64) -> Self {
            Self { a, b: Struct2 { b: 0, v: 0 } }
        }

        pub fn increment(&mut self) {
            self.a += 1;
            self.b.b += 1;
        }
    }

So the following user would also not have to change the code:

    let mut x = Struct1::new(0);
    x.increment();

> We can mess with the structure definitions but we should not have to
> touch the places where the structure fields are used at all.  If that's
> going to be a requirement (as it sounds like it would with the use of
> unsafe in the union), then this is not going to be a solution at all.

So the union approach *could* work and the users of the API would not
have to use `unsafe`. But doing it the way I suggest above is going to
be cleaner, as the people who use the API won't ever need to know of the
internals.

---
Cheers,
Benno
Benno Lossin Aug. 19, 2024, 10:16 p.m. UTC | #8
On 19.08.24 21:38, Sami Tolvanen wrote:
> Hi Benno,
> 
> On Sat, Aug 17, 2024 at 01:19:55PM +0000, Benno Lossin wrote:
>>
>> For this use-case (the one in the patch), I don't really know if we want
>> to copy the approach from C. Do we even support exporting kABI from
>> Rust? If yes, then we I would recommend we tag it in the source code
>> instead of using a union. Here the example from the patch adapted:
>>
>>     #[repr(C)] // needed for layout stability
>>     pub struct Struct1 {
>>         a: u64,
>>         #[kabi_reserved(u64)] // this marker is new
>>         _reserved: u64,
>>     }
>>
>> And then to use the reserved field, you would do this:
>>
>>     #[repr(C)]
>>     pub struct Struct1 {
>>         a: u64,
>>         #[kabi_reserved(u64)]
>>         b: Struct2,
>>     }
>>
>>     #[repr(C)]
>>     pub struct Struct2 {
>>         b: i32,
>>         v: i32,
>>     }
>>
>> The attribute would check that the size of the two types match and
>> gendwarfksyms would use the type given in "()" instead of the actual
>> type.
> 
> This definitely looks cleaner than unions in Rust, but how would this
> scheme be visible in DWARF? You might also need to expand the annotation
> to allow replacing one reserved field with multiple smaller ones without
> using structs.

Hmm that's a good question, I have no idea how DWARF works. The way you
do it in this patch is just by the name of the field, right?

If Rust's DWARF output contains exact types names (I just checked this,
I *think* that this is the case, but I have never used/seen DWARF
before), we might be able to just create a `KAbiReserved<T, R>` type
that you search for instead of the attribute. The usage would then be
like this:

    #[repr(C)]
    pub struct Struct1 {
        a: u64,
        _reserved: KAbiReserved<(), u64>,
    }

And then when adding a new field, you would do this:

    #[repr(C)]
    pub struct Struct1 {
        a: u64,
        b: KAbiReserved<Struct2, u64>,
    }

    /* Struct2 as above */

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,
    }

    impl<T, R> Drop for KAbiReserved<T, R> {
        fn drop(&mut self) {
            let val = &mut **self;
            unsafe { ptr::drop_in_place(val) };
        }
    }

    impl<T, R> Deref for KAbiReserved<T, R> {
        type Target = T;

        fn deref(&self) -> &Self::Target {
            unsafe { &self.value }
        }
    }

    impl<T, R> DerefMut for KAbiReserved<T, R> {
        fn deref_mut(&mut self) -> &mut Self::Target {
            unsafe { &mut self.value }
        }
    }

    impl<T, R> KAbiReserved<T, R> {
        pub fn new(value: T) -> Self {
            // we want to ensure that people don't accidentally use a bigger type.
            build_assert!(size_of::<R>() >= size_of::<T>());
            Self { value }
        }

        pub fn into_value(self) -> T {
            unsafe { self.value }
        }
    }

This needs some more work, but is a lot cleaner than having the users
use raw unions + unsafe (essentially they would re-implement the code
above).

If you want me to turn the above into a patch let me know (also if you
or someone else wants to give it a try, then please go ahead! If you
need help, just send me a mail or a message on zulip).

---
Cheers,
Benno
Sami Tolvanen Aug. 20, 2024, 6:47 p.m. UTC | #9
On Mon, Aug 19, 2024 at 10:17 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 19.08.24 21:38, Sami Tolvanen wrote:
> >
> > This definitely looks cleaner than unions in Rust, but how would this
> > scheme be visible in DWARF? You might also need to expand the annotation
> > to allow replacing one reserved field with multiple smaller ones without
> > using structs.
>
> Hmm that's a good question, I have no idea how DWARF works. The way you
> do it in this patch is just by the name of the field, right?

Correct, it just looks at the name of the union fields.

> If Rust's DWARF output contains exact types names (I just checked this,
> I *think* that this is the case, but I have never used/seen DWARF
> before), we might be able to just create a `KAbiReserved<T, R>` type
> that you search for instead of the attribute. The usage would then be
> like this:
>
>     #[repr(C)]
>     pub struct Struct1 {
>         a: u64,
>         _reserved: KAbiReserved<(), u64>,
>     }
>
> And then when adding a new field, you would do this:
>
>     #[repr(C)]
>     pub struct Struct1 {
>         a: u64,
>         b: KAbiReserved<Struct2, u64>,
>     }
>
>     /* Struct2 as above */
>
> 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.

Sami
Matthew Maurer Aug. 20, 2024, 8:03 p.m. UTC | #10
> > 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.
>
> 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.
Benno Lossin Aug. 21, 2024, 11:31 a.m. UTC | #11
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.

If that also sounds too much work compared to the C side, then we can
get in touch with the Rust language folks and see if they can provide us
with a better solution.

---
Cheers,
Benno
Sami Tolvanen Aug. 21, 2024, 11:01 p.m. UTC | #12
On Wed, Aug 21, 2024 at 4:31 AM Benno Lossin <benno.lossin@proton.me> 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?

Yes, this would work without changes.

Sami
Greg KH Aug. 21, 2024, 11:29 p.m. UTC | #13
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
Benno Lossin Aug. 22, 2024, 5:55 a.m. UTC | #14
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.

---
Cheers,
Benno
Greg KH Aug. 22, 2024, 7:29 a.m. UTC | #15
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
Benno Lossin Aug. 22, 2024, noon UTC | #16
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
Greg KH Aug. 22, 2024, 11:53 p.m. UTC | #17
On Thu, Aug 22, 2024 at 12:00:15PM +0000, Benno Lossin wrote:
> > 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).

That's odd/foolish, why would that be the case?  Isn't that exactly what
a union is for?  How are you going to know ahead of time what size types
to save space for?

All we really want to do here is "pad out this structure by X bytes" and
then later "take X bytes to represent this variable" at a later point in
time.

Surely rust can do that, right?  :)

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

Do we have any way to enforce at build time that exports from rust code
are in this format to ensure that this will work properly going forward?
I guess someone is going to have to write the first api in rust that
actually gets used before we worry about this...

thanks,

greg k-h
Sami Tolvanen Aug. 23, 2024, 7:17 p.m. UTC | #18
On Thu, Aug 22, 2024 at 11:53 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 22, 2024 at 12:00:15PM +0000, Benno Lossin wrote:
> > > 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).
>
> That's odd/foolish, why would that be the case?  Isn't that exactly what
> a union is for?  How are you going to know ahead of time what size types
> to save space for?

I believe Benno is referring to the lack of anonymous structures in
Rust. While you can replace a reserved field with a struct that
contains multiple smaller fields, you can't access the fields
transparently from the parent struct like you can in C:

    struct s { struct { u32 a; u32 b; }; };
    struct s s;
    s.a = 0;
    ...

It looks like nightly Rust does have some level of support for unnamed
fields in unions, but the implementation is not yet complete:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=4f268d308fe6aa7a47566c7080c6e604

Benno, Matt, are you familiar with this feature?

Sami
Benno Lossin Aug. 24, 2024, 1:27 p.m. UTC | #19
On 23.08.24 01:53, Greg Kroah-Hartman wrote:
> On Thu, Aug 22, 2024 at 12:00:15PM +0000, Benno Lossin wrote:
>>> 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).
> 
> That's odd/foolish, why would that be the case?  Isn't that exactly what
> a union is for?  How are you going to know ahead of time what size types
> to save space for?

That's what I interpreted from the links you provided above, there are
multiple invocations of `ANDROID_KABI_RESERVE` and I figured they each
can be used to insert a new field. Or can you replace each one by as
many fields as you want, as long as the size is still fine?

> All we really want to do here is "pad out this structure by X bytes" and
> then later "take X bytes to represent this variable" at a later point in
> time.
> 
> Surely rust can do that, right?  :)

Not with all the other things that you need. I feel like this discussion
is dragging a bit on, so we will just ask the Rust folks if they have
any suggestions and if they don't we will ask for a solution. We can
then get back to this when that's done.
It's not like we need this immediately.

>>> 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.
> 
> Do we have any way to enforce at build time that exports from rust code
> are in this format to ensure that this will work properly going forward?
> I guess someone is going to have to write the first api in rust that
> actually gets used before we worry about this...

I don't know if we already have a way, but we will need one if people
start writing kabi in Rust.

---
Cheers,
Benno
Benno Lossin Aug. 24, 2024, 1:29 p.m. UTC | #20
On 23.08.24 21:17, Sami Tolvanen wrote:
> On Thu, Aug 22, 2024 at 11:53 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> On Thu, Aug 22, 2024 at 12:00:15PM +0000, Benno Lossin wrote:
>>>> 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).
>>
>> That's odd/foolish, why would that be the case?  Isn't that exactly what
>> a union is for?  How are you going to know ahead of time what size types
>> to save space for?
> 
> I believe Benno is referring to the lack of anonymous structures in
> Rust. While you can replace a reserved field with a struct that
> contains multiple smaller fields, you can't access the fields
> transparently from the parent struct like you can in C:
> 
>     struct s { struct { u32 a; u32 b; }; };
>     struct s s;
>     s.a = 0;
>     ...
> 
> It looks like nightly Rust does have some level of support for unnamed
> fields in unions, but the implementation is not yet complete:
> 
> https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=4f268d308fe6aa7a47566c7080c6e604
> 
> Benno, Matt, are you familiar with this feature?

No, thanks for pointing that out!

But this will run into the issue that field access for unions is
`unsafe`. So we can't really use it. I also tried to use our current
`KAbiReserved<T, R>` approach and using this as `T`:

    struct Foo {
        _: struct { a: u32, b: u32 }
    }

But that doesn't work.

---
Cheers,
Benno
Miroslav Benes Aug. 30, 2024, 9:34 a.m. UTC | #21
Hi,

On Thu, 15 Aug 2024, 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;
>       };
>     };

yes, this is one of the approaches we use in SLES. We add kabi paddings 
to some structures in advance (see [1] as a random example) and then use 
it later if needed.

It is not the only approach. Much more often we do not have a padding and 
use alignment holes ([5]), addition of a new member to the end of a 
structure ([2] or [3]) and such "tricks" ([4] for a newly fully defined 
structure).

It is not feasible to add kabi paddings to all structures. We also have a 
different approach to kabi in terms of its coverage than RHEL does for 
example (as far as I know).

Not sure if it is interesting to upstream but I wanted to mention that it 
is not only about the ability to add reserved fields to kernel structures 
in practice.

Regards,
Miroslav

[1] https://github.com/SUSE/kernel-source/blob/SLE15-SP6/patches.suse/crypto-add-suse_kabi_padding.patch
[2] https://github.com/SUSE/kernel-source/blob/SLE15-SP6/patches.kabi/0001-iommu-Add-static-iommu_ops-release_domain.patch
[3] https://github.com/SUSE/kernel-source/blob/SLE15-SP6/patches.kabi/nfs-Block-on-write-congestion-kabi-fixup.patch.
[4] https://github.com/SUSE/kernel-source/blob/SLE15-SP6/patches.kabi/of-kabi-workaround.patch
[5] https://github.com/SUSE/kernel-source/blob/SLE15-SP6/patches.kabi/KVM-x86-Snapshot-if-a-vCPU-s-vendor-model-is-AMD-vs..patch
Sami Tolvanen Aug. 31, 2024, 12:05 a.m. UTC | #22
Hi Miroslav,

On Fri, Aug 30, 2024 at 9:34 AM Miroslav Benes <mbenes@suse.cz> wrote:
>
> yes, this is one of the approaches we use in SLES. We add kabi paddings
> to some structures in advance (see [1] as a random example) and then use
> it later if needed.
>
> It is not the only approach. Much more often we do not have a padding and
> use alignment holes ([5]), addition of a new member to the end of a
> structure ([2] or [3]) and such "tricks" ([4] for a newly fully defined
> structure).

Thanks for bringing this up! Sounds like we're also going to need a
way to completely exclude specific fields from the output then. I
think we can use a similar union approach, but instead of instructing
the tool to use another type, we can just indicate that the field
should be skipped. I'll come up with a solution for v3.

Sami
Petr Pavlu Sept. 11, 2024, 11:43 a.m. UTC | #23
On 8/31/24 02:05, Sami Tolvanen wrote:
> On Fri, Aug 30, 2024 at 9:34 AM Miroslav Benes <mbenes@suse.cz> wrote:
>>
>> yes, this is one of the approaches we use in SLES. We add kabi paddings
>> to some structures in advance (see [1] as a random example) and then use
>> it later if needed.
>>
>> It is not the only approach. Much more often we do not have a padding and
>> use alignment holes ([5]), addition of a new member to the end of a
>> structure ([2] or [3]) and such "tricks" ([4] for a newly fully defined
>> structure).
> 
> Thanks for bringing this up! Sounds like we're also going to need a
> way to completely exclude specific fields from the output then. I
> think we can use a similar union approach, but instead of instructing
> the tool to use another type, we can just indicate that the field
> should be skipped. I'll come up with a solution for v3.

It might have been mentioned previously, not sure, but one more case to
consider is handling of enum declarations. New enumerators can be
typically added without breaking ABI, e.g. 'enum E { OLD1, OLD2, NEW }'.
It would be then great to have some ability to hide them from
gendwarfksyms.

I think neither of the __kabi_reserved or __gendwarfksyms_declonly
mechanism can currently help with that.
Sami Tolvanen Sept. 12, 2024, 4:06 p.m. UTC | #24
On Wed, Sep 11, 2024 at 4:43 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
>
> On 8/31/24 02:05, Sami Tolvanen wrote:
> > On Fri, Aug 30, 2024 at 9:34 AM Miroslav Benes <mbenes@suse.cz> wrote:
> >>
> >> yes, this is one of the approaches we use in SLES. We add kabi paddings
> >> to some structures in advance (see [1] as a random example) and then use
> >> it later if needed.
> >>
> >> It is not the only approach. Much more often we do not have a padding and
> >> use alignment holes ([5]), addition of a new member to the end of a
> >> structure ([2] or [3]) and such "tricks" ([4] for a newly fully defined
> >> structure).
> >
> > Thanks for bringing this up! Sounds like we're also going to need a
> > way to completely exclude specific fields from the output then. I
> > think we can use a similar union approach, but instead of instructing
> > the tool to use another type, we can just indicate that the field
> > should be skipped. I'll come up with a solution for v3.
>
> It might have been mentioned previously, not sure, but one more case to
> consider is handling of enum declarations. New enumerators can be
> typically added without breaking ABI, e.g. 'enum E { OLD1, OLD2, NEW }'.
> It would be then great to have some ability to hide them from
> gendwarfksyms.
>
> I think neither of the __kabi_reserved or __gendwarfksyms_declonly
> mechanism can currently help with that.

I thought about this a bit and I wonder if we need a separate
mechanism for that, or is it sufficient to just #define any additional
hidden values you want to add instead of including them in the enum?

  enum e {
      A,
      B,
  #define C (B + 1)
  #define D (C + 1)
  };

Do you see any issues with this approach? I think Clang would complain
about this with -Wassign-enum, but I'm not sure if we even enable that
in the kernel, and as long as you don't overflow the underlying type,
which is a requirement for not breaking the ABI anyway, it should be
fine.

Sami
Benno Lossin Sept. 12, 2024, 6:08 p.m. UTC | #25
On 12.09.24 18:06, Sami Tolvanen wrote:
> On Wed, Sep 11, 2024 at 4:43 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
>>
>> On 8/31/24 02:05, Sami Tolvanen wrote:
>>> On Fri, Aug 30, 2024 at 9:34 AM Miroslav Benes <mbenes@suse.cz> wrote:
>>>>
>>>> yes, this is one of the approaches we use in SLES. We add kabi paddings
>>>> to some structures in advance (see [1] as a random example) and then use
>>>> it later if needed.
>>>>
>>>> It is not the only approach. Much more often we do not have a padding and
>>>> use alignment holes ([5]), addition of a new member to the end of a
>>>> structure ([2] or [3]) and such "tricks" ([4] for a newly fully defined
>>>> structure).
>>>
>>> Thanks for bringing this up! Sounds like we're also going to need a
>>> way to completely exclude specific fields from the output then. I
>>> think we can use a similar union approach, but instead of instructing
>>> the tool to use another type, we can just indicate that the field
>>> should be skipped. I'll come up with a solution for v3.
>>
>> It might have been mentioned previously, not sure, but one more case to
>> consider is handling of enum declarations. New enumerators can be
>> typically added without breaking ABI, e.g. 'enum E { OLD1, OLD2, NEW }'.
>> It would be then great to have some ability to hide them from
>> gendwarfksyms.
>>
>> I think neither of the __kabi_reserved or __gendwarfksyms_declonly
>> mechanism can currently help with that.
> 
> I thought about this a bit and I wonder if we need a separate
> mechanism for that, or is it sufficient to just #define any additional
> hidden values you want to add instead of including them in the enum?
> 
>   enum e {
>       A,
>       B,
>   #define C (B + 1)
>   #define D (C + 1)
>   };
>
> 
> Do you see any issues with this approach? I think Clang would complain
> about this with -Wassign-enum, but I'm not sure if we even enable that
> in the kernel, and as long as you don't overflow the underlying type,
> which is a requirement for not breaking the ABI anyway, it should be
> fine.

Rust has problems with `#define`-style enums, because bindgen (the tool
that generates definitions for Rust to be able to call C code) isn't
able to convert them to Rust enums.

So if you can come up with an approach that allows you to continue to
use C enums instead of `#define`, we would appreciate that, since it
would make our lives a lot easier.

---
Cheers,
Benno
Sami Tolvanen Sept. 12, 2024, 8:58 p.m. UTC | #26
Hi Benno,

On Thu, Sep 12, 2024 at 11:08 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 12.09.24 18:06, Sami Tolvanen wrote:
> >
> > I thought about this a bit and I wonder if we need a separate
> > mechanism for that, or is it sufficient to just #define any additional
> > hidden values you want to add instead of including them in the enum?
> >
> >   enum e {
> >       A,
> >       B,
> >   #define C (B + 1)
> >   #define D (C + 1)
> >   };
> >
> >
> > Do you see any issues with this approach? I think Clang would complain
> > about this with -Wassign-enum, but I'm not sure if we even enable that
> > in the kernel, and as long as you don't overflow the underlying type,
> > which is a requirement for not breaking the ABI anyway, it should be
> > fine.
>
> Rust has problems with `#define`-style enums, because bindgen (the tool
> that generates definitions for Rust to be able to call C code) isn't
> able to convert them to Rust enums.
>
> So if you can come up with an approach that allows you to continue to
> use C enums instead of `#define`, we would appreciate that, since it
> would make our lives a lot easier.

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.

Sami
Benno Lossin Sept. 12, 2024, 9:58 p.m. UTC | #27
On 12.09.24 22:58, Sami Tolvanen wrote:
> Hi Benno,
> 
> On Thu, Sep 12, 2024 at 11:08 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On 12.09.24 18:06, Sami Tolvanen wrote:
>>>
>>> I thought about this a bit and I wonder if we need a separate
>>> mechanism for that, or is it sufficient to just #define any additional
>>> hidden values you want to add instead of including them in the enum?
>>>
>>>   enum e {
>>>       A,
>>>       B,
>>>   #define C (B + 1)
>>>   #define D (C + 1)
>>>   };
>>>
>>>
>>> Do you see any issues with this approach? I think Clang would complain
>>> about this with -Wassign-enum, but I'm not sure if we even enable that
>>> in the kernel, and as long as you don't overflow the underlying type,
>>> which is a requirement for not breaking the ABI anyway, it should be
>>> fine.
>>
>> Rust has problems with `#define`-style enums, because bindgen (the tool
>> that generates definitions for Rust to be able to call C code) isn't
>> able to convert them to Rust enums.
>>
>> So if you can come up with an approach that allows you to continue to
>> use C enums instead of `#define`, we would appreciate that, since it
>> would make our lives a lot easier.
> 
> 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).

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


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

Because we might want to have something similar on the Rust side then:

    #[repr(u32)]
    #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
    pub enum e {
        A = 0,
        B = 1,
        #[cfg(kabi >= "some-version")]
        C = 2,
        #[cfg(kabi >= "some-version")]
        B = 3,
    }

(still generated by bindgen though)

---
Cheers,
Benno
Sami Tolvanen Sept. 12, 2024, 10:37 p.m. UTC | #28
Hi,

On Thu, Sep 12, 2024 at 2:58 PM Benno Lossin <benno.lossin@proton.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
Benno Lossin Sept. 13, 2024, 8 a.m. UTC | #29
On 13.09.24 00:37, Sami Tolvanen wrote:
> Hi,
> 
> On Thu, Sep 12, 2024 at 2:58 PM Benno Lossin <benno.lossin@proton.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.

They might also be a problem, though I haven't checked. It would be best
if they can just stay in the `enum`.

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

You mean you don't see the `--clang-macro-fallback` option? I think it
was added in version 0.70.0.

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

Well we don't exactly plan to use `--rustified-enum`, the problem is
that transmuting the integer that C gives us to that enum is UB, when
the integer is not a valid bit pattern for that enum. Instead we would
like to have an option to generate both the Rust-style enum and a
newtype enum that can hold any integer value. We then check at runtime
that the value is in range and error otherwise. This is being worked on
at [1]. 
I would say that it has the same issue that `--rustified-enum` currently
has.

[1]: https://github.com/rust-lang/rust-bindgen/pull/2908

---
Cheers,
Benno
diff mbox series

Patch

diff --git a/scripts/gendwarfksyms/dwarf.c b/scripts/gendwarfksyms/dwarf.c
index bf28946c321e..d6252194692d 100644
--- a/scripts/gendwarfksyms/dwarf.c
+++ b/scripts/gendwarfksyms/dwarf.c
@@ -274,8 +274,12 @@  int process_die_container(struct state *state, struct die *cache,
 
 	res = checkp(dwarf_child(die, &current));
 	while (!res) {
-		if (match(&current))
-			check(func(state, cache, &current));
+		if (match(&current)) {
+			/* <0 = error, 0 = continue, >0 = stop */
+			res = checkp(func(state, cache, &current));
+			if (res)
+				return res;
+		}
 		res = checkp(dwarf_siblingof(&current, &current));
 	}
 
@@ -490,7 +494,145 @@  static int __process_structure_type(struct state *state, struct die *cache,
 
 DEFINE_PROCESS_STRUCTURE_TYPE(class)
 DEFINE_PROCESS_STRUCTURE_TYPE(structure)
-DEFINE_PROCESS_STRUCTURE_TYPE(union)
+
+static bool is_reserved_member(Dwarf_Die *die)
+{
+	const char *name = get_name(die);
+
+	return name && !strncmp(name, RESERVED_PREFIX, RESERVED_PREFIX_LEN);
+}
+
+static int __process_reserved_struct(struct state *state, struct die *cache,
+				     Dwarf_Die *die)
+{
+	Dwarf_Die type;
+
+	/*
+	 * If the union member is a struct, expect the placeholder type to
+	 * be the first member, i.e.:
+	 *
+	 * union {
+	 * 	type replaced_member;
+	 * 	struct {
+	 * 		type placeholder; // <- type
+	 * 	}
+	 * };
+	 *
+	 * Stop processing if this member isn't reserved.
+	 */
+	if (!is_reserved_member(die))
+		return NOT_RESERVED;
+
+	if (!get_ref_die_attr(die, DW_AT_type, &type)) {
+		error("structure member missing a type?");
+		return -1;
+	}
+
+	check(process_type(state, cache, &type));
+	return RESERVED;
+}
+
+static int __process_reserved_union(struct state *state, struct die *cache,
+				    Dwarf_Die *die)
+{
+	int res = NOT_RESERVED;
+	Dwarf_Die type;
+
+	if (!get_ref_die_attr(die, DW_AT_type, &type)) {
+		error("union member missing a type?");
+		return -1;
+	}
+
+	/*
+	 * We expect a union with two members. Check if either of them
+	 * has the reserved name prefix, i.e.:
+	 *
+	 * union {
+	 * 	...
+	 * 	type memberN; // <- type, N = {0,1}
+	 *	...
+	 * };
+	 *
+	 * The member can also be a structure type, in which case we'll
+	 * check the first structure member.
+	 *
+	 * Stop processing after we've seen two members.
+	 */
+	if (is_reserved_member(die)) {
+		check(process_type(state, cache, &type));
+		return RESERVED;
+	}
+
+	if (dwarf_tag(&type) == DW_TAG_structure_type)
+		res = checkp(process_die_container(state, cache, &type,
+						   __process_reserved_struct,
+						   match_member_type));
+	if (res != RESERVED && ++state->reserved.members < 2)
+		return 0; /* Continue */
+
+	return res;
+}
+
+static int process_reserved(struct state *state, struct die *cache,
+			    Dwarf_Die *die)
+{
+	if (!stable)
+		return NOT_RESERVED;
+
+	/*
+	 * To maintain a stable kABI, distributions may choose to reserve
+	 * space in structs for later use by adding placeholder members,
+	 * for example:
+	 *
+	 * struct s {
+	 * 	u64 a;
+	 *	// placeholder
+	 * 	u64 __kabi_reserved_0;
+	 * };
+	 *
+	 * When the reserved member is taken into use, the type change
+	 * would normally cause the symbol version to change as well, but
+	 * if the replacement uses the following convention, gendwarfksyms
+	 * continues to use the placeholder type for versioning instead,
+	 * thus maintaining the same symbol version:
+	 *
+	 * struct s {
+	 * 	u64 a;
+	 *	union {
+	 * 		// replaced member
+	 * 		struct t b;
+	 * 		struct {
+	 * 			// original placeholder
+	 * 			u64 __kabi_reserved_0;
+	 * 		};
+	 * 	};
+	 * };
+	 *
+	 * I.e., as long as the replaced member is in a union, and the
+	 * placeholder has a __kabi_reserved name prefix, we'll continue
+	 * to use the placeholder type (here u64) for version calculation
+	 * instead of the union type.
+	 *
+	 * Note that the user is responsible for ensuring that the
+	 * replacement type is ABI compatible with the placeholder type.
+	 */
+	state->reserved.members = 0;
+
+	return checkp(process_die_container(state, cache, die,
+					    __process_reserved_union,
+					    match_member_type));
+}
+
+static int process_union_type(struct state *state, struct die *cache,
+			      Dwarf_Die *die)
+{
+	if (checkp(process_reserved(state, cache, die)) == RESERVED)
+		return 0;
+
+	return check(__process_structure_type(state, cache, die, "union_type ",
+					      ___process_structure_type,
+					      match_all));
+}
 
 static int process_enumerator_type(struct state *state, struct die *cache,
 				   Dwarf_Die *die)
diff --git a/scripts/gendwarfksyms/examples/reserved.c b/scripts/gendwarfksyms/examples/reserved.c
new file mode 100644
index 000000000000..1e8de7ccd7d2
--- /dev/null
+++ b/scripts/gendwarfksyms/examples/reserved.c
@@ -0,0 +1,66 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 Google LLC
+ *
+ * Reserved data structure field example. See dwarf.c:process_reserved
+ * for details.
+ *
+ * $ gcc -g -c examples/reserved.c
+ *
+ * With --stable, only the reserved field placeholder is used for calculating
+ * symbol versions.
+ *
+ * $ echo exported0 | ./gendwarfksyms --stable --dump-dies reserved.o
+ * variable structure_type struct0 {
+ *   member base_type int byte_size(4) encoding(5) data_member_location(0),
+ *   member base_type long int byte_size(8) encoding(5) data_member_location(8),
+ * } byte_size(16)
+ *
+ * $ echo exported1 | ./gendwarfksyms --stable --dump-dies reserved.o
+ * variable structure_type struct1 {
+ *   member base_type int byte_size(4) encoding(5) data_member_location(0),
+ *   member base_type long int byte_size(8) encoding(5) data_member_location(8),
+ * } byte_size(16)
+ *
+ * $ echo exported2 | ./gendwarfksyms --stable --dump-dies reserved.o
+ * variable structure_type struct2 {
+ *   member base_type int byte_size(4) encoding(5) data_member_location(0),
+ *   member base_type long int byte_size(8) encoding(5) data_member_location(8),
+ * } byte_size(16)
+ */
+
+struct struct0 {
+	int a;
+	union {
+		long __kabi_reserved_0;
+		struct {
+			int b;
+			int c;
+		};
+	};
+};
+
+struct struct1 {
+	int a;
+	union {
+		struct {
+			int b;
+			int c;
+		};
+		long __kabi_reserved_1;
+	};
+};
+
+struct struct2 {
+	int a;
+	union {
+		unsigned long b;
+		struct {
+			long __kabi_reserved_1;
+		};
+	};
+};
+
+struct struct0 exported0;
+struct struct1 exported1;
+struct struct2 exported2;
diff --git a/scripts/gendwarfksyms/gendwarfksyms.h b/scripts/gendwarfksyms/gendwarfksyms.h
index 05b5c01b1c2a..963a07167892 100644
--- a/scripts/gendwarfksyms/gendwarfksyms.h
+++ b/scripts/gendwarfksyms/gendwarfksyms.h
@@ -220,12 +220,27 @@  extern void cache_clear_expanded(struct expansion_cache *ec);
 /*
  * dwarf.c
  */
+
+/* See dwarf.c:process_reserved */
+#define RESERVED_PREFIX "__kabi_reserved"
+#define RESERVED_PREFIX_LEN (sizeof(RESERVED_PREFIX) - 1)
+
 struct expansion_state {
 	bool expand;
 	bool in_pointer_type;
 	unsigned int ptr_expansion_depth;
 };
 
+enum reserved_status {
+	/* >0 to stop DIE processing */
+	NOT_RESERVED = 1,
+	RESERVED
+};
+
+struct reserved_state {
+	int members;
+};
+
 struct state {
 	Dwfl_Module *mod;
 	Dwarf *dbg;
@@ -235,6 +250,9 @@  struct state {
 	/* Structure expansion */
 	struct expansion_state expand;
 	struct expansion_cache expansion_cache;
+
+	/* Reserved members */
+	struct reserved_state reserved;
 };
 
 typedef int (*die_callback_t)(struct state *state, struct die *cache,