Message ID | 20240815173903.4172139-37-samitolvanen@google.com (mailing list archive) |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | Implement DWARF modversions | expand |
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
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
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
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
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
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
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
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
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
> > 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.
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
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
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
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
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
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 --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, ¤t)); while (!res) { - if (match(¤t)) - check(func(state, cache, ¤t)); + if (match(¤t)) { + /* <0 = error, 0 = continue, >0 = stop */ + res = checkp(func(state, cache, ¤t)); + if (res) + return res; + } res = checkp(dwarf_siblingof(¤t, ¤t)); } @@ -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,
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