Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Hannes Frederic Sowa]

On 15.12.2016 15:15, Nicholas Piggin wrote:
> On Thu, 15 Dec 2016 14:15:31 +0100
> Hannes Frederic Sowa  wrote:
> 
>> On 15.12.2016 13:03, Nicholas Piggin wrote:
>>> On Thu, 15 Dec 2016 12:19:02 +0100
>>> Hannes Frederic Sowa  wrote:
>>>   
 On 15.12.2016 03:06, Nicholas Piggin wrote:  
> On Wed, 14 Dec 2016 15:04:36 +0100
> Hannes Frederic Sowa  wrote:
> 
>> On 09.12.2016 17:03, Greg Kroah-Hartman wrote:
>>> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:  
 On Fri, 9 Dec 2016 15:36:04 +0100
 Stanislav Kozina  wrote:
  
> The question is how to provide a similar guarantee if a different 
> way?
 As a tool to aid distro reviewers, modversions has some value, but 
 the
 debug info parsing tools that have been mentioned in this thread 
 seem
 superior (not that I've tested them).
>>> On the other hand the big advantage of modversions is that it also
>>> verifies the checksum during runtime (module loading). In other 
>>> words, I
>>> believe that any other solution should still generate some form of
>>> checksum/watermark which can be easily checked for compatibility on
>>> module load.
>>> It should not be hard to add to the DWARF based tools though. We'd 
>>> just
>>> parse DWARF data instead of the C code.
>> A runtime check is still done, with per-module vermagic which distros
>> can change when they bump the ABI version. Is it really necessary to
>> have more than that (i.e., per-symbol versioning)?
>
>  From my point of view, it is. We need to allow changing ABI for some 
> modules while maintaining it for others.
> In fact I think that there should be version not only for every 
> exported 
> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> (in the sense of eg. structure defined in the public header file).
>   

 Well the distro can just append _v2, _v3 to the name of the function
 or type if it has to break compat for some reason. Would that be 
 enough?  
>>>
>>> There are other ways that distros can work around when upstream "breaks"
>>> the ABI, sometimes they can rename functions, and others they can
>>> "preload" structures with padding in anticipation for when/if fields get
>>> added to them.  But that's all up to the distros, no need for us to
>>> worry about that at all :)  
>>
>> The _v2 and _v3 functions are probably the ones that also get used by
>> future backports in the distro kernel itself and are probably the reason
>> for the ABI change in the first place. Thus going down this route will
>> basically require distros to touch every future backport patch and will
>> in general generate a big mess internally.
>
> What kind of big mess? You have to check the logic of each backport even
> if it does apply cleanly, so the added overhead of the name change should
> be relatively tiny, no?

 Basically single patches are backported in huge series. Reviewing each
 single patch also definitely makes sense, a review of the series as a
 whole is much more worthwhile because it focuses more on logic.

 The patches themselves are checked by individual robots or humans
 against merge conflict introduced mistakes which ring alarm bells for
 people to look more closely during review.

 Merge conflicts introduced mistakes definitely can happen because
 developers/backporters lose the focus from the actual logic but deal
 with shifting lines around or just fixing up postfixes to function names.

 We still try to align the kernel as much as possible with upstream,
 because most developers can't really hold the differences between
 upstream and the internal functions in their heads (is this function RMW
 safe in this version but not that kernel version...).  
>>>
>>> I agree with all this, but in the case of a function rename, you can
>>> automate it all with scripts if that's what you want.
>>>
>>> When you have your list of exported symbols with non-zero version number,
>>> then you can script that __abivXXX into the changeset applying process,
>>> or alternatively apply the rename after your patches are applied, or
>>> use the c preprocessor to define names to something else.  
>>
>> Yes, probably one could come up with coccinelle patches to do this,
>> preprocessing/string matching could have false positives. But as I wrote
>> above, we need one stable ABI and not multiple for our particular
>> kernels, so it seems like a lot of overhead to rename particular
>> functions 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Hannes Frederic Sowa]

On 15.12.2016 15:15, Nicholas Piggin wrote:
> On Thu, 15 Dec 2016 14:15:31 +0100
> Hannes Frederic Sowa  wrote:
> 
>> On 15.12.2016 13:03, Nicholas Piggin wrote:
>>> On Thu, 15 Dec 2016 12:19:02 +0100
>>> Hannes Frederic Sowa  wrote:
>>>   
 On 15.12.2016 03:06, Nicholas Piggin wrote:  
> On Wed, 14 Dec 2016 15:04:36 +0100
> Hannes Frederic Sowa  wrote:
> 
>> On 09.12.2016 17:03, Greg Kroah-Hartman wrote:
>>> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:  
 On Fri, 9 Dec 2016 15:36:04 +0100
 Stanislav Kozina  wrote:
  
> The question is how to provide a similar guarantee if a different 
> way?
 As a tool to aid distro reviewers, modversions has some value, but 
 the
 debug info parsing tools that have been mentioned in this thread 
 seem
 superior (not that I've tested them).
>>> On the other hand the big advantage of modversions is that it also
>>> verifies the checksum during runtime (module loading). In other 
>>> words, I
>>> believe that any other solution should still generate some form of
>>> checksum/watermark which can be easily checked for compatibility on
>>> module load.
>>> It should not be hard to add to the DWARF based tools though. We'd 
>>> just
>>> parse DWARF data instead of the C code.
>> A runtime check is still done, with per-module vermagic which distros
>> can change when they bump the ABI version. Is it really necessary to
>> have more than that (i.e., per-symbol versioning)?
>
>  From my point of view, it is. We need to allow changing ABI for some 
> modules while maintaining it for others.
> In fact I think that there should be version not only for every 
> exported 
> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> (in the sense of eg. structure defined in the public header file).
>   

 Well the distro can just append _v2, _v3 to the name of the function
 or type if it has to break compat for some reason. Would that be 
 enough?  
>>>
>>> There are other ways that distros can work around when upstream "breaks"
>>> the ABI, sometimes they can rename functions, and others they can
>>> "preload" structures with padding in anticipation for when/if fields get
>>> added to them.  But that's all up to the distros, no need for us to
>>> worry about that at all :)  
>>
>> The _v2 and _v3 functions are probably the ones that also get used by
>> future backports in the distro kernel itself and are probably the reason
>> for the ABI change in the first place. Thus going down this route will
>> basically require distros to touch every future backport patch and will
>> in general generate a big mess internally.
>
> What kind of big mess? You have to check the logic of each backport even
> if it does apply cleanly, so the added overhead of the name change should
> be relatively tiny, no?

 Basically single patches are backported in huge series. Reviewing each
 single patch also definitely makes sense, a review of the series as a
 whole is much more worthwhile because it focuses more on logic.

 The patches themselves are checked by individual robots or humans
 against merge conflict introduced mistakes which ring alarm bells for
 people to look more closely during review.

 Merge conflicts introduced mistakes definitely can happen because
 developers/backporters lose the focus from the actual logic but deal
 with shifting lines around or just fixing up postfixes to function names.

 We still try to align the kernel as much as possible with upstream,
 because most developers can't really hold the differences between
 upstream and the internal functions in their heads (is this function RMW
 safe in this version but not that kernel version...).  
>>>
>>> I agree with all this, but in the case of a function rename, you can
>>> automate it all with scripts if that's what you want.
>>>
>>> When you have your list of exported symbols with non-zero version number,
>>> then you can script that __abivXXX into the changeset applying process,
>>> or alternatively apply the rename after your patches are applied, or
>>> use the c preprocessor to define names to something else.  
>>
>> Yes, probably one could come up with coccinelle patches to do this,
>> preprocessing/string matching could have false positives. But as I wrote
>> above, we need one stable ABI and not multiple for our particular
>> kernels, so it seems like a lot of overhead to rename particular
>> functions internally all the time to make them inaccessible for external
>> modules.
> 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Thu, 15 Dec 2016 14:15:31 +0100
Hannes Frederic Sowa  wrote:

> On 15.12.2016 13:03, Nicholas Piggin wrote:
> > On Thu, 15 Dec 2016 12:19:02 +0100
> > Hannes Frederic Sowa  wrote:
> >   
> >> On 15.12.2016 03:06, Nicholas Piggin wrote:  
> >>> On Wed, 14 Dec 2016 15:04:36 +0100
> >>> Hannes Frederic Sowa  wrote:
> >>> 
>  On 09.12.2016 17:03, Greg Kroah-Hartman wrote:
> > On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:  
> >> On Fri, 9 Dec 2016 15:36:04 +0100
> >> Stanislav Kozina  wrote:
> >>  
> >>> The question is how to provide a similar guarantee if a different 
> >>> way?
> >> As a tool to aid distro reviewers, modversions has some value, but 
> >> the
> >> debug info parsing tools that have been mentioned in this thread 
> >> seem
> >> superior (not that I've tested them).
> > On the other hand the big advantage of modversions is that it also
> > verifies the checksum during runtime (module loading). In other 
> > words, I
> > believe that any other solution should still generate some form of
> > checksum/watermark which can be easily checked for compatibility on
> > module load.
> > It should not be hard to add to the DWARF based tools though. We'd 
> > just
> > parse DWARF data instead of the C code.
>  A runtime check is still done, with per-module vermagic which distros
>  can change when they bump the ABI version. Is it really necessary to
>  have more than that (i.e., per-symbol versioning)?
> >>>
> >>>  From my point of view, it is. We need to allow changing ABI for some 
> >>> modules while maintaining it for others.
> >>> In fact I think that there should be version not only for every 
> >>> exported 
> >>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> >>> (in the sense of eg. structure defined in the public header file).
> >>>   
> >>
> >> Well the distro can just append _v2, _v3 to the name of the function
> >> or type if it has to break compat for some reason. Would that be 
> >> enough?  
> >
> > There are other ways that distros can work around when upstream "breaks"
> > the ABI, sometimes they can rename functions, and others they can
> > "preload" structures with padding in anticipation for when/if fields get
> > added to them.  But that's all up to the distros, no need for us to
> > worry about that at all :)  
> 
>  The _v2 and _v3 functions are probably the ones that also get used by
>  future backports in the distro kernel itself and are probably the reason
>  for the ABI change in the first place. Thus going down this route will
>  basically require distros to touch every future backport patch and will
>  in general generate a big mess internally.
> >>>
> >>> What kind of big mess? You have to check the logic of each backport even
> >>> if it does apply cleanly, so the added overhead of the name change should
> >>> be relatively tiny, no?
> >>
> >> Basically single patches are backported in huge series. Reviewing each
> >> single patch also definitely makes sense, a review of the series as a
> >> whole is much more worthwhile because it focuses more on logic.
> >>
> >> The patches themselves are checked by individual robots or humans
> >> against merge conflict introduced mistakes which ring alarm bells for
> >> people to look more closely during review.
> >>
> >> Merge conflicts introduced mistakes definitely can happen because
> >> developers/backporters lose the focus from the actual logic but deal
> >> with shifting lines around or just fixing up postfixes to function names.
> >>
> >> We still try to align the kernel as much as possible with upstream,
> >> because most developers can't really hold the differences between
> >> upstream and the internal functions in their heads (is this function RMW
> >> safe in this version but not that kernel version...).  
> > 
> > I agree with all this, but in the case of a function rename, you can
> > automate it all with scripts if that's what you want.
> > 
> > When you have your list of exported symbols with non-zero version number,
> > then you can script that __abivXXX into the changeset applying process,
> > or alternatively apply the rename after your patches are applied, or
> > use the c preprocessor to define names to something else.  
> 
> Yes, probably one could come up with coccinelle patches to do this,
> preprocessing/string matching could have false positives. But as I wrote
> above, we need one stable ABI and not multiple for our particular
> kernels, so it seems like a lot of overhead to rename particular
> functions internally all the time to make them inaccessible for 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Thu, 15 Dec 2016 14:15:31 +0100
Hannes Frederic Sowa  wrote:

> On 15.12.2016 13:03, Nicholas Piggin wrote:
> > On Thu, 15 Dec 2016 12:19:02 +0100
> > Hannes Frederic Sowa  wrote:
> >   
> >> On 15.12.2016 03:06, Nicholas Piggin wrote:  
> >>> On Wed, 14 Dec 2016 15:04:36 +0100
> >>> Hannes Frederic Sowa  wrote:
> >>> 
>  On 09.12.2016 17:03, Greg Kroah-Hartman wrote:
> > On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:  
> >> On Fri, 9 Dec 2016 15:36:04 +0100
> >> Stanislav Kozina  wrote:
> >>  
> >>> The question is how to provide a similar guarantee if a different 
> >>> way?
> >> As a tool to aid distro reviewers, modversions has some value, but 
> >> the
> >> debug info parsing tools that have been mentioned in this thread 
> >> seem
> >> superior (not that I've tested them).
> > On the other hand the big advantage of modversions is that it also
> > verifies the checksum during runtime (module loading). In other 
> > words, I
> > believe that any other solution should still generate some form of
> > checksum/watermark which can be easily checked for compatibility on
> > module load.
> > It should not be hard to add to the DWARF based tools though. We'd 
> > just
> > parse DWARF data instead of the C code.
>  A runtime check is still done, with per-module vermagic which distros
>  can change when they bump the ABI version. Is it really necessary to
>  have more than that (i.e., per-symbol versioning)?
> >>>
> >>>  From my point of view, it is. We need to allow changing ABI for some 
> >>> modules while maintaining it for others.
> >>> In fact I think that there should be version not only for every 
> >>> exported 
> >>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> >>> (in the sense of eg. structure defined in the public header file).
> >>>   
> >>
> >> Well the distro can just append _v2, _v3 to the name of the function
> >> or type if it has to break compat for some reason. Would that be 
> >> enough?  
> >
> > There are other ways that distros can work around when upstream "breaks"
> > the ABI, sometimes they can rename functions, and others they can
> > "preload" structures with padding in anticipation for when/if fields get
> > added to them.  But that's all up to the distros, no need for us to
> > worry about that at all :)  
> 
>  The _v2 and _v3 functions are probably the ones that also get used by
>  future backports in the distro kernel itself and are probably the reason
>  for the ABI change in the first place. Thus going down this route will
>  basically require distros to touch every future backport patch and will
>  in general generate a big mess internally.
> >>>
> >>> What kind of big mess? You have to check the logic of each backport even
> >>> if it does apply cleanly, so the added overhead of the name change should
> >>> be relatively tiny, no?
> >>
> >> Basically single patches are backported in huge series. Reviewing each
> >> single patch also definitely makes sense, a review of the series as a
> >> whole is much more worthwhile because it focuses more on logic.
> >>
> >> The patches themselves are checked by individual robots or humans
> >> against merge conflict introduced mistakes which ring alarm bells for
> >> people to look more closely during review.
> >>
> >> Merge conflicts introduced mistakes definitely can happen because
> >> developers/backporters lose the focus from the actual logic but deal
> >> with shifting lines around or just fixing up postfixes to function names.
> >>
> >> We still try to align the kernel as much as possible with upstream,
> >> because most developers can't really hold the differences between
> >> upstream and the internal functions in their heads (is this function RMW
> >> safe in this version but not that kernel version...).  
> > 
> > I agree with all this, but in the case of a function rename, you can
> > automate it all with scripts if that's what you want.
> > 
> > When you have your list of exported symbols with non-zero version number,
> > then you can script that __abivXXX into the changeset applying process,
> > or alternatively apply the rename after your patches are applied, or
> > use the c preprocessor to define names to something else.  
> 
> Yes, probably one could come up with coccinelle patches to do this,
> preprocessing/string matching could have false positives. But as I wrote
> above, we need one stable ABI and not multiple for our particular
> kernels, so it seems like a lot of overhead to rename particular
> functions internally all the time to make them inaccessible for external
> modules.

I can't be sure until it's implemented in a workflow 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

Yeah it's great work, so is Stanislav's checker. I wouldn't mind having
a kernel-centric checker tool merged in the kernel if it is small,
maintained, and does a sufficient job for distros.


I'd be very happy to see the resulting tool in the kernel tree, as it 
needs to be kept in sync with any significant changes done to the kernel 
layout in the future.
It's not important if the result is based off libabigail or kabi-dw 
code, I'm sure both can be adjusted to serve the needs. kabi-dw tends to 
be smaller and still rather proof-of-concept, libabigail is definitely 
more mature. The only real difference is C vs. C++ code.



So if I understand where we are, moving the ABI compatibility checking
to one of these tools looks possible. What to do when we have an ABI change
is not settled, but feeding version numbers explicitly into modversions
is an option that would be close to what distros do today.


Sounds great!

Thank you!
-Stanislav

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

Yeah it's great work, so is Stanislav's checker. I wouldn't mind having
a kernel-centric checker tool merged in the kernel if it is small,
maintained, and does a sufficient job for distros.


I'd be very happy to see the resulting tool in the kernel tree, as it 
needs to be kept in sync with any significant changes done to the kernel 
layout in the future.
It's not important if the result is based off libabigail or kabi-dw 
code, I'm sure both can be adjusted to serve the needs. kabi-dw tends to 
be smaller and still rather proof-of-concept, libabigail is definitely 
more mature. The only real difference is C vs. C++ code.



So if I understand where we are, moving the ABI compatibility checking
to one of these tools looks possible. What to do when we have an ABI change
is not settled, but feeding version numbers explicitly into modversions
is an option that would be close to what distros do today.


Sounds great!

Thank you!
-Stanislav

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Hannes Frederic Sowa]

On 15.12.2016 13:03, Nicholas Piggin wrote:
> On Thu, 15 Dec 2016 12:19:02 +0100
> Hannes Frederic Sowa  wrote:
> 
>> On 15.12.2016 03:06, Nicholas Piggin wrote:
>>> On Wed, 14 Dec 2016 15:04:36 +0100
>>> Hannes Frederic Sowa  wrote:
>>>   
 On 09.12.2016 17:03, Greg Kroah-Hartman wrote:  
> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:
>> On Fri, 9 Dec 2016 15:36:04 +0100
>> Stanislav Kozina  wrote:
>>
>>> The question is how to provide a similar guarantee if a different 
>>> way?  
>> As a tool to aid distro reviewers, modversions has some value, but 
>> the
>> debug info parsing tools that have been mentioned in this thread seem
>> superior (not that I've tested them).  
> On the other hand the big advantage of modversions is that it also
> verifies the checksum during runtime (module loading). In other 
> words, I
> believe that any other solution should still generate some form of
> checksum/watermark which can be easily checked for compatibility on
> module load.
> It should not be hard to add to the DWARF based tools though. We'd 
> just
> parse DWARF data instead of the C code.  
 A runtime check is still done, with per-module vermagic which distros
 can change when they bump the ABI version. Is it really necessary to
 have more than that (i.e., per-symbol versioning)?  
>>>
>>>  From my point of view, it is. We need to allow changing ABI for some 
>>> modules while maintaining it for others.
>>> In fact I think that there should be version not only for every 
>>> exported 
>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
>>> (in the sense of eg. structure defined in the public header file).
>>
>> Well the distro can just append _v2, _v3 to the name of the function
>> or type if it has to break compat for some reason. Would that be enough? 
>>
>
> There are other ways that distros can work around when upstream "breaks"
> the ABI, sometimes they can rename functions, and others they can
> "preload" structures with padding in anticipation for when/if fields get
> added to them.  But that's all up to the distros, no need for us to
> worry about that at all :)

 The _v2 and _v3 functions are probably the ones that also get used by
 future backports in the distro kernel itself and are probably the reason
 for the ABI change in the first place. Thus going down this route will
 basically require distros to touch every future backport patch and will
 in general generate a big mess internally.  
>>>
>>> What kind of big mess? You have to check the logic of each backport even
>>> if it does apply cleanly, so the added overhead of the name change should
>>> be relatively tiny, no?  
>>
>> Basically single patches are backported in huge series. Reviewing each
>> single patch also definitely makes sense, a review of the series as a
>> whole is much more worthwhile because it focuses more on logic.
>>
>> The patches themselves are checked by individual robots or humans
>> against merge conflict introduced mistakes which ring alarm bells for
>> people to look more closely during review.
>>
>> Merge conflicts introduced mistakes definitely can happen because
>> developers/backporters lose the focus from the actual logic but deal
>> with shifting lines around or just fixing up postfixes to function names.
>>
>> We still try to align the kernel as much as possible with upstream,
>> because most developers can't really hold the differences between
>> upstream and the internal functions in their heads (is this function RMW
>> safe in this version but not that kernel version...).
> 
> I agree with all this, but in the case of a function rename, you can
> automate it all with scripts if that's what you want.
> 
> When you have your list of exported symbols with non-zero version number,
> then you can script that __abivXXX into the changeset applying process,
> or alternatively apply the rename after your patches are applied, or
> use the c preprocessor to define names to something else.

Yes, probably one could come up with coccinelle patches to do this,
preprocessing/string matching could have false positives. But as I wrote
above, we need one stable ABI and not multiple for our particular
kernels, so it seems like a lot of overhead to rename particular
functions internally all the time to make them inaccessible for external
modules.

>> Anyway, I don't think we will at any time have multiple versions of a
>> function exported to 3rd party kernel modules. The headaches are just
>> too big. Basically we would have to version structs and not functions
>> (this is our bigger problem), thus exporting new versions of functions
>> don't 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Hannes Frederic Sowa]

On 15.12.2016 13:03, Nicholas Piggin wrote:
> On Thu, 15 Dec 2016 12:19:02 +0100
> Hannes Frederic Sowa  wrote:
> 
>> On 15.12.2016 03:06, Nicholas Piggin wrote:
>>> On Wed, 14 Dec 2016 15:04:36 +0100
>>> Hannes Frederic Sowa  wrote:
>>>   
 On 09.12.2016 17:03, Greg Kroah-Hartman wrote:  
> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:
>> On Fri, 9 Dec 2016 15:36:04 +0100
>> Stanislav Kozina  wrote:
>>
>>> The question is how to provide a similar guarantee if a different 
>>> way?  
>> As a tool to aid distro reviewers, modversions has some value, but 
>> the
>> debug info parsing tools that have been mentioned in this thread seem
>> superior (not that I've tested them).  
> On the other hand the big advantage of modversions is that it also
> verifies the checksum during runtime (module loading). In other 
> words, I
> believe that any other solution should still generate some form of
> checksum/watermark which can be easily checked for compatibility on
> module load.
> It should not be hard to add to the DWARF based tools though. We'd 
> just
> parse DWARF data instead of the C code.  
 A runtime check is still done, with per-module vermagic which distros
 can change when they bump the ABI version. Is it really necessary to
 have more than that (i.e., per-symbol versioning)?  
>>>
>>>  From my point of view, it is. We need to allow changing ABI for some 
>>> modules while maintaining it for others.
>>> In fact I think that there should be version not only for every 
>>> exported 
>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
>>> (in the sense of eg. structure defined in the public header file).
>>
>> Well the distro can just append _v2, _v3 to the name of the function
>> or type if it has to break compat for some reason. Would that be enough? 
>>
>
> There are other ways that distros can work around when upstream "breaks"
> the ABI, sometimes they can rename functions, and others they can
> "preload" structures with padding in anticipation for when/if fields get
> added to them.  But that's all up to the distros, no need for us to
> worry about that at all :)

 The _v2 and _v3 functions are probably the ones that also get used by
 future backports in the distro kernel itself and are probably the reason
 for the ABI change in the first place. Thus going down this route will
 basically require distros to touch every future backport patch and will
 in general generate a big mess internally.  
>>>
>>> What kind of big mess? You have to check the logic of each backport even
>>> if it does apply cleanly, so the added overhead of the name change should
>>> be relatively tiny, no?  
>>
>> Basically single patches are backported in huge series. Reviewing each
>> single patch also definitely makes sense, a review of the series as a
>> whole is much more worthwhile because it focuses more on logic.
>>
>> The patches themselves are checked by individual robots or humans
>> against merge conflict introduced mistakes which ring alarm bells for
>> people to look more closely during review.
>>
>> Merge conflicts introduced mistakes definitely can happen because
>> developers/backporters lose the focus from the actual logic but deal
>> with shifting lines around or just fixing up postfixes to function names.
>>
>> We still try to align the kernel as much as possible with upstream,
>> because most developers can't really hold the differences between
>> upstream and the internal functions in their heads (is this function RMW
>> safe in this version but not that kernel version...).
> 
> I agree with all this, but in the case of a function rename, you can
> automate it all with scripts if that's what you want.
> 
> When you have your list of exported symbols with non-zero version number,
> then you can script that __abivXXX into the changeset applying process,
> or alternatively apply the rename after your patches are applied, or
> use the c preprocessor to define names to something else.

Yes, probably one could come up with coccinelle patches to do this,
preprocessing/string matching could have false positives. But as I wrote
above, we need one stable ABI and not multiple for our particular
kernels, so it seems like a lot of overhead to rename particular
functions internally all the time to make them inaccessible for external
modules.

>> Anyway, I don't think we will at any time have multiple versions of a
>> function exported to 3rd party kernel modules. The headaches are just
>> too big. Basically we would have to version structs and not functions
>> (this is our bigger problem), thus exporting new versions of functions
>> don't really help at all. Having multiple versions of structs 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Thu, 15 Dec 2016 12:19:02 +0100
Hannes Frederic Sowa  wrote:

> On 15.12.2016 03:06, Nicholas Piggin wrote:
> > On Wed, 14 Dec 2016 15:04:36 +0100
> > Hannes Frederic Sowa  wrote:
> >   
> >> On 09.12.2016 17:03, Greg Kroah-Hartman wrote:  
> >>> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:
>  On Fri, 9 Dec 2016 15:36:04 +0100
>  Stanislav Kozina  wrote:
> 
> > The question is how to provide a similar guarantee if a different 
> > way?  
>  As a tool to aid distro reviewers, modversions has some value, but 
>  the
>  debug info parsing tools that have been mentioned in this thread seem
>  superior (not that I've tested them).  
> >>> On the other hand the big advantage of modversions is that it also
> >>> verifies the checksum during runtime (module loading). In other 
> >>> words, I
> >>> believe that any other solution should still generate some form of
> >>> checksum/watermark which can be easily checked for compatibility on
> >>> module load.
> >>> It should not be hard to add to the DWARF based tools though. We'd 
> >>> just
> >>> parse DWARF data instead of the C code.  
> >> A runtime check is still done, with per-module vermagic which distros
> >> can change when they bump the ABI version. Is it really necessary to
> >> have more than that (i.e., per-symbol versioning)?  
> >
> >  From my point of view, it is. We need to allow changing ABI for some 
> > modules while maintaining it for others.
> > In fact I think that there should be version not only for every 
> > exported 
> > symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> > (in the sense of eg. structure defined in the public header file).
> 
>  Well the distro can just append _v2, _v3 to the name of the function
>  or type if it has to break compat for some reason. Would that be enough? 
> 
> >>>
> >>> There are other ways that distros can work around when upstream "breaks"
> >>> the ABI, sometimes they can rename functions, and others they can
> >>> "preload" structures with padding in anticipation for when/if fields get
> >>> added to them.  But that's all up to the distros, no need for us to
> >>> worry about that at all :)
> >>
> >> The _v2 and _v3 functions are probably the ones that also get used by
> >> future backports in the distro kernel itself and are probably the reason
> >> for the ABI change in the first place. Thus going down this route will
> >> basically require distros to touch every future backport patch and will
> >> in general generate a big mess internally.  
> > 
> > What kind of big mess? You have to check the logic of each backport even
> > if it does apply cleanly, so the added overhead of the name change should
> > be relatively tiny, no?  
> 
> Basically single patches are backported in huge series. Reviewing each
> single patch also definitely makes sense, a review of the series as a
> whole is much more worthwhile because it focuses more on logic.
> 
> The patches themselves are checked by individual robots or humans
> against merge conflict introduced mistakes which ring alarm bells for
> people to look more closely during review.
> 
> Merge conflicts introduced mistakes definitely can happen because
> developers/backporters lose the focus from the actual logic but deal
> with shifting lines around or just fixing up postfixes to function names.
> 
> We still try to align the kernel as much as possible with upstream,
> because most developers can't really hold the differences between
> upstream and the internal functions in their heads (is this function RMW
> safe in this version but not that kernel version...).

I agree with all this, but in the case of a function rename, you can
automate it all with scripts if that's what you want.

When you have your list of exported symbols with non-zero version number,
then you can script that __abivXXX into the changeset applying process,
or alternatively apply the rename after your patches are applied, or
use the c preprocessor to define names to something else.

> 
> Anyway, I don't think we will at any time have multiple versions of a
> function exported to 3rd party kernel modules. The headaches are just
> too big. Basically we would have to version structs and not functions
> (this is our bigger problem), thus exporting new versions of functions
> don't really help at all. Having multiple versions of structs really
> scares me. ;)
> 
> We already pad structs to allow for additional struct members to be
> added, which helps a lot.
> 
> If versioning of function symbols would be an issue we probably would
> have switched to ELF function versioning (like glibc does it) long time ago.
> 
> >> I think it is important to keep versioning information outside of the
> >> source code. Some 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Thu, 15 Dec 2016 12:19:02 +0100
Hannes Frederic Sowa  wrote:

> On 15.12.2016 03:06, Nicholas Piggin wrote:
> > On Wed, 14 Dec 2016 15:04:36 +0100
> > Hannes Frederic Sowa  wrote:
> >   
> >> On 09.12.2016 17:03, Greg Kroah-Hartman wrote:  
> >>> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:
>  On Fri, 9 Dec 2016 15:36:04 +0100
>  Stanislav Kozina  wrote:
> 
> > The question is how to provide a similar guarantee if a different 
> > way?  
>  As a tool to aid distro reviewers, modversions has some value, but 
>  the
>  debug info parsing tools that have been mentioned in this thread seem
>  superior (not that I've tested them).  
> >>> On the other hand the big advantage of modversions is that it also
> >>> verifies the checksum during runtime (module loading). In other 
> >>> words, I
> >>> believe that any other solution should still generate some form of
> >>> checksum/watermark which can be easily checked for compatibility on
> >>> module load.
> >>> It should not be hard to add to the DWARF based tools though. We'd 
> >>> just
> >>> parse DWARF data instead of the C code.  
> >> A runtime check is still done, with per-module vermagic which distros
> >> can change when they bump the ABI version. Is it really necessary to
> >> have more than that (i.e., per-symbol versioning)?  
> >
> >  From my point of view, it is. We need to allow changing ABI for some 
> > modules while maintaining it for others.
> > In fact I think that there should be version not only for every 
> > exported 
> > symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> > (in the sense of eg. structure defined in the public header file).
> 
>  Well the distro can just append _v2, _v3 to the name of the function
>  or type if it has to break compat for some reason. Would that be enough? 
> 
> >>>
> >>> There are other ways that distros can work around when upstream "breaks"
> >>> the ABI, sometimes they can rename functions, and others they can
> >>> "preload" structures with padding in anticipation for when/if fields get
> >>> added to them.  But that's all up to the distros, no need for us to
> >>> worry about that at all :)
> >>
> >> The _v2 and _v3 functions are probably the ones that also get used by
> >> future backports in the distro kernel itself and are probably the reason
> >> for the ABI change in the first place. Thus going down this route will
> >> basically require distros to touch every future backport patch and will
> >> in general generate a big mess internally.  
> > 
> > What kind of big mess? You have to check the logic of each backport even
> > if it does apply cleanly, so the added overhead of the name change should
> > be relatively tiny, no?  
> 
> Basically single patches are backported in huge series. Reviewing each
> single patch also definitely makes sense, a review of the series as a
> whole is much more worthwhile because it focuses more on logic.
> 
> The patches themselves are checked by individual robots or humans
> against merge conflict introduced mistakes which ring alarm bells for
> people to look more closely during review.
> 
> Merge conflicts introduced mistakes definitely can happen because
> developers/backporters lose the focus from the actual logic but deal
> with shifting lines around or just fixing up postfixes to function names.
> 
> We still try to align the kernel as much as possible with upstream,
> because most developers can't really hold the differences between
> upstream and the internal functions in their heads (is this function RMW
> safe in this version but not that kernel version...).

I agree with all this, but in the case of a function rename, you can
automate it all with scripts if that's what you want.

When you have your list of exported symbols with non-zero version number,
then you can script that __abivXXX into the changeset applying process,
or alternatively apply the rename after your patches are applied, or
use the c preprocessor to define names to something else.

> 
> Anyway, I don't think we will at any time have multiple versions of a
> function exported to 3rd party kernel modules. The headaches are just
> too big. Basically we would have to version structs and not functions
> (this is our bigger problem), thus exporting new versions of functions
> don't really help at all. Having multiple versions of structs really
> scares me. ;)
> 
> We already pad structs to allow for additional struct members to be
> added, which helps a lot.
> 
> If versioning of function symbols would be an issue we probably would
> have switched to ELF function versioning (like glibc does it) long time ago.
> 
> >> I think it is important to keep versioning information outside of the
> >> source code. Some kind of modversions will still be required, but
> >> 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Hannes Frederic Sowa]

On 15.12.2016 03:06, Nicholas Piggin wrote:
> On Wed, 14 Dec 2016 15:04:36 +0100
> Hannes Frederic Sowa  wrote:
> 
>> On 09.12.2016 17:03, Greg Kroah-Hartman wrote:
>>> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:  
 On Fri, 9 Dec 2016 15:36:04 +0100
 Stanislav Kozina  wrote:
  
> The question is how to provide a similar guarantee if a different 
> way?
 As a tool to aid distro reviewers, modversions has some value, but the
 debug info parsing tools that have been mentioned in this thread seem
 superior (not that I've tested them).
>>> On the other hand the big advantage of modversions is that it also
>>> verifies the checksum during runtime (module loading). In other words, I
>>> believe that any other solution should still generate some form of
>>> checksum/watermark which can be easily checked for compatibility on
>>> module load.
>>> It should not be hard to add to the DWARF based tools though. We'd just
>>> parse DWARF data instead of the C code.
>> A runtime check is still done, with per-module vermagic which distros
>> can change when they bump the ABI version. Is it really necessary to
>> have more than that (i.e., per-symbol versioning)?
>
>  From my point of view, it is. We need to allow changing ABI for some 
> modules while maintaining it for others.
> In fact I think that there should be version not only for every exported 
> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> (in the sense of eg. structure defined in the public header file).  

 Well the distro can just append _v2, _v3 to the name of the function
 or type if it has to break compat for some reason. Would that be enough?  
>>>
>>> There are other ways that distros can work around when upstream "breaks"
>>> the ABI, sometimes they can rename functions, and others they can
>>> "preload" structures with padding in anticipation for when/if fields get
>>> added to them.  But that's all up to the distros, no need for us to
>>> worry about that at all :)  
>>
>> The _v2 and _v3 functions are probably the ones that also get used by
>> future backports in the distro kernel itself and are probably the reason
>> for the ABI change in the first place. Thus going down this route will
>> basically require distros to touch every future backport patch and will
>> in general generate a big mess internally.
> 
> What kind of big mess? You have to check the logic of each backport even
> if it does apply cleanly, so the added overhead of the name change should
> be relatively tiny, no?

Basically single patches are backported in huge series. Reviewing each
single patch also definitely makes sense, a review of the series as a
whole is much more worthwhile because it focuses more on logic.

The patches themselves are checked by individual robots or humans
against merge conflict introduced mistakes which ring alarm bells for
people to look more closely during review.

Merge conflicts introduced mistakes definitely can happen because
developers/backporters lose the focus from the actual logic but deal
with shifting lines around or just fixing up postfixes to function names.

We still try to align the kernel as much as possible with upstream,
because most developers can't really hold the differences between
upstream and the internal functions in their heads (is this function RMW
safe in this version but not that kernel version...).

Anyway, I don't think we will at any time have multiple versions of a
function exported to 3rd party kernel modules. The headaches are just
too big. Basically we would have to version structs and not functions
(this is our bigger problem), thus exporting new versions of functions
don't really help at all. Having multiple versions of structs really
scares me. ;)

We already pad structs to allow for additional struct members to be
added, which helps a lot.

If versioning of function symbols would be an issue we probably would
have switched to ELF function versioning (like glibc does it) long time ago.

>> I think it is important to keep versioning information outside of the
>> source code. Some kind of modversions will still be required, but
>> distros should be able to decide if they put in some kind of checksum or
>> a string, what suites them most.
> 
> The module crc symbols are just an integer that requires a match, so it
> could easily be populated by a list that the distro keeps, rather than
> by genksyms. Most of the complexity is on the build side, so that would
> still be an improvement for the kernel. So we *could* do this if the
> distros need it.

Like Don also already said, genksyms already did a pretty good job so
far. We are right now working with Dodji to come up with a way to
replace genksyms, in case people really want to have very specific
control about what causes the symbol version 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Hannes Frederic Sowa]

On 15.12.2016 03:06, Nicholas Piggin wrote:
> On Wed, 14 Dec 2016 15:04:36 +0100
> Hannes Frederic Sowa  wrote:
> 
>> On 09.12.2016 17:03, Greg Kroah-Hartman wrote:
>>> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:  
 On Fri, 9 Dec 2016 15:36:04 +0100
 Stanislav Kozina  wrote:
  
> The question is how to provide a similar guarantee if a different 
> way?
 As a tool to aid distro reviewers, modversions has some value, but the
 debug info parsing tools that have been mentioned in this thread seem
 superior (not that I've tested them).
>>> On the other hand the big advantage of modversions is that it also
>>> verifies the checksum during runtime (module loading). In other words, I
>>> believe that any other solution should still generate some form of
>>> checksum/watermark which can be easily checked for compatibility on
>>> module load.
>>> It should not be hard to add to the DWARF based tools though. We'd just
>>> parse DWARF data instead of the C code.
>> A runtime check is still done, with per-module vermagic which distros
>> can change when they bump the ABI version. Is it really necessary to
>> have more than that (i.e., per-symbol versioning)?
>
>  From my point of view, it is. We need to allow changing ABI for some 
> modules while maintaining it for others.
> In fact I think that there should be version not only for every exported 
> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> (in the sense of eg. structure defined in the public header file).  

 Well the distro can just append _v2, _v3 to the name of the function
 or type if it has to break compat for some reason. Would that be enough?  
>>>
>>> There are other ways that distros can work around when upstream "breaks"
>>> the ABI, sometimes they can rename functions, and others they can
>>> "preload" structures with padding in anticipation for when/if fields get
>>> added to them.  But that's all up to the distros, no need for us to
>>> worry about that at all :)  
>>
>> The _v2 and _v3 functions are probably the ones that also get used by
>> future backports in the distro kernel itself and are probably the reason
>> for the ABI change in the first place. Thus going down this route will
>> basically require distros to touch every future backport patch and will
>> in general generate a big mess internally.
> 
> What kind of big mess? You have to check the logic of each backport even
> if it does apply cleanly, so the added overhead of the name change should
> be relatively tiny, no?

Basically single patches are backported in huge series. Reviewing each
single patch also definitely makes sense, a review of the series as a
whole is much more worthwhile because it focuses more on logic.

The patches themselves are checked by individual robots or humans
against merge conflict introduced mistakes which ring alarm bells for
people to look more closely during review.

Merge conflicts introduced mistakes definitely can happen because
developers/backporters lose the focus from the actual logic but deal
with shifting lines around or just fixing up postfixes to function names.

We still try to align the kernel as much as possible with upstream,
because most developers can't really hold the differences between
upstream and the internal functions in their heads (is this function RMW
safe in this version but not that kernel version...).

Anyway, I don't think we will at any time have multiple versions of a
function exported to 3rd party kernel modules. The headaches are just
too big. Basically we would have to version structs and not functions
(this is our bigger problem), thus exporting new versions of functions
don't really help at all. Having multiple versions of structs really
scares me. ;)

We already pad structs to allow for additional struct members to be
added, which helps a lot.

If versioning of function symbols would be an issue we probably would
have switched to ELF function versioning (like glibc does it) long time ago.

>> I think it is important to keep versioning information outside of the
>> source code. Some kind of modversions will still be required, but
>> distros should be able to decide if they put in some kind of checksum or
>> a string, what suites them most.
> 
> The module crc symbols are just an integer that requires a match, so it
> could easily be populated by a list that the distro keeps, rather than
> by genksyms. Most of the complexity is on the build side, so that would
> still be an improvement for the kernel. So we *could* do this if the
> distros need it.

Like Don also already said, genksyms already did a pretty good job so
far. We are right now working with Dodji to come up with a way to
replace genksyms, in case people really want to have very specific
control about what causes the symbol version to be changed.

Also I wonder what Ben's 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Wed, 14 Dec 2016 15:04:36 +0100
Hannes Frederic Sowa  wrote:

> On 09.12.2016 17:03, Greg Kroah-Hartman wrote:
> > On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:  
> >> On Fri, 9 Dec 2016 15:36:04 +0100
> >> Stanislav Kozina  wrote:
> >>  
> >>> The question is how to provide a similar guarantee if a different 
> >>> way?
> >> As a tool to aid distro reviewers, modversions has some value, but the
> >> debug info parsing tools that have been mentioned in this thread seem
> >> superior (not that I've tested them).
> > On the other hand the big advantage of modversions is that it also
> > verifies the checksum during runtime (module loading). In other words, I
> > believe that any other solution should still generate some form of
> > checksum/watermark which can be easily checked for compatibility on
> > module load.
> > It should not be hard to add to the DWARF based tools though. We'd just
> > parse DWARF data instead of the C code.
>  A runtime check is still done, with per-module vermagic which distros
>  can change when they bump the ABI version. Is it really necessary to
>  have more than that (i.e., per-symbol versioning)?
> >>>
> >>>  From my point of view, it is. We need to allow changing ABI for some 
> >>> modules while maintaining it for others.
> >>> In fact I think that there should be version not only for every exported 
> >>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> >>> (in the sense of eg. structure defined in the public header file).  
> >>
> >> Well the distro can just append _v2, _v3 to the name of the function
> >> or type if it has to break compat for some reason. Would that be enough?  
> > 
> > There are other ways that distros can work around when upstream "breaks"
> > the ABI, sometimes they can rename functions, and others they can
> > "preload" structures with padding in anticipation for when/if fields get
> > added to them.  But that's all up to the distros, no need for us to
> > worry about that at all :)  
> 
> The _v2 and _v3 functions are probably the ones that also get used by
> future backports in the distro kernel itself and are probably the reason
> for the ABI change in the first place. Thus going down this route will
> basically require distros to touch every future backport patch and will
> in general generate a big mess internally.

What kind of big mess? You have to check the logic of each backport even
if it does apply cleanly, so the added overhead of the name change should
be relatively tiny, no?

> 
> I think it is important to keep versioning information outside of the
> source code. Some kind of modversions will still be required, but
> distros should be able to decide if they put in some kind of checksum or
> a string, what suites them most.

The module crc symbols are just an integer that requires a match, so it
could easily be populated by a list that the distro keeps, rather than
by genksyms. Most of the complexity is on the build side, so that would
still be an improvement for the kernel. So we *could* do this if the
distros need it.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Wed, 14 Dec 2016 15:04:36 +0100
Hannes Frederic Sowa  wrote:

> On 09.12.2016 17:03, Greg Kroah-Hartman wrote:
> > On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:  
> >> On Fri, 9 Dec 2016 15:36:04 +0100
> >> Stanislav Kozina  wrote:
> >>  
> >>> The question is how to provide a similar guarantee if a different 
> >>> way?
> >> As a tool to aid distro reviewers, modversions has some value, but the
> >> debug info parsing tools that have been mentioned in this thread seem
> >> superior (not that I've tested them).
> > On the other hand the big advantage of modversions is that it also
> > verifies the checksum during runtime (module loading). In other words, I
> > believe that any other solution should still generate some form of
> > checksum/watermark which can be easily checked for compatibility on
> > module load.
> > It should not be hard to add to the DWARF based tools though. We'd just
> > parse DWARF data instead of the C code.
>  A runtime check is still done, with per-module vermagic which distros
>  can change when they bump the ABI version. Is it really necessary to
>  have more than that (i.e., per-symbol versioning)?
> >>>
> >>>  From my point of view, it is. We need to allow changing ABI for some 
> >>> modules while maintaining it for others.
> >>> In fact I think that there should be version not only for every exported 
> >>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> >>> (in the sense of eg. structure defined in the public header file).  
> >>
> >> Well the distro can just append _v2, _v3 to the name of the function
> >> or type if it has to break compat for some reason. Would that be enough?  
> > 
> > There are other ways that distros can work around when upstream "breaks"
> > the ABI, sometimes they can rename functions, and others they can
> > "preload" structures with padding in anticipation for when/if fields get
> > added to them.  But that's all up to the distros, no need for us to
> > worry about that at all :)  
> 
> The _v2 and _v3 functions are probably the ones that also get used by
> future backports in the distro kernel itself and are probably the reason
> for the ABI change in the first place. Thus going down this route will
> basically require distros to touch every future backport patch and will
> in general generate a big mess internally.

What kind of big mess? You have to check the logic of each backport even
if it does apply cleanly, so the added overhead of the name change should
be relatively tiny, no?

> 
> I think it is important to keep versioning information outside of the
> source code. Some kind of modversions will still be required, but
> distros should be able to decide if they put in some kind of checksum or
> a string, what suites them most.

The module crc symbols are just an integer that requires a match, so it
could easily be populated by a list that the distro keeps, rather than
by genksyms. Most of the complexity is on the build side, so that would
still be an improvement for the kernel. So we *could* do this if the
distros need it.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Don Zickus]

On Sat, Dec 10, 2016 at 01:41:03PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote:
> > Hello,
> > 
> > Nicholas Piggin  a �crit:
> > 
> > [...]
> > 
> > > That said, a dwarf based checker tool should be able to do as good a job
> > > (maybe a bit better because report is very informative and it may pick up
> > > compiler alignments or padding options).
> > 
> > So, Nicholas was kind enough to send me the two Linux Kernel binaries
> > that he built with the tiny little interface change that we were
> > discussing earlier.  Here is what the abidiff[1] tools says about that
> > interface change:
> > 
> > $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi 
> > vmlinux.abi2.abi
> > Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> > 
> > 1 function with some indirect sub-type change:
> > 
> >   [C]'function int foo(blah*)' at memory.c:82:1 has some indirect 
> > sub-type changes:
> > parameter 1 of type 'blah*' has sub-type changes:
> >   in pointed to type 'struct blah' at memory.c:78:1:
> > type size changed from 32 to 64 bits
> > 1 data member insertion:
> >   'int blah::y', at offset 0 (in bits) at memory.c:79:1
> > 1 data member change:
> >  'int blah::x' offset changed from 0 to 32 (in bits) (by +32 
> > bits)
> > 
> > 
> > 
> > real0m2.595s
> > user0m2.489s
> > sys 0m0.108s
> > $ 
> > 
> > I kept the timing information to give you an idea of the time it takes
> > on a non-optimized build of abidiff.
> > 
> > One could for instance want that types that are not defined in header
> > files be kept out of the change report.  In that case it's possible to
> > write a little suppression specification file like this one:
> > 
> > $ cat vmlinux.abignore 
> > [suppress_type]
> >   source_location_not_regexp = .*\\.h
> > $
> > 
> > You can then pass that suppression file to the tool:
> > 
> > $ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr 
> > vmlinux.abignore vmlinux.abi1.abi vmlinux.abi2.abi
> > Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 
> > Added function
> > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> > 
> > 
> > real0m2.574s
> > user0m2.473s
> > sys 0m0.102s
> > $
> > 
> > So this is the kind of interface change analysis tool we are working on
> > at the moment.
> > 
> > One could also imagine a tool that would compute a CRC that takes the
> > very same suppression specification files into account, letting people
> > to decide that some interface changes are OK.  That CRC would thus be
> > added to the special ELF sections we already have today.  We could keep
> > the modversion machinery, but with a greater dose of flexibility.
> > Whenever modversion detects a change, abidiff would tell people what the
> > change is exactly.
> > 
> > What do you guys think?
> 
> YES YES YES!!!
> 
> Now I don't work on a distro anymore, but I would think that something
> like this would be really useful, pointing out exactly what changed is
> very important for distro maintainers to determine what they want to do
> (either fix up the abi change with strange hacks, or ignore it due to
> the change being in an area they don't care at all about, i.e. a random
> driver subsystem.)

Well, genksyms does provide this today with the .symref files.  It may not
be as thorough and flexible as libabigail, but RH has been using it for
years to quickly determine what patches broke the abi and more importantly
where (which can be challenging).  I just didn't want to downplay what is
available today.

On the flip side, I do like what libabigail has to offer.  There seems to be
some interesting new ways of handling our abi and I look forward to our kabi
team putting it to use. :-)

> 
> So yes, I think this is really good stuff.  But if the distro
> maintainers correct me and think it's useless, then I need to revisit my
> view of exactly what they do for their customers :)

I also don't want folks to forget that are two parts to this equation.  The
checking above is the first part.  But the second part is what to do about
the stuff you ignored, which leads to the run time checks.

If you don't maintain 100% abi (which RH doesn't), then we need a way to
block drivers from loading that use symbols which we do not maintain and
broke.  The crc checks at load time work great for this.  Hopefully we can
continue to support what modversions is providing today (or something
similar).

I do not think vermagic will be usable at all for us.

Thanks!

Cheers,
Don

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Don Zickus]

On Sat, Dec 10, 2016 at 01:41:03PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote:
> > Hello,
> > 
> > Nicholas Piggin  a �crit:
> > 
> > [...]
> > 
> > > That said, a dwarf based checker tool should be able to do as good a job
> > > (maybe a bit better because report is very informative and it may pick up
> > > compiler alignments or padding options).
> > 
> > So, Nicholas was kind enough to send me the two Linux Kernel binaries
> > that he built with the tiny little interface change that we were
> > discussing earlier.  Here is what the abidiff[1] tools says about that
> > interface change:
> > 
> > $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi 
> > vmlinux.abi2.abi
> > Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> > 
> > 1 function with some indirect sub-type change:
> > 
> >   [C]'function int foo(blah*)' at memory.c:82:1 has some indirect 
> > sub-type changes:
> > parameter 1 of type 'blah*' has sub-type changes:
> >   in pointed to type 'struct blah' at memory.c:78:1:
> > type size changed from 32 to 64 bits
> > 1 data member insertion:
> >   'int blah::y', at offset 0 (in bits) at memory.c:79:1
> > 1 data member change:
> >  'int blah::x' offset changed from 0 to 32 (in bits) (by +32 
> > bits)
> > 
> > 
> > 
> > real0m2.595s
> > user0m2.489s
> > sys 0m0.108s
> > $ 
> > 
> > I kept the timing information to give you an idea of the time it takes
> > on a non-optimized build of abidiff.
> > 
> > One could for instance want that types that are not defined in header
> > files be kept out of the change report.  In that case it's possible to
> > write a little suppression specification file like this one:
> > 
> > $ cat vmlinux.abignore 
> > [suppress_type]
> >   source_location_not_regexp = .*\\.h
> > $
> > 
> > You can then pass that suppression file to the tool:
> > 
> > $ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr 
> > vmlinux.abignore vmlinux.abi1.abi vmlinux.abi2.abi
> > Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 
> > Added function
> > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> > 
> > 
> > real0m2.574s
> > user0m2.473s
> > sys 0m0.102s
> > $
> > 
> > So this is the kind of interface change analysis tool we are working on
> > at the moment.
> > 
> > One could also imagine a tool that would compute a CRC that takes the
> > very same suppression specification files into account, letting people
> > to decide that some interface changes are OK.  That CRC would thus be
> > added to the special ELF sections we already have today.  We could keep
> > the modversion machinery, but with a greater dose of flexibility.
> > Whenever modversion detects a change, abidiff would tell people what the
> > change is exactly.
> > 
> > What do you guys think?
> 
> YES YES YES!!!
> 
> Now I don't work on a distro anymore, but I would think that something
> like this would be really useful, pointing out exactly what changed is
> very important for distro maintainers to determine what they want to do
> (either fix up the abi change with strange hacks, or ignore it due to
> the change being in an area they don't care at all about, i.e. a random
> driver subsystem.)

Well, genksyms does provide this today with the .symref files.  It may not
be as thorough and flexible as libabigail, but RH has been using it for
years to quickly determine what patches broke the abi and more importantly
where (which can be challenging).  I just didn't want to downplay what is
available today.

On the flip side, I do like what libabigail has to offer.  There seems to be
some interesting new ways of handling our abi and I look forward to our kabi
team putting it to use. :-)

> 
> So yes, I think this is really good stuff.  But if the distro
> maintainers correct me and think it's useless, then I need to revisit my
> view of exactly what they do for their customers :)

I also don't want folks to forget that are two parts to this equation.  The
checking above is the first part.  But the second part is what to do about
the stuff you ignored, which leads to the run time checks.

If you don't maintain 100% abi (which RH doesn't), then we need a way to
block drivers from loading that use symbols which we do not maintain and
broke.  The crc checks at load time work great for this.  Hopefully we can
continue to support what modversions is providing today (or something
similar).

I do not think vermagic will be usable at all for us.

Thanks!

Cheers,
Don

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Hannes Frederic Sowa]

On 09.12.2016 17:03, Greg Kroah-Hartman wrote:
> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:
>> On Fri, 9 Dec 2016 15:36:04 +0100
>> Stanislav Kozina  wrote:
>>
>>> The question is how to provide a similar guarantee if a different way?  
>> As a tool to aid distro reviewers, modversions has some value, but the
>> debug info parsing tools that have been mentioned in this thread seem
>> superior (not that I've tested them).  
> On the other hand the big advantage of modversions is that it also
> verifies the checksum during runtime (module loading). In other words, I
> believe that any other solution should still generate some form of
> checksum/watermark which can be easily checked for compatibility on
> module load.
> It should not be hard to add to the DWARF based tools though. We'd just
> parse DWARF data instead of the C code.  
 A runtime check is still done, with per-module vermagic which distros
 can change when they bump the ABI version. Is it really necessary to
 have more than that (i.e., per-symbol versioning)?  
>>>
>>>  From my point of view, it is. We need to allow changing ABI for some 
>>> modules while maintaining it for others.
>>> In fact I think that there should be version not only for every exported 
>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
>>> (in the sense of eg. structure defined in the public header file).
>>
>> Well the distro can just append _v2, _v3 to the name of the function
>> or type if it has to break compat for some reason. Would that be enough?
> 
> There are other ways that distros can work around when upstream "breaks"
> the ABI, sometimes they can rename functions, and others they can
> "preload" structures with padding in anticipation for when/if fields get
> added to them.  But that's all up to the distros, no need for us to
> worry about that at all :)

The _v2 and _v3 functions are probably the ones that also get used by
future backports in the distro kernel itself and are probably the reason
for the ABI change in the first place. Thus going down this route will
basically require distros to touch every future backport patch and will
in general generate a big mess internally.

I think it is important to keep versioning information outside of the
source code. Some kind of modversions will still be required, but
distros should be able to decide if they put in some kind of checksum or
a string, what suites them most.

Thanks,
Hannes (who is still impressed by the genksyms tools)

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Hannes Frederic Sowa]

On 09.12.2016 17:03, Greg Kroah-Hartman wrote:
> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:
>> On Fri, 9 Dec 2016 15:36:04 +0100
>> Stanislav Kozina  wrote:
>>
>>> The question is how to provide a similar guarantee if a different way?  
>> As a tool to aid distro reviewers, modversions has some value, but the
>> debug info parsing tools that have been mentioned in this thread seem
>> superior (not that I've tested them).  
> On the other hand the big advantage of modversions is that it also
> verifies the checksum during runtime (module loading). In other words, I
> believe that any other solution should still generate some form of
> checksum/watermark which can be easily checked for compatibility on
> module load.
> It should not be hard to add to the DWARF based tools though. We'd just
> parse DWARF data instead of the C code.  
 A runtime check is still done, with per-module vermagic which distros
 can change when they bump the ABI version. Is it really necessary to
 have more than that (i.e., per-symbol versioning)?  
>>>
>>>  From my point of view, it is. We need to allow changing ABI for some 
>>> modules while maintaining it for others.
>>> In fact I think that there should be version not only for every exported 
>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
>>> (in the sense of eg. structure defined in the public header file).
>>
>> Well the distro can just append _v2, _v3 to the name of the function
>> or type if it has to break compat for some reason. Would that be enough?
> 
> There are other ways that distros can work around when upstream "breaks"
> the ABI, sometimes they can rename functions, and others they can
> "preload" structures with padding in anticipation for when/if fields get
> added to them.  But that's all up to the distros, no need for us to
> worry about that at all :)

The _v2 and _v3 functions are probably the ones that also get used by
future backports in the distro kernel itself and are probably the reason
for the ABI change in the first place. Thus going down this route will
basically require distros to touch every future backport patch and will
in general generate a big mess internally.

I think it is important to keep versioning information outside of the
source code. Some kind of modversions will still be required, but
distros should be able to decide if they put in some kind of checksum or
a string, what suites them most.

Thanks,
Hannes (who is still impressed by the genksyms tools)

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-14 10:15, Michal Marek wrote:
> A minimal example would be
> 
> t1.c:
> struct s1;
> struct s2 {
>   int i;
> }
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> }
> void foo(struct s3*);
> EXPORT_SYMBOL(foo);
> 
> t2.c:
> struct s1 {
>   int j;
> }
> struct s2;
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> }
> void foo(struct s3*);
> EXPORT_SYMBOL(foo);

Note that the above, if passed to genksyms verbatim, would result in
genksyms treating all the types as internal. Here is a complete
example including linemarkers:

$ cat t1.i
# 1 "t1.c"
# 1 ""
# 1 ""
# 1 "t1.c"
# 1 "t1.h" 1
# 1 "t.h" 1
struct s1;
struct s2;
struct s3 {
 struct s1 *ptr1;
 struct s2 *ptr2;
};
# 2 "t1.h" 2
struct s2 {
 int i;
};
# 2 "t1.c" 2
void foo(struct s3 *s) { }
EXPORT_SYMBOL(foo);

$ cat t2.i
# 1 "t2.c"
# 1 ""
# 1 ""
# 1 "t2.c"
# 1 "t2.h" 1
# 1 "t.h" 1
struct s1;
struct s2;
struct s3 {
 struct s1 *ptr1;
 struct s2 *ptr2;
};
# 2 "t2.h" 2
struct s1 {
 int j;
};
# 2 "t2.c" 2
void foo(struct s3 *s) { }
EXPORT_SYMBOL(foo);

$ ./scripts/genksyms/genksyms -D 
__crc_foo = 0xf731cef8 ;

$ ./scripts/genksyms/genksyms -D 
__crc_foo = 0xc925dae5 ;

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-14 10:15, Michal Marek wrote:
> A minimal example would be
> 
> t1.c:
> struct s1;
> struct s2 {
>   int i;
> }
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> }
> void foo(struct s3*);
> EXPORT_SYMBOL(foo);
> 
> t2.c:
> struct s1 {
>   int j;
> }
> struct s2;
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> }
> void foo(struct s3*);
> EXPORT_SYMBOL(foo);

Note that the above, if passed to genksyms verbatim, would result in
genksyms treating all the types as internal. Here is a complete
example including linemarkers:

$ cat t1.i
# 1 "t1.c"
# 1 ""
# 1 ""
# 1 "t1.c"
# 1 "t1.h" 1
# 1 "t.h" 1
struct s1;
struct s2;
struct s3 {
 struct s1 *ptr1;
 struct s2 *ptr2;
};
# 2 "t1.h" 2
struct s2 {
 int i;
};
# 2 "t1.c" 2
void foo(struct s3 *s) { }
EXPORT_SYMBOL(foo);

$ cat t2.i
# 1 "t2.c"
# 1 ""
# 1 ""
# 1 "t2.c"
# 1 "t2.h" 1
# 1 "t.h" 1
struct s1;
struct s2;
struct s3 {
 struct s1 *ptr1;
 struct s2 *ptr2;
};
# 2 "t2.h" 2
struct s1 {
 int j;
};
# 2 "t2.c" 2
void foo(struct s3 *s) { }
EXPORT_SYMBOL(foo);

$ ./scripts/genksyms/genksyms -D 
__crc_foo = 0xf731cef8 ;

$ ./scripts/genksyms/genksyms -D 
__crc_foo = 0xc925dae5 ;

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-14 11:02, Dodji Seketeli wrote:
> Michal Marek  a écrit:
> 
>>> Libabigail does a "whole binary" analysis of types.
>>>
>>> So, consider the point of use of the type 'struct s1*'.  Even if 'struct
>>> s' is just forward-declared at that point, the declaration of struct s1
>>> is "resolved" to its definition.  Even if the definition comes later in
>>> the binary.
>>
>> But there isn't any definition of struct s1 in t1.o. Does abidiff
>> "steal" the definition from the other object file? That would be
>> legitimate, I'm just curious.
> 
> If there is another translation unit in the *same* binary that defines
> struct s1, then yes, it's "stolen", as you say.
> 
> But if in the entire binary, struct s1 is just declared (not defined),
> then it'll compare equal to any struct s1 that is defined in the
> *second* binary.

That makes sense, thanks.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-14 11:02, Dodji Seketeli wrote:
> Michal Marek  a écrit:
> 
>>> Libabigail does a "whole binary" analysis of types.
>>>
>>> So, consider the point of use of the type 'struct s1*'.  Even if 'struct
>>> s' is just forward-declared at that point, the declaration of struct s1
>>> is "resolved" to its definition.  Even if the definition comes later in
>>> the binary.
>>
>> But there isn't any definition of struct s1 in t1.o. Does abidiff
>> "steal" the definition from the other object file? That would be
>> legitimate, I'm just curious.
> 
> If there is another translation unit in the *same* binary that defines
> struct s1, then yes, it's "stolen", as you say.
> 
> But if in the entire binary, struct s1 is just declared (not defined),
> then it'll compare equal to any struct s1 that is defined in the
> *second* binary.

That makes sense, thanks.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Michal Marek  a écrit:

>> Libabigail does a "whole binary" analysis of types.
>> 
>> So, consider the point of use of the type 'struct s1*'.  Even if 'struct
>> s' is just forward-declared at that point, the declaration of struct s1
>> is "resolved" to its definition.  Even if the definition comes later in
>> the binary.
>
> But there isn't any definition of struct s1 in t1.o. Does abidiff
> "steal" the definition from the other object file? That would be
> legitimate, I'm just curious.

If there is another translation unit in the *same* binary that defines
struct s1, then yes, it's "stolen", as you say.

But if in the entire binary, struct s1 is just declared (not defined),
then it'll compare equal to any struct s1 that is defined in the
*second* binary.

Cheers,

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Michal Marek  a écrit:

>> Libabigail does a "whole binary" analysis of types.
>> 
>> So, consider the point of use of the type 'struct s1*'.  Even if 'struct
>> s' is just forward-declared at that point, the declaration of struct s1
>> is "resolved" to its definition.  Even if the definition comes later in
>> the binary.
>
> But there isn't any definition of struct s1 in t1.o. Does abidiff
> "steal" the definition from the other object file? That would be
> legitimate, I'm just curious.

If there is another translation unit in the *same* binary that defines
struct s1, then yes, it's "stolen", as you say.

But if in the entire binary, struct s1 is just declared (not defined),
then it'll compare equal to any struct s1 that is defined in the
*second* binary.

Cheers,

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Dodji Seketeli  a écrit:

Grr, I did paste the wrong content of t1.c and t2.c in my last message sorry.

Here are the correct ones:

$ cat t1.c
struct s1;
struct s2 {
int i;
};
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
};

void foo(struct s3* s __attribute__((unused)))
{
}

$ cat t2.c
struct s1 {
int j;
};
struct s2;
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
};

void foo(struct s3* s __attribute__((unused)))
{
}

$ gcc -g -c t1.c
$ gcc -g -c t2.c
$ abidiff t1.o t2.o
$ 

The rest of my previous message still applies :-)

> So, as you see here, abidiff considers t1.o and t2.o has having the same
> ABI, so it considers the two foo functions to be equivalent.
>
>> The types are the same, but their visibility in the different
>> compilation units differs.
>
> I see, for genksyms, the order of declarations matters, especially when
> forward declarations are involved.
>
> Libabigail does a "whole binary" analysis of types.
>
> So, consider the point of use of the type 'struct s1*'.  Even if 'struct
> s' is just forward-declared at that point, the declaration of struct s1
> is "resolved" to its definition.  Even if the definition comes later in
> the binary.
>
> In other words, if struct s1 is defined in the binary, you'll never have
> that "struct s1 {UNKNOWN} *ptr1;" that you see in genksyms's
> representation.

Thanks.

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Dodji Seketeli  a écrit:

Grr, I did paste the wrong content of t1.c and t2.c in my last message sorry.

Here are the correct ones:

$ cat t1.c
struct s1;
struct s2 {
int i;
};
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
};

void foo(struct s3* s __attribute__((unused)))
{
}

$ cat t2.c
struct s1 {
int j;
};
struct s2;
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
};

void foo(struct s3* s __attribute__((unused)))
{
}

$ gcc -g -c t1.c
$ gcc -g -c t2.c
$ abidiff t1.o t2.o
$ 

The rest of my previous message still applies :-)

> So, as you see here, abidiff considers t1.o and t2.o has having the same
> ABI, so it considers the two foo functions to be equivalent.
>
>> The types are the same, but their visibility in the different
>> compilation units differs.
>
> I see, for genksyms, the order of declarations matters, especially when
> forward declarations are involved.
>
> Libabigail does a "whole binary" analysis of types.
>
> So, consider the point of use of the type 'struct s1*'.  Even if 'struct
> s' is just forward-declared at that point, the declaration of struct s1
> is "resolved" to its definition.  Even if the definition comes later in
> the binary.
>
> In other words, if struct s1 is defined in the binary, you'll never have
> that "struct s1 {UNKNOWN} *ptr1;" that you see in genksyms's
> representation.

Thanks.

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-14 10:36, Dodji Seketeli wrote:
> Michal Marek  a écrit:
> 
> [...]
> 
>> A minimal example would be
>>
>> t1.c:
>> struct s1;
>> struct s2 {
>>  int i;
>> }
>> struct s3 {
>>  struct s1 *ptr1;
>>  struct s2 *ptr2;
>> }
>> void foo(struct s3*);
>> EXPORT_SYMBOL(foo);
>>
>> t2.c:
>> struct s1 {
>>  int j;
>> }
>> struct s2;
>> struct s3 {
>>  struct s1 *ptr1;
>>  struct s2 *ptr2;
>> }
>> void foo(struct s3*);
>> EXPORT_SYMBOL(foo);
>>
>> genksyms expands this to
>> void foo ( struct s3 { struct s1 { UNKNOWN } * ptr1 ; struct s2 { int i ; } 
>> * ptr2 ; } * )
>>
>> or
>>
>> void foo ( struct s3 { struct s1 { int j ; } * ptr1 ; struct s2 { UNKNOWN } 
>> * ptr2 ; } * )
>> respectively.
> 
> Thanks, I have built an independant test case from this:
> 
> $ cat t1.c
> struct s1;
> struct s2 {
>   int i;
> };
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> };
> void foo(struct s3*);
> $ cat t2.c
> struct s1 {
>   int j;
> };
> struct s2;
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> };
> void foo(struct s3*);
> $ gcc -g -c t1.c
> $ gcc -g -c t2.c
> $ abidiff t1.o t2.o
> $ 
> 
> So, as you see here, abidiff considers t1.o and t2.o has having the same
> ABI, so it considers the two foo functions to be equivalent.

Wow. That sounds too good to be true.


>> The types are the same, but their visibility in the different
>> compilation units differs.
> 
> I see, for genksyms, the order of declarations matters, especially when
> forward declarations are involved.
> 
> Libabigail does a "whole binary" analysis of types.
> 
> So, consider the point of use of the type 'struct s1*'.  Even if 'struct
> s' is just forward-declared at that point, the declaration of struct s1
> is "resolved" to its definition.  Even if the definition comes later in
> the binary.

But there isn't any definition of struct s1 in t1.o. Does abidiff
"steal" the definition from the other object file? That would be
legitimate, I'm just curious.

Thanks,
Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-14 10:36, Dodji Seketeli wrote:
> Michal Marek  a écrit:
> 
> [...]
> 
>> A minimal example would be
>>
>> t1.c:
>> struct s1;
>> struct s2 {
>>  int i;
>> }
>> struct s3 {
>>  struct s1 *ptr1;
>>  struct s2 *ptr2;
>> }
>> void foo(struct s3*);
>> EXPORT_SYMBOL(foo);
>>
>> t2.c:
>> struct s1 {
>>  int j;
>> }
>> struct s2;
>> struct s3 {
>>  struct s1 *ptr1;
>>  struct s2 *ptr2;
>> }
>> void foo(struct s3*);
>> EXPORT_SYMBOL(foo);
>>
>> genksyms expands this to
>> void foo ( struct s3 { struct s1 { UNKNOWN } * ptr1 ; struct s2 { int i ; } 
>> * ptr2 ; } * )
>>
>> or
>>
>> void foo ( struct s3 { struct s1 { int j ; } * ptr1 ; struct s2 { UNKNOWN } 
>> * ptr2 ; } * )
>> respectively.
> 
> Thanks, I have built an independant test case from this:
> 
> $ cat t1.c
> struct s1;
> struct s2 {
>   int i;
> };
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> };
> void foo(struct s3*);
> $ cat t2.c
> struct s1 {
>   int j;
> };
> struct s2;
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> };
> void foo(struct s3*);
> $ gcc -g -c t1.c
> $ gcc -g -c t2.c
> $ abidiff t1.o t2.o
> $ 
> 
> So, as you see here, abidiff considers t1.o and t2.o has having the same
> ABI, so it considers the two foo functions to be equivalent.

Wow. That sounds too good to be true.


>> The types are the same, but their visibility in the different
>> compilation units differs.
> 
> I see, for genksyms, the order of declarations matters, especially when
> forward declarations are involved.
> 
> Libabigail does a "whole binary" analysis of types.
> 
> So, consider the point of use of the type 'struct s1*'.  Even if 'struct
> s' is just forward-declared at that point, the declaration of struct s1
> is "resolved" to its definition.  Even if the definition comes later in
> the binary.

But there isn't any definition of struct s1 in t1.o. Does abidiff
"steal" the definition from the other object file? That would be
legitimate, I'm just curious.

Thanks,
Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Michal Marek  a écrit:

[...]

> A minimal example would be
>
> t1.c:
> struct s1;
> struct s2 {
>   int i;
> }
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> }
> void foo(struct s3*);
> EXPORT_SYMBOL(foo);
>
> t2.c:
> struct s1 {
>   int j;
> }
> struct s2;
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> }
> void foo(struct s3*);
> EXPORT_SYMBOL(foo);
>
> genksyms expands this to
> void foo ( struct s3 { struct s1 { UNKNOWN } * ptr1 ; struct s2 { int i ; } * 
> ptr2 ; } * )
>
> or
>
> void foo ( struct s3 { struct s1 { int j ; } * ptr1 ; struct s2 { UNKNOWN } * 
> ptr2 ; } * )
> respectively.

Thanks, I have built an independant test case from this:

$ cat t1.c
struct s1;
struct s2 {
int i;
};
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
};
void foo(struct s3*);
$ cat t2.c
struct s1 {
int j;
};
struct s2;
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
};
void foo(struct s3*);
$ gcc -g -c t1.c
$ gcc -g -c t2.c
$ abidiff t1.o t2.o
$ 

So, as you see here, abidiff considers t1.o and t2.o has having the same
ABI, so it considers the two foo functions to be equivalent.

> The types are the same, but their visibility in the different
> compilation units differs.

I see, for genksyms, the order of declarations matters, especially when
forward declarations are involved.

Libabigail does a "whole binary" analysis of types.

So, consider the point of use of the type 'struct s1*'.  Even if 'struct
s' is just forward-declared at that point, the declaration of struct s1
is "resolved" to its definition.  Even if the definition comes later in
the binary.

In other words, if struct s1 is defined in the binary, you'll never have
that "struct s1 {UNKNOWN} *ptr1;" that you see in genksyms's
representation.

Cheers,

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Michal Marek  a écrit:

[...]

> A minimal example would be
>
> t1.c:
> struct s1;
> struct s2 {
>   int i;
> }
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> }
> void foo(struct s3*);
> EXPORT_SYMBOL(foo);
>
> t2.c:
> struct s1 {
>   int j;
> }
> struct s2;
> struct s3 {
>   struct s1 *ptr1;
>   struct s2 *ptr2;
> }
> void foo(struct s3*);
> EXPORT_SYMBOL(foo);
>
> genksyms expands this to
> void foo ( struct s3 { struct s1 { UNKNOWN } * ptr1 ; struct s2 { int i ; } * 
> ptr2 ; } * )
>
> or
>
> void foo ( struct s3 { struct s1 { int j ; } * ptr1 ; struct s2 { UNKNOWN } * 
> ptr2 ; } * )
> respectively.

Thanks, I have built an independant test case from this:

$ cat t1.c
struct s1;
struct s2 {
int i;
};
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
};
void foo(struct s3*);
$ cat t2.c
struct s1 {
int j;
};
struct s2;
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
};
void foo(struct s3*);
$ gcc -g -c t1.c
$ gcc -g -c t2.c
$ abidiff t1.o t2.o
$ 

So, as you see here, abidiff considers t1.o and t2.o has having the same
ABI, so it considers the two foo functions to be equivalent.

> The types are the same, but their visibility in the different
> compilation units differs.

I see, for genksyms, the order of declarations matters, especially when
forward declarations are involved.

Libabigail does a "whole binary" analysis of types.

So, consider the point of use of the type 'struct s1*'.  Even if 'struct
s' is just forward-declared at that point, the declaration of struct s1
is "resolved" to its definition.  Even if the definition comes later in
the binary.

In other words, if struct s1 is defined in the binary, you'll never have
that "struct s1 {UNKNOWN} *ptr1;" that you see in genksyms's
representation.

Cheers,

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-14 09:58, Dodji Seketeli wrote:
> Michal Marek  a écrit:
> 
> [...]
> 
>> Does the abidiff tool handle the case when an exported symbol is moved
>> between .c files? This is always a mess with genksyms, because the two
>> .c files have different includes and thus the type expansion stops at
>> different points. So typically the move needs to be reverted as a
>> workaround.
> 
> Let's consider the function:
> 
>   'void foo(struct S*);'
> 
> If two ELF binaries contain a definition of that function foo which ELF
> symbol is exported, if the type struct S hasn't changed, and if the only
> difference between the ELF binaries is that foo was defined in the
> translation unit a.c in the first binary and in b.c in the second
> binary, then the comparison engine of libabigail (which is the library
> that abidiff uses) will consider the declarations of the two foo
> functions as being equal -- no matter what include file comes before the
> definition point of foo in a.c and b.c.  If it does not, then it's a bug
> that ought to be fixed.
> 
> If you feel that I haven't understood your question, then I guess a
> minimal standalone example (in the form of C source code) that
> illustrates your use case could be helpful to me.

A minimal example would be

t1.c:
struct s1;
struct s2 {
int i;
}
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
}
void foo(struct s3*);
EXPORT_SYMBOL(foo);

t2.c:
struct s1 {
int j;
}
struct s2;
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
}
void foo(struct s3*);
EXPORT_SYMBOL(foo);

genksyms expands this to
void foo ( struct s3 { struct s1 { UNKNOWN } * ptr1 ; struct s2 { int i ; } * 
ptr2 ; } * )

or

void foo ( struct s3 { struct s1 { int j ; } * ptr1 ; struct s2 { UNKNOWN } * 
ptr2 ; } * )

respectively. The types are the same, but their visibility in the
different compilation units differs.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-14 09:58, Dodji Seketeli wrote:
> Michal Marek  a écrit:
> 
> [...]
> 
>> Does the abidiff tool handle the case when an exported symbol is moved
>> between .c files? This is always a mess with genksyms, because the two
>> .c files have different includes and thus the type expansion stops at
>> different points. So typically the move needs to be reverted as a
>> workaround.
> 
> Let's consider the function:
> 
>   'void foo(struct S*);'
> 
> If two ELF binaries contain a definition of that function foo which ELF
> symbol is exported, if the type struct S hasn't changed, and if the only
> difference between the ELF binaries is that foo was defined in the
> translation unit a.c in the first binary and in b.c in the second
> binary, then the comparison engine of libabigail (which is the library
> that abidiff uses) will consider the declarations of the two foo
> functions as being equal -- no matter what include file comes before the
> definition point of foo in a.c and b.c.  If it does not, then it's a bug
> that ought to be fixed.
> 
> If you feel that I haven't understood your question, then I guess a
> minimal standalone example (in the form of C source code) that
> illustrates your use case could be helpful to me.

A minimal example would be

t1.c:
struct s1;
struct s2 {
int i;
}
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
}
void foo(struct s3*);
EXPORT_SYMBOL(foo);

t2.c:
struct s1 {
int j;
}
struct s2;
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
}
void foo(struct s3*);
EXPORT_SYMBOL(foo);

genksyms expands this to
void foo ( struct s3 { struct s1 { UNKNOWN } * ptr1 ; struct s2 { int i ; } * 
ptr2 ; } * )

or

void foo ( struct s3 { struct s1 { int j ; } * ptr1 ; struct s2 { UNKNOWN } * 
ptr2 ; } * )

respectively. The types are the same, but their visibility in the
different compilation units differs.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Michal Marek  a écrit:

[...]

> Does the abidiff tool handle the case when an exported symbol is moved
> between .c files? This is always a mess with genksyms, because the two
> .c files have different includes and thus the type expansion stops at
> different points. So typically the move needs to be reverted as a
> workaround.

Let's consider the function:

  'void foo(struct S*);'

If two ELF binaries contain a definition of that function foo which ELF
symbol is exported, if the type struct S hasn't changed, and if the only
difference between the ELF binaries is that foo was defined in the
translation unit a.c in the first binary and in b.c in the second
binary, then the comparison engine of libabigail (which is the library
that abidiff uses) will consider the declarations of the two foo
functions as being equal -- no matter what include file comes before the
definition point of foo in a.c and b.c.  If it does not, then it's a bug
that ought to be fixed.

If you feel that I haven't understood your question, then I guess a
minimal standalone example (in the form of C source code) that
illustrates your use case could be helpful to me.

Thanks.

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Michal Marek  a écrit:

[...]

> Does the abidiff tool handle the case when an exported symbol is moved
> between .c files? This is always a mess with genksyms, because the two
> .c files have different includes and thus the type expansion stops at
> different points. So typically the move needs to be reverted as a
> workaround.

Let's consider the function:

  'void foo(struct S*);'

If two ELF binaries contain a definition of that function foo which ELF
symbol is exported, if the type struct S hasn't changed, and if the only
difference between the ELF binaries is that foo was defined in the
translation unit a.c in the first binary and in b.c in the second
binary, then the comparison engine of libabigail (which is the library
that abidiff uses) will consider the declarations of the two foo
functions as being equal -- no matter what include file comes before the
definition point of foo in a.c and b.c.  If it does not, then it's a bug
that ought to be fixed.

If you feel that I haven't understood your question, then I guess a
minimal standalone example (in the form of C source code) that
illustrates your use case could be helpful to me.

Thanks.

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

Dne 9.12.2016 v 23:46 Dodji Seketeli napsal(a):
> Hello,
> 
> Nicholas Piggin  a écrit:
> 
> [...]
> 
>> That said, a dwarf based checker tool should be able to do as good a job
>> (maybe a bit better because report is very informative and it may pick up
>> compiler alignments or padding options).
> 
> So, Nicholas was kind enough to send me the two Linux Kernel binaries
> that he built with the tiny little interface change that we were
> discussing earlier.  Here is what the abidiff[1] tools says about that
> interface change:
> 
> $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi 
> vmlinux.abi2.abi
> Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> 
> 1 function with some indirect sub-type change:
> 
>   [C]'function int foo(blah*)' at memory.c:82:1 has some indirect 
> sub-type changes:
> parameter 1 of type 'blah*' has sub-type changes:
>   in pointed to type 'struct blah' at memory.c:78:1:
> type size changed from 32 to 64 bits
> 1 data member insertion:
>   'int blah::y', at offset 0 (in bits) at memory.c:79:1
> 1 data member change:
>  'int blah::x' offset changed from 0 to 32 (in bits) (by +32 bits)

For completeness, with a foo.symref file in the tree, genksyms would print

foo.c: warning: foo: modversion changed because of changes in struct blah

So there is some sort of diagnostics already. Does the abidiff tool
handle the case when an exported symbol is moved between .c files? This
is always a mess with genksyms, because the two .c files have different
includes and thus the type expansion stops at different points. So
typically the move needs to be reverted as a workaround.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

Dne 9.12.2016 v 23:46 Dodji Seketeli napsal(a):
> Hello,
> 
> Nicholas Piggin  a écrit:
> 
> [...]
> 
>> That said, a dwarf based checker tool should be able to do as good a job
>> (maybe a bit better because report is very informative and it may pick up
>> compiler alignments or padding options).
> 
> So, Nicholas was kind enough to send me the two Linux Kernel binaries
> that he built with the tiny little interface change that we were
> discussing earlier.  Here is what the abidiff[1] tools says about that
> interface change:
> 
> $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi 
> vmlinux.abi2.abi
> Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> 
> 1 function with some indirect sub-type change:
> 
>   [C]'function int foo(blah*)' at memory.c:82:1 has some indirect 
> sub-type changes:
> parameter 1 of type 'blah*' has sub-type changes:
>   in pointed to type 'struct blah' at memory.c:78:1:
> type size changed from 32 to 64 bits
> 1 data member insertion:
>   'int blah::y', at offset 0 (in bits) at memory.c:79:1
> 1 data member change:
>  'int blah::x' offset changed from 0 to 32 (in bits) (by +32 bits)

For completeness, with a foo.symref file in the tree, genksyms would print

foo.c: warning: foo: modversion changed because of changes in struct blah

So there is some sort of diagnostics already. Does the abidiff tool
handle the case when an exported symbol is moved between .c files? This
is always a mess with genksyms, because the two .c files have different
includes and thus the type expansion stops at different points. So
typically the move needs to be reverted as a workaround.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Mon, 12 Dec 2016 10:48:47 +0100
Stanislav Kozina  wrote:

>  A runtime check is still done, with per-module vermagic which distros
>  can change when they bump the ABI version. Is it really necessary to
>  have more than that (i.e., per-symbol versioning)?  
> >>>   From my point of view, it is. We need to allow changing ABI for some
> >>> modules while maintaining it for others.
> >>> In fact I think that there should be version not only for every exported
> >>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type
> >>> (in the sense of eg. structure defined in the public header file).  
> >> Well the distro can just append _v2, _v3 to the name of the function
> >> or type if it has to break compat for some reason. Would that be enough?  
> > There are other ways that distros can work around when upstream "breaks"
> > the ABI, sometimes they can rename functions, and others they can
> > "preload" structures with padding in anticipation for when/if fields get
> > added to them.  But that's all up to the distros, no need for us to
> > worry about that at all :)  
> 
> Currently, the ABI version (checksum) is stored outside of the actual 
> code in the __ksymtab section. That means that the distributions can 
> still apply upstream patches cleanly and only update the version 
> checksum if these break ABI.
> 
> With the _v2, _v3 suffixes (or similar solutions) we'd be effectively 
> storing the ABI versions directly in the code and that would cause 
> conflicts when pulling further patches from upstream.
> 
> My view is that it would be than easier to maintain out-of-tree 
> modversions (or similar tool) rather than to solve all these conflicts.

That kind of name clash should hardly be an issue, in comparison with the
care it requires to merge fixes on top of a backport which has already
changed ABI. It tends to be trivially fixable just by search/replace
in the patchfile before applying, if nothing else. But you probably *want*
to be flagged on those things and merge by hand anyway.

Backporting alone I don't think can justify symbol versioning, but I'd
like to hear from distro maintainers if any disagree.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Mon, 12 Dec 2016 10:48:47 +0100
Stanislav Kozina  wrote:

>  A runtime check is still done, with per-module vermagic which distros
>  can change when they bump the ABI version. Is it really necessary to
>  have more than that (i.e., per-symbol versioning)?  
> >>>   From my point of view, it is. We need to allow changing ABI for some
> >>> modules while maintaining it for others.
> >>> In fact I think that there should be version not only for every exported
> >>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type
> >>> (in the sense of eg. structure defined in the public header file).  
> >> Well the distro can just append _v2, _v3 to the name of the function
> >> or type if it has to break compat for some reason. Would that be enough?  
> > There are other ways that distros can work around when upstream "breaks"
> > the ABI, sometimes they can rename functions, and others they can
> > "preload" structures with padding in anticipation for when/if fields get
> > added to them.  But that's all up to the distros, no need for us to
> > worry about that at all :)  
> 
> Currently, the ABI version (checksum) is stored outside of the actual 
> code in the __ksymtab section. That means that the distributions can 
> still apply upstream patches cleanly and only update the version 
> checksum if these break ABI.
> 
> With the _v2, _v3 suffixes (or similar solutions) we'd be effectively 
> storing the ABI versions directly in the code and that would cause 
> conflicts when pulling further patches from upstream.
> 
> My view is that it would be than easier to maintain out-of-tree 
> modversions (or similar tool) rather than to solve all these conflicts.

That kind of name clash should hardly be an issue, in comparison with the
care it requires to merge fixes on top of a backport which has already
changed ABI. It tends to be trivially fixable just by search/replace
in the patchfile before applying, if nothing else. But you probably *want*
to be flagged on those things and merge by hand anyway.

Backporting alone I don't think can justify symbol versioning, but I'd
like to hear from distro maintainers if any disagree.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

Hello,


That said, a dwarf based checker tool should be able to do as good a job
(maybe a bit better because report is very informative and it may pick up
compiler alignments or padding options).

So, Nicholas was kind enough to send me the two Linux Kernel binaries
that he built with the tiny little interface change that we were
discussing earlier.  Here is what the abidiff[1] tools says about that
interface change:


Thanks Nicholas and Dodji for this great example, for comparison I think 
it would be nice to share the example run with kabi-dw too.
kabi-dw first dumps and unifies all type information into a set of text 
files, the unification takes a significant time. Then the two sets of 
these text files can be compared.


An example run would look like:
$ time ~/Code/kabi-dw/kabi-dw generate -o abi1 vmlinux.abi1
Generating symbol defs from vmlinux.abi1...

real0m29.057s
user0m13.929s
sys0m14.862s
$ time ~/Code/kabi-dw/kabi-dw generate -o abi2 vmlinux.abi2
Generating symbol defs from vmlinux.abi2...

real0m29.134s
user0m13.961s
sys0m14.921s
$ time ~/Code/kabi-dw/kabi-dw compare abi1 abi2
Changes detected in: 
home/npiggin/src/linux.vectors/mm/memory.c/struct--blah.txt

Inserted:
+0x0 int y;
Shifted:
-0x0 int x;
+0x4 int x;


real0m0.176s
user0m0.135s
sys0m0.040s

The size of the generated text files with all the relevant type 
information is as follows:

$ du -hs abi1
16Mabi1
$ find abi1 -type f | wc -l
3162

Warm regards,
-Stanislav

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

Hello,


That said, a dwarf based checker tool should be able to do as good a job
(maybe a bit better because report is very informative and it may pick up
compiler alignments or padding options).

So, Nicholas was kind enough to send me the two Linux Kernel binaries
that he built with the tiny little interface change that we were
discussing earlier.  Here is what the abidiff[1] tools says about that
interface change:


Thanks Nicholas and Dodji for this great example, for comparison I think 
it would be nice to share the example run with kabi-dw too.
kabi-dw first dumps and unifies all type information into a set of text 
files, the unification takes a significant time. Then the two sets of 
these text files can be compared.


An example run would look like:
$ time ~/Code/kabi-dw/kabi-dw generate -o abi1 vmlinux.abi1
Generating symbol defs from vmlinux.abi1...

real0m29.057s
user0m13.929s
sys0m14.862s
$ time ~/Code/kabi-dw/kabi-dw generate -o abi2 vmlinux.abi2
Generating symbol defs from vmlinux.abi2...

real0m29.134s
user0m13.961s
sys0m14.921s
$ time ~/Code/kabi-dw/kabi-dw compare abi1 abi2
Changes detected in: 
home/npiggin/src/linux.vectors/mm/memory.c/struct--blah.txt

Inserted:
+0x0 int y;
Shifted:
-0x0 int x;
+0x4 int x;


real0m0.176s
user0m0.135s
sys0m0.040s

The size of the generated text files with all the relevant type 
information is as follows:

$ du -hs abi1
16Mabi1
$ find abi1 -type f | wc -l
3162

Warm regards,
-Stanislav

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

A runtime check is still done, with per-module vermagic which distros
can change when they bump the ABI version. Is it really necessary to
have more than that (i.e., per-symbol versioning)?

  From my point of view, it is. We need to allow changing ABI for some
modules while maintaining it for others.
In fact I think that there should be version not only for every exported
symbol (in the EXPORT_SYMBOL() sense), but also for every public type
(in the sense of eg. structure defined in the public header file).

Well the distro can just append _v2, _v3 to the name of the function
or type if it has to break compat for some reason. Would that be enough?

There are other ways that distros can work around when upstream "breaks"
the ABI, sometimes they can rename functions, and others they can
"preload" structures with padding in anticipation for when/if fields get
added to them.  But that's all up to the distros, no need for us to
worry about that at all :)


Currently, the ABI version (checksum) is stored outside of the actual 
code in the __ksymtab section. That means that the distributions can 
still apply upstream patches cleanly and only update the version 
checksum if these break ABI.


With the _v2, _v3 suffixes (or similar solutions) we'd be effectively 
storing the ABI versions directly in the code and that would cause 
conflicts when pulling further patches from upstream.


My view is that it would be than easier to maintain out-of-tree 
modversions (or similar tool) rather than to solve all these conflicts.


Warm Regards,
-Stanislav

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

A runtime check is still done, with per-module vermagic which distros
can change when they bump the ABI version. Is it really necessary to
have more than that (i.e., per-symbol versioning)?

  From my point of view, it is. We need to allow changing ABI for some
modules while maintaining it for others.
In fact I think that there should be version not only for every exported
symbol (in the EXPORT_SYMBOL() sense), but also for every public type
(in the sense of eg. structure defined in the public header file).

Well the distro can just append _v2, _v3 to the name of the function
or type if it has to break compat for some reason. Would that be enough?

There are other ways that distros can work around when upstream "breaks"
the ABI, sometimes they can rename functions, and others they can
"preload" structures with padding in anticipation for when/if fields get
added to them.  But that's all up to the distros, no need for us to
worry about that at all :)


Currently, the ABI version (checksum) is stored outside of the actual 
code in the __ksymtab section. That means that the distributions can 
still apply upstream patches cleanly and only update the version 
checksum if these break ABI.


With the _v2, _v3 suffixes (or similar solutions) we'd be effectively 
storing the ABI versions directly in the code and that would cause 
conflicts when pulling further patches from upstream.


My view is that it would be than easier to maintain out-of-tree 
modversions (or similar tool) rather than to solve all these conflicts.


Warm Regards,
-Stanislav

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Ian Campbell]

On Sat, 2016-12-10 at 13:41 +0100, Greg Kroah-Hartman wrote:
> Now I don't work on a distro anymore, but I would think that something
> like this would be really useful, pointing out exactly what changed is
> very important for distro maintainers to determine what they want to do

The .symvers produced by the current scheme aren't completely useless
from this PoV, although they aren't ideal since you need both before an
d after trees and if the changes are large or far reaching the diff can
get a bit unwieldy, so better tooling which points directly to the
actual relevant change would be no bad thing.

Ian.

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Ian Campbell]

On Sat, 2016-12-10 at 13:41 +0100, Greg Kroah-Hartman wrote:
> Now I don't work on a distro anymore, but I would think that something
> like this would be really useful, pointing out exactly what changed is
> very important for distro maintainers to determine what they want to do

The .symvers produced by the current scheme aren't completely useless
from this PoV, although they aren't ideal since you need both before an
d after trees and if the changes are large or far reaching the diff can
get a bit unwieldy, so better tooling which points directly to the
actual relevant change would be no bad thing.

Ian.

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Sat, 10 Dec 2016 13:41:03 +0100
Greg Kroah-Hartman  wrote:

> On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote:
> > Hello,
> > 
> > Nicholas Piggin  a écrit:
> > 
> > [...]
> >   
> > > That said, a dwarf based checker tool should be able to do as good a job
> > > (maybe a bit better because report is very informative and it may pick up
> > > compiler alignments or padding options).  
> > 
> > So, Nicholas was kind enough to send me the two Linux Kernel binaries
> > that he built with the tiny little interface change that we were
> > discussing earlier.  Here is what the abidiff[1] tools says about that
> > interface change:
> > 
> > $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi 
> > vmlinux.abi2.abi
> > Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> > 
> > 1 function with some indirect sub-type change:
> > 
> >   [C]'function int foo(blah*)' at memory.c:82:1 has some indirect 
> > sub-type changes:
> > parameter 1 of type 'blah*' has sub-type changes:
> >   in pointed to type 'struct blah' at memory.c:78:1:
> > type size changed from 32 to 64 bits
> > 1 data member insertion:
> >   'int blah::y', at offset 0 (in bits) at memory.c:79:1
> > 1 data member change:
> >  'int blah::x' offset changed from 0 to 32 (in bits) (by +32 
> > bits)
> > 
> > 
> > 
> > real0m2.595s
> > user0m2.489s
> > sys 0m0.108s
> > $ 
> > 
> > I kept the timing information to give you an idea of the time it takes
> > on a non-optimized build of abidiff.
> > 
> > One could for instance want that types that are not defined in header
> > files be kept out of the change report.  In that case it's possible to
> > write a little suppression specification file like this one:
> > 
> > $ cat vmlinux.abignore 
> > [suppress_type]
> >   source_location_not_regexp = .*\\.h
> > $
> > 
> > You can then pass that suppression file to the tool:
> > 
> > $ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr 
> > vmlinux.abignore vmlinux.abi1.abi vmlinux.abi2.abi
> > Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 
> > Added function
> > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> > 
> > 
> > real0m2.574s
> > user0m2.473s
> > sys 0m0.102s
> > $
> > 
> > So this is the kind of interface change analysis tool we are working on
> > at the moment.
> > 
> > One could also imagine a tool that would compute a CRC that takes the
> > very same suppression specification files into account, letting people
> > to decide that some interface changes are OK.  That CRC would thus be
> > added to the special ELF sections we already have today.  We could keep
> > the modversion machinery, but with a greater dose of flexibility.
> > Whenever modversion detects a change, abidiff would tell people what the
> > change is exactly.
> > 
> > What do you guys think?  
> 
> YES YES YES!!!
> 
> Now I don't work on a distro anymore, but I would think that something
> like this would be really useful, pointing out exactly what changed is
> very important for distro maintainers to determine what they want to do
> (either fix up the abi change with strange hacks, or ignore it due to
> the change being in an area they don't care at all about, i.e. a random
> driver subsystem.)
> 
> So yes, I think this is really good stuff.  But if the distro
> maintainers correct me and think it's useless, then I need to revisit my
> view of exactly what they do for their customers :)

Agree completely. BTW (for those who might be looking into these tools),
we also have https://github.com/skozina/kabi-dw that Stanislav (cc'ed)
mentioned earlier.

It's true that the current modversions __crc_ matching infrastructure is
"just" a symbol versioning system, and we could keep it and just populate
it with something other than genksyms (e.g., a symbol version list provided
by distros). But the starting point should be *no* versioning and simply
using names to break linkage. Unless there's a compelling reason not to,
symbols are simpler, easier, everyone knows how they work.

The other question would be whether to pull a minimal tool into the kernel
source or keep them out of tree (but possibly add some helper scripts etc).
I guess we'll need to see what distros want.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Sat, 10 Dec 2016 13:41:03 +0100
Greg Kroah-Hartman  wrote:

> On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote:
> > Hello,
> > 
> > Nicholas Piggin  a écrit:
> > 
> > [...]
> >   
> > > That said, a dwarf based checker tool should be able to do as good a job
> > > (maybe a bit better because report is very informative and it may pick up
> > > compiler alignments or padding options).  
> > 
> > So, Nicholas was kind enough to send me the two Linux Kernel binaries
> > that he built with the tiny little interface change that we were
> > discussing earlier.  Here is what the abidiff[1] tools says about that
> > interface change:
> > 
> > $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi 
> > vmlinux.abi2.abi
> > Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> > 
> > 1 function with some indirect sub-type change:
> > 
> >   [C]'function int foo(blah*)' at memory.c:82:1 has some indirect 
> > sub-type changes:
> > parameter 1 of type 'blah*' has sub-type changes:
> >   in pointed to type 'struct blah' at memory.c:78:1:
> > type size changed from 32 to 64 bits
> > 1 data member insertion:
> >   'int blah::y', at offset 0 (in bits) at memory.c:79:1
> > 1 data member change:
> >  'int blah::x' offset changed from 0 to 32 (in bits) (by +32 
> > bits)
> > 
> > 
> > 
> > real0m2.595s
> > user0m2.489s
> > sys 0m0.108s
> > $ 
> > 
> > I kept the timing information to give you an idea of the time it takes
> > on a non-optimized build of abidiff.
> > 
> > One could for instance want that types that are not defined in header
> > files be kept out of the change report.  In that case it's possible to
> > write a little suppression specification file like this one:
> > 
> > $ cat vmlinux.abignore 
> > [suppress_type]
> >   source_location_not_regexp = .*\\.h
> > $
> > 
> > You can then pass that suppression file to the tool:
> > 
> > $ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr 
> > vmlinux.abignore vmlinux.abi1.abi vmlinux.abi2.abi
> > Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 
> > Added function
> > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> > 
> > 
> > real0m2.574s
> > user0m2.473s
> > sys 0m0.102s
> > $
> > 
> > So this is the kind of interface change analysis tool we are working on
> > at the moment.
> > 
> > One could also imagine a tool that would compute a CRC that takes the
> > very same suppression specification files into account, letting people
> > to decide that some interface changes are OK.  That CRC would thus be
> > added to the special ELF sections we already have today.  We could keep
> > the modversion machinery, but with a greater dose of flexibility.
> > Whenever modversion detects a change, abidiff would tell people what the
> > change is exactly.
> > 
> > What do you guys think?  
> 
> YES YES YES!!!
> 
> Now I don't work on a distro anymore, but I would think that something
> like this would be really useful, pointing out exactly what changed is
> very important for distro maintainers to determine what they want to do
> (either fix up the abi change with strange hacks, or ignore it due to
> the change being in an area they don't care at all about, i.e. a random
> driver subsystem.)
> 
> So yes, I think this is really good stuff.  But if the distro
> maintainers correct me and think it's useless, then I need to revisit my
> view of exactly what they do for their customers :)

Agree completely. BTW (for those who might be looking into these tools),
we also have https://github.com/skozina/kabi-dw that Stanislav (cc'ed)
mentioned earlier.

It's true that the current modversions __crc_ matching infrastructure is
"just" a symbol versioning system, and we could keep it and just populate
it with something other than genksyms (e.g., a symbol version list provided
by distros). But the starting point should be *no* versioning and simply
using names to break linkage. Unless there's a compelling reason not to,
symbols are simpler, easier, everyone knows how they work.

The other question would be whether to pull a minimal tool into the kernel
source or keep them out of tree (but possibly add some helper scripts etc).
I guess we'll need to see what distros want.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Greg Kroah-Hartman]

On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote:
> Hello,
> 
> Nicholas Piggin  a écrit:
> 
> [...]
> 
> > That said, a dwarf based checker tool should be able to do as good a job
> > (maybe a bit better because report is very informative and it may pick up
> > compiler alignments or padding options).
> 
> So, Nicholas was kind enough to send me the two Linux Kernel binaries
> that he built with the tiny little interface change that we were
> discussing earlier.  Here is what the abidiff[1] tools says about that
> interface change:
> 
> $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi 
> vmlinux.abi2.abi
> Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> 
> 1 function with some indirect sub-type change:
> 
>   [C]'function int foo(blah*)' at memory.c:82:1 has some indirect 
> sub-type changes:
> parameter 1 of type 'blah*' has sub-type changes:
>   in pointed to type 'struct blah' at memory.c:78:1:
> type size changed from 32 to 64 bits
> 1 data member insertion:
>   'int blah::y', at offset 0 (in bits) at memory.c:79:1
> 1 data member change:
>  'int blah::x' offset changed from 0 to 32 (in bits) (by +32 bits)
> 
> 
> 
> real  0m2.595s
> user  0m2.489s
> sys   0m0.108s
> $ 
> 
> I kept the timing information to give you an idea of the time it takes
> on a non-optimized build of abidiff.
> 
> One could for instance want that types that are not defined in header
> files be kept out of the change report.  In that case it's possible to
> write a little suppression specification file like this one:
> 
> $ cat vmlinux.abignore 
> [suppress_type]
>   source_location_not_regexp = .*\\.h
> $
> 
> You can then pass that suppression file to the tool:
> 
> $ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr vmlinux.abignore 
> vmlinux.abi1.abi vmlinux.abi2.abi
> Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added 
> function
> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> 
> 
> real  0m2.574s
> user  0m2.473s
> sys   0m0.102s
> $
> 
> So this is the kind of interface change analysis tool we are working on
> at the moment.
> 
> One could also imagine a tool that would compute a CRC that takes the
> very same suppression specification files into account, letting people
> to decide that some interface changes are OK.  That CRC would thus be
> added to the special ELF sections we already have today.  We could keep
> the modversion machinery, but with a greater dose of flexibility.
> Whenever modversion detects a change, abidiff would tell people what the
> change is exactly.
> 
> What do you guys think?

YES YES YES!!!

Now I don't work on a distro anymore, but I would think that something
like this would be really useful, pointing out exactly what changed is
very important for distro maintainers to determine what they want to do
(either fix up the abi change with strange hacks, or ignore it due to
the change being in an area they don't care at all about, i.e. a random
driver subsystem.)

So yes, I think this is really good stuff.  But if the distro
maintainers correct me and think it's useless, then I need to revisit my
view of exactly what they do for their customers :)

thanks,

greg k-h

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Greg Kroah-Hartman]

On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote:
> Hello,
> 
> Nicholas Piggin  a écrit:
> 
> [...]
> 
> > That said, a dwarf based checker tool should be able to do as good a job
> > (maybe a bit better because report is very informative and it may pick up
> > compiler alignments or padding options).
> 
> So, Nicholas was kind enough to send me the two Linux Kernel binaries
> that he built with the tiny little interface change that we were
> discussing earlier.  Here is what the abidiff[1] tools says about that
> interface change:
> 
> $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi 
> vmlinux.abi2.abi
> Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> 
> 1 function with some indirect sub-type change:
> 
>   [C]'function int foo(blah*)' at memory.c:82:1 has some indirect 
> sub-type changes:
> parameter 1 of type 'blah*' has sub-type changes:
>   in pointed to type 'struct blah' at memory.c:78:1:
> type size changed from 32 to 64 bits
> 1 data member insertion:
>   'int blah::y', at offset 0 (in bits) at memory.c:79:1
> 1 data member change:
>  'int blah::x' offset changed from 0 to 32 (in bits) (by +32 bits)
> 
> 
> 
> real  0m2.595s
> user  0m2.489s
> sys   0m0.108s
> $ 
> 
> I kept the timing information to give you an idea of the time it takes
> on a non-optimized build of abidiff.
> 
> One could for instance want that types that are not defined in header
> files be kept out of the change report.  In that case it's possible to
> write a little suppression specification file like this one:
> 
> $ cat vmlinux.abignore 
> [suppress_type]
>   source_location_not_regexp = .*\\.h
> $
> 
> You can then pass that suppression file to the tool:
> 
> $ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr vmlinux.abignore 
> vmlinux.abi1.abi vmlinux.abi2.abi
> Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added 
> function
> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> 
> 
> real  0m2.574s
> user  0m2.473s
> sys   0m0.102s
> $
> 
> So this is the kind of interface change analysis tool we are working on
> at the moment.
> 
> One could also imagine a tool that would compute a CRC that takes the
> very same suppression specification files into account, letting people
> to decide that some interface changes are OK.  That CRC would thus be
> added to the special ELF sections we already have today.  We could keep
> the modversion machinery, but with a greater dose of flexibility.
> Whenever modversion detects a change, abidiff would tell people what the
> change is exactly.
> 
> What do you guys think?

YES YES YES!!!

Now I don't work on a distro anymore, but I would think that something
like this would be really useful, pointing out exactly what changed is
very important for distro maintainers to determine what they want to do
(either fix up the abi change with strange hacks, or ignore it due to
the change being in an area they don't care at all about, i.e. a random
driver subsystem.)

So yes, I think this is really good stuff.  But if the distro
maintainers correct me and think it's useless, then I need to revisit my
view of exactly what they do for their customers :)

thanks,

greg k-h

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Hello,

Nicholas Piggin  a écrit:

[...]

> That said, a dwarf based checker tool should be able to do as good a job
> (maybe a bit better because report is very informative and it may pick up
> compiler alignments or padding options).

So, Nicholas was kind enough to send me the two Linux Kernel binaries
that he built with the tiny little interface change that we were
discussing earlier.  Here is what the abidiff[1] tools says about that
interface change:

$ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi 
vmlinux.abi2.abi
Functions changes summary: 0 Removed, 1 Changed, 0 Added function
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 function with some indirect sub-type change:

  [C]'function int foo(blah*)' at memory.c:82:1 has some indirect sub-type 
changes:
parameter 1 of type 'blah*' has sub-type changes:
  in pointed to type 'struct blah' at memory.c:78:1:
type size changed from 32 to 64 bits
1 data member insertion:
  'int blah::y', at offset 0 (in bits) at memory.c:79:1
1 data member change:
 'int blah::x' offset changed from 0 to 32 (in bits) (by +32 bits)



real0m2.595s
user0m2.489s
sys 0m0.108s
$ 

I kept the timing information to give you an idea of the time it takes
on a non-optimized build of abidiff.

One could for instance want that types that are not defined in header
files be kept out of the change report.  In that case it's possible to
write a little suppression specification file like this one:

$ cat vmlinux.abignore 
[suppress_type]
  source_location_not_regexp = .*\\.h
$

You can then pass that suppression file to the tool:

$ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr vmlinux.abignore 
vmlinux.abi1.abi vmlinux.abi2.abi
Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added 
function
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable


real0m2.574s
user0m2.473s
sys 0m0.102s
$

So this is the kind of interface change analysis tool we are working on
at the moment.

One could also imagine a tool that would compute a CRC that takes the
very same suppression specification files into account, letting people
to decide that some interface changes are OK.  That CRC would thus be
added to the special ELF sections we already have today.  We could keep
the modversion machinery, but with a greater dose of flexibility.
Whenever modversion detects a change, abidiff would tell people what the
change is exactly.

What do you guys think?

[1]: https://sourceware.org/libabigail/manual/abidiff.html
 Okay, the abidiff I used in this message is one that is not yet
 released.  It's source code is in the dodji/kabidiff branch of the
 Git repository at https://sourceware.org/git/?p=libabigail.git;a=summary


Cheers,

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Hello,

Nicholas Piggin  a écrit:

[...]

> That said, a dwarf based checker tool should be able to do as good a job
> (maybe a bit better because report is very informative and it may pick up
> compiler alignments or padding options).

So, Nicholas was kind enough to send me the two Linux Kernel binaries
that he built with the tiny little interface change that we were
discussing earlier.  Here is what the abidiff[1] tools says about that
interface change:

$ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi 
vmlinux.abi2.abi
Functions changes summary: 0 Removed, 1 Changed, 0 Added function
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 function with some indirect sub-type change:

  [C]'function int foo(blah*)' at memory.c:82:1 has some indirect sub-type 
changes:
parameter 1 of type 'blah*' has sub-type changes:
  in pointed to type 'struct blah' at memory.c:78:1:
type size changed from 32 to 64 bits
1 data member insertion:
  'int blah::y', at offset 0 (in bits) at memory.c:79:1
1 data member change:
 'int blah::x' offset changed from 0 to 32 (in bits) (by +32 bits)



real0m2.595s
user0m2.489s
sys 0m0.108s
$ 

I kept the timing information to give you an idea of the time it takes
on a non-optimized build of abidiff.

One could for instance want that types that are not defined in header
files be kept out of the change report.  In that case it's possible to
write a little suppression specification file like this one:

$ cat vmlinux.abignore 
[suppress_type]
  source_location_not_regexp = .*\\.h
$

You can then pass that suppression file to the tool:

$ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr vmlinux.abignore 
vmlinux.abi1.abi vmlinux.abi2.abi
Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added 
function
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable


real0m2.574s
user0m2.473s
sys 0m0.102s
$

So this is the kind of interface change analysis tool we are working on
at the moment.

One could also imagine a tool that would compute a CRC that takes the
very same suppression specification files into account, letting people
to decide that some interface changes are OK.  That CRC would thus be
added to the special ELF sections we already have today.  We could keep
the modversion machinery, but with a greater dose of flexibility.
Whenever modversion detects a change, abidiff would tell people what the
change is exactly.

What do you guys think?

[1]: https://sourceware.org/libabigail/manual/abidiff.html
 Okay, the abidiff I used in this message is one that is not yet
 released.  It's source code is in the dodji/kabidiff branch of the
 Git repository at https://sourceware.org/git/?p=libabigail.git;a=summary


Cheers,

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Don Zickus]

On Fri, Dec 09, 2016 at 01:50:41PM +1000, Nicholas Piggin wrote:
> > 
> > We have plenty of customers with 10 year old drivers, where the expertise
> > has long left the company.  The engineers still around, recompile and make
> > tweaks to get things working on the latest RHEL.  Verify it passes testing
> > and release it.  Then they hope to not touch it again for a few years until
> > the next RHEL comes along.
> > 
> > Scary, huh? :-)
> 
> Oh yeah my aim here is not to make distro or out of tree module vendors
> life harder, actually the opposite. If it turns out modversions really is
> the best approach, I'm not in a position to complain about its complexity
> because we have Suse and Redhat people maintaining the build and module
> systems :) I just want to see if we can do things better.

Hi Nick,

I think we are in pretty good agreement here.  We can do better than
modversions.  On the flip side, I would hate to see modversions ripped out
until we have an alternate path forward as it does get us by for now. :-)

Cheers,
Don

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Don Zickus]

On Fri, Dec 09, 2016 at 01:50:41PM +1000, Nicholas Piggin wrote:
> > 
> > We have plenty of customers with 10 year old drivers, where the expertise
> > has long left the company.  The engineers still around, recompile and make
> > tweaks to get things working on the latest RHEL.  Verify it passes testing
> > and release it.  Then they hope to not touch it again for a few years until
> > the next RHEL comes along.
> > 
> > Scary, huh? :-)
> 
> Oh yeah my aim here is not to make distro or out of tree module vendors
> life harder, actually the opposite. If it turns out modversions really is
> the best approach, I'm not in a position to complain about its complexity
> because we have Suse and Redhat people maintaining the build and module
> systems :) I just want to see if we can do things better.

Hi Nick,

I think we are in pretty good agreement here.  We can do better than
modversions.  On the flip side, I would hate to see modversions ripped out
until we have an alternate path forward as it does get us by for now. :-)

Cheers,
Don

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Fri, 09 Dec 2016 15:21:33 +
Ian Campbell  wrote:

> On Fri, 2016-12-09 at 13:33 +1000, Nicholas Piggin wrote:
> > 
> > Well I simply tested the outcome. If you have:
> > 
> > struct blah {
> >   int x;
> > };
> > int foo(struct blah *blah)
> > {
> >   return blah->x;
> > }
> > EXPORT(foo);
> > 
> > $ nm vmlinux | grep __crc_foo
> > a0cf13a0 A __crc_foo
> > 
> > Now change to
> > 
> > struct blah {
> >   int y;
> >   int x;
> > };
> > 
> > $ nm vmlinux | grep __crc_foo
> > a0cf13a0 A __crc_foo
> > 
> > It just doesn't catch these things.  
> 
> I found the same when I just added your snippet to init/main.c.
> 
> _But_ when I moved the struct into include/types.h (which happened to
> be included by init/main.c) then, with just x in the struct:
> 
> $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes 
> && nm init/main.o  | grep __crc_foo
> s#blah struct blah { int x ; } 
> foo int foo ( s#blah * ) 
> 0cd0312e A __crc_foo
> 
> but adding y:
> 
> $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes 
> && nm init/main.o  | grep __crc_foo
> s#blah struct blah { int x ; int y ; } 
> foo int foo ( s#blah * ) 
> eda220c6 A __crc_foo
> 
> So it does catch things in that case.
> 
> With struct blah inline in main.c it was:
> 
> $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes 
> && nm init/main.o  | grep __crc_foo
> s#blah struct blah { UNKNOWN } 
> foo int foo ( s#blah * ) 
> a0cf13a0 A __crc_foo
> 
> So I suppose it only cares about structs which are in headers, which I
> guess makes sense. I think it is working in at least one of the
> important cases.

Aha thanks, well that's my mistake. Clever little bugger, isn't it? Okay
it's not so useless as I first thought!

That said, a dwarf based checker tool should be able to do as good a job
(maybe a bit better because report is very informative and it may pick up
compiler alignments or padding options). So I still think it's worth
looking at those if we can remove modversions.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Fri, 09 Dec 2016 15:21:33 +
Ian Campbell  wrote:

> On Fri, 2016-12-09 at 13:33 +1000, Nicholas Piggin wrote:
> > 
> > Well I simply tested the outcome. If you have:
> > 
> > struct blah {
> >   int x;
> > };
> > int foo(struct blah *blah)
> > {
> >   return blah->x;
> > }
> > EXPORT(foo);
> > 
> > $ nm vmlinux | grep __crc_foo
> > a0cf13a0 A __crc_foo
> > 
> > Now change to
> > 
> > struct blah {
> >   int y;
> >   int x;
> > };
> > 
> > $ nm vmlinux | grep __crc_foo
> > a0cf13a0 A __crc_foo
> > 
> > It just doesn't catch these things.  
> 
> I found the same when I just added your snippet to init/main.c.
> 
> _But_ when I moved the struct into include/types.h (which happened to
> be included by init/main.c) then, with just x in the struct:
> 
> $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes 
> && nm init/main.o  | grep __crc_foo
> s#blah struct blah { int x ; } 
> foo int foo ( s#blah * ) 
> 0cd0312e A __crc_foo
> 
> but adding y:
> 
> $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes 
> && nm init/main.o  | grep __crc_foo
> s#blah struct blah { int x ; int y ; } 
> foo int foo ( s#blah * ) 
> eda220c6 A __crc_foo
> 
> So it does catch things in that case.
> 
> With struct blah inline in main.c it was:
> 
> $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes 
> && nm init/main.o  | grep __crc_foo
> s#blah struct blah { UNKNOWN } 
> foo int foo ( s#blah * ) 
> a0cf13a0 A __crc_foo
> 
> So I suppose it only cares about structs which are in headers, which I
> guess makes sense. I think it is working in at least one of the
> important cases.

Aha thanks, well that's my mistake. Clever little bugger, isn't it? Okay
it's not so useless as I first thought!

That said, a dwarf based checker tool should be able to do as good a job
(maybe a bit better because report is very informative and it may pick up
compiler alignments or padding options). So I still think it's worth
looking at those if we can remove modversions.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Greg Kroah-Hartman]

On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:
> On Fri, 9 Dec 2016 15:36:04 +0100
> Stanislav Kozina  wrote:
> 
> >  The question is how to provide a similar guarantee if a different way? 
> >   
> > >>> As a tool to aid distro reviewers, modversions has some value, but the
> > >>> debug info parsing tools that have been mentioned in this thread seem
> > >>> superior (not that I've tested them).  
> > >> On the other hand the big advantage of modversions is that it also
> > >> verifies the checksum during runtime (module loading). In other words, I
> > >> believe that any other solution should still generate some form of
> > >> checksum/watermark which can be easily checked for compatibility on
> > >> module load.
> > >> It should not be hard to add to the DWARF based tools though. We'd just
> > >> parse DWARF data instead of the C code.  
> > > A runtime check is still done, with per-module vermagic which distros
> > > can change when they bump the ABI version. Is it really necessary to
> > > have more than that (i.e., per-symbol versioning)?  
> > 
> >  From my point of view, it is. We need to allow changing ABI for some 
> > modules while maintaining it for others.
> > In fact I think that there should be version not only for every exported 
> > symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> > (in the sense of eg. structure defined in the public header file).
> 
> Well the distro can just append _v2, _v3 to the name of the function
> or type if it has to break compat for some reason. Would that be enough?

There are other ways that distros can work around when upstream "breaks"
the ABI, sometimes they can rename functions, and others they can
"preload" structures with padding in anticipation for when/if fields get
added to them.  But that's all up to the distros, no need for us to
worry about that at all :)

thanks,

greg k-h

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Greg Kroah-Hartman]

On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:
> On Fri, 9 Dec 2016 15:36:04 +0100
> Stanislav Kozina  wrote:
> 
> >  The question is how to provide a similar guarantee if a different way? 
> >   
> > >>> As a tool to aid distro reviewers, modversions has some value, but the
> > >>> debug info parsing tools that have been mentioned in this thread seem
> > >>> superior (not that I've tested them).  
> > >> On the other hand the big advantage of modversions is that it also
> > >> verifies the checksum during runtime (module loading). In other words, I
> > >> believe that any other solution should still generate some form of
> > >> checksum/watermark which can be easily checked for compatibility on
> > >> module load.
> > >> It should not be hard to add to the DWARF based tools though. We'd just
> > >> parse DWARF data instead of the C code.  
> > > A runtime check is still done, with per-module vermagic which distros
> > > can change when they bump the ABI version. Is it really necessary to
> > > have more than that (i.e., per-symbol versioning)?  
> > 
> >  From my point of view, it is. We need to allow changing ABI for some 
> > modules while maintaining it for others.
> > In fact I think that there should be version not only for every exported 
> > symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> > (in the sense of eg. structure defined in the public header file).
> 
> Well the distro can just append _v2, _v3 to the name of the function
> or type if it has to break compat for some reason. Would that be enough?

There are other ways that distros can work around when upstream "breaks"
the ABI, sometimes they can rename functions, and others they can
"preload" structures with padding in anticipation for when/if fields get
added to them.  But that's all up to the distros, no need for us to
worry about that at all :)

thanks,

greg k-h

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Fri, 9 Dec 2016 15:36:04 +0100
Stanislav Kozina  wrote:

>  The question is how to provide a similar guarantee if a different way?  
> >>> As a tool to aid distro reviewers, modversions has some value, but the
> >>> debug info parsing tools that have been mentioned in this thread seem
> >>> superior (not that I've tested them).  
> >> On the other hand the big advantage of modversions is that it also
> >> verifies the checksum during runtime (module loading). In other words, I
> >> believe that any other solution should still generate some form of
> >> checksum/watermark which can be easily checked for compatibility on
> >> module load.
> >> It should not be hard to add to the DWARF based tools though. We'd just
> >> parse DWARF data instead of the C code.  
> > A runtime check is still done, with per-module vermagic which distros
> > can change when they bump the ABI version. Is it really necessary to
> > have more than that (i.e., per-symbol versioning)?  
> 
>  From my point of view, it is. We need to allow changing ABI for some 
> modules while maintaining it for others.
> In fact I think that there should be version not only for every exported 
> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> (in the sense of eg. structure defined in the public header file).

Well the distro can just append _v2, _v3 to the name of the function
or type if it has to break compat for some reason. Would that be enough?

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Fri, 9 Dec 2016 15:36:04 +0100
Stanislav Kozina  wrote:

>  The question is how to provide a similar guarantee if a different way?  
> >>> As a tool to aid distro reviewers, modversions has some value, but the
> >>> debug info parsing tools that have been mentioned in this thread seem
> >>> superior (not that I've tested them).  
> >> On the other hand the big advantage of modversions is that it also
> >> verifies the checksum during runtime (module loading). In other words, I
> >> believe that any other solution should still generate some form of
> >> checksum/watermark which can be easily checked for compatibility on
> >> module load.
> >> It should not be hard to add to the DWARF based tools though. We'd just
> >> parse DWARF data instead of the C code.  
> > A runtime check is still done, with per-module vermagic which distros
> > can change when they bump the ABI version. Is it really necessary to
> > have more than that (i.e., per-symbol versioning)?  
> 
>  From my point of view, it is. We need to allow changing ABI for some 
> modules while maintaining it for others.
> In fact I think that there should be version not only for every exported 
> symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
> (in the sense of eg. structure defined in the public header file).

Well the distro can just append _v2, _v3 to the name of the function
or type if it has to break compat for some reason. Would that be enough?

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Ian Campbell]

On Fri, 2016-12-09 at 13:33 +1000, Nicholas Piggin wrote:
> 
> Well I simply tested the outcome. If you have:
> 
> struct blah {
>   int x;
> };
> int foo(struct blah *blah)
> {
>   return blah->x;
> }
> EXPORT(foo);
> 
> $ nm vmlinux | grep __crc_foo
> a0cf13a0 A __crc_foo
> 
> Now change to
> 
> struct blah {
>   int y;
>   int x;
> };
> 
> $ nm vmlinux | grep __crc_foo
> a0cf13a0 A __crc_foo
> 
> It just doesn't catch these things.

I found the same when I just added your snippet to init/main.c.

_But_ when I moved the struct into include/types.h (which happened to
be included by init/main.c) then, with just x in the struct:

$ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && 
nm init/main.o  | grep __crc_foo
s#blah struct blah { int x ; } 
foo int foo ( s#blah * ) 
0cd0312e A __crc_foo

but adding y:

$ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && 
nm init/main.o  | grep __crc_foo
s#blah struct blah { int x ; int y ; } 
foo int foo ( s#blah * ) 
eda220c6 A __crc_foo

So it does catch things in that case.

With struct blah inline in main.c it was:

$ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && 
nm init/main.o  | grep __crc_foo
s#blah struct blah { UNKNOWN } 
foo int foo ( s#blah * ) 
a0cf13a0 A __crc_foo

So I suppose it only cares about structs which are in headers, which I
guess makes sense. I think it is working in at least one of the
important cases.

Ian.

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Ian Campbell]

On Fri, 2016-12-09 at 13:33 +1000, Nicholas Piggin wrote:
> 
> Well I simply tested the outcome. If you have:
> 
> struct blah {
>   int x;
> };
> int foo(struct blah *blah)
> {
>   return blah->x;
> }
> EXPORT(foo);
> 
> $ nm vmlinux | grep __crc_foo
> a0cf13a0 A __crc_foo
> 
> Now change to
> 
> struct blah {
>   int y;
>   int x;
> };
> 
> $ nm vmlinux | grep __crc_foo
> a0cf13a0 A __crc_foo
> 
> It just doesn't catch these things.

I found the same when I just added your snippet to init/main.c.

_But_ when I moved the struct into include/types.h (which happened to
be included by init/main.c) then, with just x in the struct:

$ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && 
nm init/main.o  | grep __crc_foo
s#blah struct blah { int x ; } 
foo int foo ( s#blah * ) 
0cd0312e A __crc_foo

but adding y:

$ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && 
nm init/main.o  | grep __crc_foo
s#blah struct blah { int x ; int y ; } 
foo int foo ( s#blah * ) 
eda220c6 A __crc_foo

So it does catch things in that case.

With struct blah inline in main.c it was:

$ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && 
nm init/main.o  | grep __crc_foo
s#blah struct blah { UNKNOWN } 
foo int foo ( s#blah * ) 
a0cf13a0 A __crc_foo

So I suppose it only cares about structs which are in headers, which I
guess makes sense. I think it is working in at least one of the
important cases.

Ian.

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

The question is how to provide a similar guarantee if a different way?

As a tool to aid distro reviewers, modversions has some value, but the
debug info parsing tools that have been mentioned in this thread seem
superior (not that I've tested them).

On the other hand the big advantage of modversions is that it also
verifies the checksum during runtime (module loading). In other words, I
believe that any other solution should still generate some form of
checksum/watermark which can be easily checked for compatibility on
module load.
It should not be hard to add to the DWARF based tools though. We'd just
parse DWARF data instead of the C code.

A runtime check is still done, with per-module vermagic which distros
can change when they bump the ABI version. Is it really necessary to
have more than that (i.e., per-symbol versioning)?


From my point of view, it is. We need to allow changing ABI for some 
modules while maintaining it for others.
In fact I think that there should be version not only for every exported 
symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
(in the sense of eg. structure defined in the public header file).


Thanks,
-Stanislav

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

The question is how to provide a similar guarantee if a different way?

As a tool to aid distro reviewers, modversions has some value, but the
debug info parsing tools that have been mentioned in this thread seem
superior (not that I've tested them).

On the other hand the big advantage of modversions is that it also
verifies the checksum during runtime (module loading). In other words, I
believe that any other solution should still generate some form of
checksum/watermark which can be easily checked for compatibility on
module load.
It should not be hard to add to the DWARF based tools though. We'd just
parse DWARF data instead of the C code.

A runtime check is still done, with per-module vermagic which distros
can change when they bump the ABI version. Is it really necessary to
have more than that (i.e., per-symbol versioning)?


From my point of view, it is. We need to allow changing ABI for some 
modules while maintaining it for others.
In fact I think that there should be version not only for every exported 
symbol (in the EXPORT_SYMBOL() sense), but also for every public type 
(in the sense of eg. structure defined in the public header file).


Thanks,
-Stanislav

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Fri, 9 Dec 2016 08:55:51 +0100
Stanislav Kozina  wrote:

> >> The question is how to provide a similar guarantee if a different way?  
> > As a tool to aid distro reviewers, modversions has some value, but the
> > debug info parsing tools that have been mentioned in this thread seem
> > superior (not that I've tested them).  
> 
> On the other hand the big advantage of modversions is that it also 
> verifies the checksum during runtime (module loading). In other words, I 
> believe that any other solution should still generate some form of 
> checksum/watermark which can be easily checked for compatibility on 
> module load.
> It should not be hard to add to the DWARF based tools though. We'd just 
> parse DWARF data instead of the C code.

A runtime check is still done, with per-module vermagic which distros
can change when they bump the ABI version. Is it really necessary to
have more than that (i.e., per-symbol versioning)?

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Fri, 9 Dec 2016 08:55:51 +0100
Stanislav Kozina  wrote:

> >> The question is how to provide a similar guarantee if a different way?  
> > As a tool to aid distro reviewers, modversions has some value, but the
> > debug info parsing tools that have been mentioned in this thread seem
> > superior (not that I've tested them).  
> 
> On the other hand the big advantage of modversions is that it also 
> verifies the checksum during runtime (module loading). In other words, I 
> believe that any other solution should still generate some form of 
> checksum/watermark which can be easily checked for compatibility on 
> module load.
> It should not be hard to add to the DWARF based tools though. We'd just 
> parse DWARF data instead of the C code.

A runtime check is still done, with per-module vermagic which distros
can change when they bump the ABI version. Is it really necessary to
have more than that (i.e., per-symbol versioning)?

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

The question is how to provide a similar guarantee if a different way?

As a tool to aid distro reviewers, modversions has some value, but the
debug info parsing tools that have been mentioned in this thread seem
superior (not that I've tested them).


On the other hand the big advantage of modversions is that it also 
verifies the checksum during runtime (module loading). In other words, I 
believe that any other solution should still generate some form of 
checksum/watermark which can be easily checked for compatibility on 
module load.
It should not be hard to add to the DWARF based tools though. We'd just 
parse DWARF data instead of the C code.


Regards,
-Stanislav

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

The question is how to provide a similar guarantee if a different way?

As a tool to aid distro reviewers, modversions has some value, but the
debug info parsing tools that have been mentioned in this thread seem
superior (not that I've tested them).


On the other hand the big advantage of modversions is that it also 
verifies the checksum during runtime (module loading). In other words, I 
believe that any other solution should still generate some form of 
checksum/watermark which can be easily checked for compatibility on 
module load.
It should not be hard to add to the DWARF based tools though. We'd just 
parse DWARF data instead of the C code.


Regards,
-Stanislav

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Thu, 1 Dec 2016 10:20:39 -0500
Don Zickus  wrote:

> On Thu, Dec 01, 2016 at 03:32:15PM +1100, Nicholas Piggin wrote:
> > > Anyway, MODVERSIONS is our way of protecting our kabi for the last 10 
> > > years.
> > > It isn't perfect and we have fixed the genksyms tool over the years, but 
> > > so
> > > far it mostly works fine.  
> > 
> > Okay. It would be good to get all the distros in on this.
> > 
> > What I want to do is work out exactly what it is that modversions is
> > giving you.
> > 
> > We know it's fairly nasty code to maintain and it does not detect ABI
> > changes very well. But it's not such a burden that we can't maintain
> > it if there are good reasons to keep it.  
> 
> Hi Nick,
> 
> I won't disagree with you there. :-)

Sorry for the late reply, I was moving house and got side tracked.

> modversions is a pretty heavy handed approach that basically says if all the
> symbols and types haven't changed for a given EXPORT_SYMBOL (recursively
> checked), then there is a high degree of confidence the OOT driver will not
> only load, but run correctly.

It's heavy handed in that it is quite complex in the kernel build system,
but it is also light handed in that it does not do a very good job.

I would say the degree of confidence is not very high. People have told
me modversions follows pointers to objects in its calculation, but I have
not seen that to be the case. Even if you did have that, it can not replace
a code review for semantics of data and code.

> The question is how to provide a similar guarantee if a different way?

As a tool to aid distro reviewers, modversions has some value, but the
debug info parsing tools that have been mentioned in this thread seem
superior (not that I've tested them).

> 
> We have plenty of customers with 10 year old drivers, where the expertise
> has long left the company.  The engineers still around, recompile and make
> tweaks to get things working on the latest RHEL.  Verify it passes testing
> and release it.  Then they hope to not touch it again for a few years until
> the next RHEL comes along.
> 
> Scary, huh? :-)

Oh yeah my aim here is not to make distro or out of tree module vendors
life harder, actually the opposite. If it turns out modversions really is
the best approach, I'm not in a position to complain about its complexity
because we have Suse and Redhat people maintaining the build and module
systems :) I just want to see if we can do things better.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Thu, 1 Dec 2016 10:20:39 -0500
Don Zickus  wrote:

> On Thu, Dec 01, 2016 at 03:32:15PM +1100, Nicholas Piggin wrote:
> > > Anyway, MODVERSIONS is our way of protecting our kabi for the last 10 
> > > years.
> > > It isn't perfect and we have fixed the genksyms tool over the years, but 
> > > so
> > > far it mostly works fine.  
> > 
> > Okay. It would be good to get all the distros in on this.
> > 
> > What I want to do is work out exactly what it is that modversions is
> > giving you.
> > 
> > We know it's fairly nasty code to maintain and it does not detect ABI
> > changes very well. But it's not such a burden that we can't maintain
> > it if there are good reasons to keep it.  
> 
> Hi Nick,
> 
> I won't disagree with you there. :-)

Sorry for the late reply, I was moving house and got side tracked.

> modversions is a pretty heavy handed approach that basically says if all the
> symbols and types haven't changed for a given EXPORT_SYMBOL (recursively
> checked), then there is a high degree of confidence the OOT driver will not
> only load, but run correctly.

It's heavy handed in that it is quite complex in the kernel build system,
but it is also light handed in that it does not do a very good job.

I would say the degree of confidence is not very high. People have told
me modversions follows pointers to objects in its calculation, but I have
not seen that to be the case. Even if you did have that, it can not replace
a code review for semantics of data and code.

> The question is how to provide a similar guarantee if a different way?

As a tool to aid distro reviewers, modversions has some value, but the
debug info parsing tools that have been mentioned in this thread seem
superior (not that I've tested them).

> 
> We have plenty of customers with 10 year old drivers, where the expertise
> has long left the company.  The engineers still around, recompile and make
> tweaks to get things working on the latest RHEL.  Verify it passes testing
> and release it.  Then they hope to not touch it again for a few years until
> the next RHEL comes along.
> 
> Scary, huh? :-)

Oh yeah my aim here is not to make distro or out of tree module vendors
life harder, actually the opposite. If it turns out modversions really is
the best approach, I'm not in a position to complain about its complexity
because we have Suse and Redhat people maintaining the build and module
systems :) I just want to see if we can do things better.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Thu, 1 Dec 2016 17:12:41 +0100
Michal Marek  wrote:

> On 2016-12-01 04:39, Nicholas Piggin wrote:
> > On Thu, 01 Dec 2016 02:35:54 +
> > Ben Hutchings  wrote:  
> >> As I understand it, genksyms incorporates the definitions of a
> >> function's parameter and return types - not just their names - and all
> >> the types they refer to, recursively.  So a structure size change
> >> should change the version of all functions where the function and its
> >> caller pass that structure between them, however indirectly.  It finds
> >> such indirect ABI breakage for me fairly regularly, though of course I
> >> don't know that it finds everything.  
> > 
> > It is only the type name.
> > 
> > Not only that but even if you did extend it further to structure type
> > arrangement then you still have to deal with other structures followed
> > via pointers. Or (rarer but not unheard of):
> > 
> > - changes to structures without changes of the types of their members
> > - changes to arguments without changes of their type  
> 
> This is already covered by genksyms. Try make V=1 with
> CONFIG_MODVERSIONS=y and add the -D option to one of the genksyms
> command. I wanted to paste the expanded signature for
> register_filesystem() as an example, but vger would probably drop the
> mail for being too big :).

Well I simply tested the outcome. If you have:

struct blah {
  int x;
};
int foo(struct blah *blah)
{
  return blah->x;
}
EXPORT(foo);

$ nm vmlinux | grep __crc_foo
a0cf13a0 A __crc_foo

Now change to

struct blah {
  int y;
  int x;
};

$ nm vmlinux | grep __crc_foo
a0cf13a0 A __crc_foo

It just doesn't catch these things. Honestly, stable ABI distros *have*
to review all patches to ensure the ABI is unchanged. Some tools could
help significantly, but for that, the debug info ABI checking tools that
have been mentioned in this thread are far better tool for this job than
modversions.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Nicholas Piggin]

On Thu, 1 Dec 2016 17:12:41 +0100
Michal Marek  wrote:

> On 2016-12-01 04:39, Nicholas Piggin wrote:
> > On Thu, 01 Dec 2016 02:35:54 +
> > Ben Hutchings  wrote:  
> >> As I understand it, genksyms incorporates the definitions of a
> >> function's parameter and return types - not just their names - and all
> >> the types they refer to, recursively.  So a structure size change
> >> should change the version of all functions where the function and its
> >> caller pass that structure between them, however indirectly.  It finds
> >> such indirect ABI breakage for me fairly regularly, though of course I
> >> don't know that it finds everything.  
> > 
> > It is only the type name.
> > 
> > Not only that but even if you did extend it further to structure type
> > arrangement then you still have to deal with other structures followed
> > via pointers. Or (rarer but not unheard of):
> > 
> > - changes to structures without changes of the types of their members
> > - changes to arguments without changes of their type  
> 
> This is already covered by genksyms. Try make V=1 with
> CONFIG_MODVERSIONS=y and add the -D option to one of the genksyms
> command. I wanted to paste the expanded signature for
> register_filesystem() as an example, but vger would probably drop the
> mail for being too big :).

Well I simply tested the outcome. If you have:

struct blah {
  int x;
};
int foo(struct blah *blah)
{
  return blah->x;
}
EXPORT(foo);

$ nm vmlinux | grep __crc_foo
a0cf13a0 A __crc_foo

Now change to

struct blah {
  int y;
  int x;
};

$ nm vmlinux | grep __crc_foo
a0cf13a0 A __crc_foo

It just doesn't catch these things. Honestly, stable ABI distros *have*
to review all patches to ensure the ABI is unchanged. Some tools could
help significantly, but for that, the debug info ABI checking tools that
have been mentioned in this thread are far better tool for this job than
modversions.

Thanks,
Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Linus Torvalds]

On Sat, Dec 3, 2016 at 11:44 PM, Alan Modra  wrote:
>
> As far as reverting the binutils commit goes, I'm quite willing to do
> that if necessary

I think we have the proper fix in the kernel now, witht he "mark weak
asm symbols with value 0". Or if not "proper", then at least
acceptable. So I think the ship has sailed on the binutils change, and
there's no point in reverting it any more.

  Linus

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Linus Torvalds]

On Sat, Dec 3, 2016 at 11:44 PM, Alan Modra  wrote:
>
> As far as reverting the binutils commit goes, I'm quite willing to do
> that if necessary

I think we have the proper fix in the kernel now, witht he "mark weak
asm symbols with value 0". Or if not "proper", then at least
acceptable. So I think the ship has sailed on the binutils change, and
there's no point in reverting it any more.

  Linus

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Alan Modra]

On Fri, Dec 02, 2016 at 11:55:58AM +0100, Arnd Bergmann wrote:
> I have managed to bisect the link failure to a specific binutils
> commit by Alan Modra now:
> 
> d983c8c ("Strip undefined symbols from .symtab")
> 
> went into binutils-2_26 and was reverted in 
> 
> a82e3ef ("Revert "Strip undefined symbols from .symtab"")
> 
> after the release branch for 2.26 was started, so that version
> ended up being fine. However, the 2.27 version never saw the revert
> and causes loadable kernel modules to become unusable when they refer
> to a weak symbol in vmlinux. This works with 2.26 and lower:

See https://sourceware.org/ml/binutils/2016-01/msg00118.html thread
for discussion on why the patch was reverted for 2.26.  At the time I
believed that only the ppc64 kernel was affected, by a weak undefined
"__crc_TOC." disappearing.  Am I correct in thinking that remained
true up until Linus' merge commit 84d69848c9 2016-10-14?

As far as reverting the binutils commit goes, I'm quite willing to do
that if necessary.  You can see how important I think the fix was by
viewing https://sourceware.org/bugzilla/show_bug.cgi?id=4317 and
noticing that the bug was reported in 2007 and didn't see any action
for 8 years..

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Alan Modra]

On Fri, Dec 02, 2016 at 11:55:58AM +0100, Arnd Bergmann wrote:
> I have managed to bisect the link failure to a specific binutils
> commit by Alan Modra now:
> 
> d983c8c ("Strip undefined symbols from .symtab")
> 
> went into binutils-2_26 and was reverted in 
> 
> a82e3ef ("Revert "Strip undefined symbols from .symtab"")
> 
> after the release branch for 2.26 was started, so that version
> ended up being fine. However, the 2.27 version never saw the revert
> and causes loadable kernel modules to become unusable when they refer
> to a weak symbol in vmlinux. This works with 2.26 and lower:

See https://sourceware.org/ml/binutils/2016-01/msg00118.html thread
for discussion on why the patch was reverted for 2.26.  At the time I
believed that only the ppc64 kernel was affected, by a weak undefined
"__crc_TOC." disappearing.  Am I correct in thinking that remained
true up until Linus' merge commit 84d69848c9 2016-10-14?

As far as reverting the binutils commit goes, I'm quite willing to do
that if necessary.  You can see how important I think the fix was by
viewing https://sourceware.org/bugzilla/show_bug.cgi?id=4317 and
noticing that the bug was reported in 2007 and didn't see any action
for 8 years..

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Linus Torvalds]

On Fri, Dec 2, 2016 at 2:55 AM, Arnd Bergmann  wrote:
>
> Yes, it's always been just the assembly symbols that broke, these were
> the ones that Al's original patch changed and that ended up with
> no version information.

Ok, and the reason is because even if we have a weak symbol from C, it
would always have a value.

Good to know what the heck the problem was.

> I have managed to bisect the link failure to a specific binutils
> commit by Alan Modra now:

Thanks. And I committed your "set to zero" patch, so we can hopefully
leave this all behind us.

I marked it for stable (not because older kernels need it, but because
it's the right thing to do and if we ever backport anything that
causes this we don't want to forget this).

And I'll keep the workaround in kernel/module.c just because it's
really late in the rc series, and I'd rather have both belt and
suspenders for this all right now.

   Linus

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Linus Torvalds]

On Fri, Dec 2, 2016 at 2:55 AM, Arnd Bergmann  wrote:
>
> Yes, it's always been just the assembly symbols that broke, these were
> the ones that Al's original patch changed and that ended up with
> no version information.

Ok, and the reason is because even if we have a weak symbol from C, it
would always have a value.

Good to know what the heck the problem was.

> I have managed to bisect the link failure to a specific binutils
> commit by Alan Modra now:

Thanks. And I committed your "set to zero" patch, so we can hopefully
leave this all behind us.

I marked it for stable (not because older kernels need it, but because
it's the right thing to do and if we ever backport anything that
causes this we don't want to forget this).

And I'll keep the workaround in kernel/module.c just because it's
really late in the rc series, and I'd rather have both belt and
suspenders for this all right now.

   Linus

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Hannes Frederic Sowa]

On 01.12.2016 17:12, Michal Marek wrote:
> On 2016-12-01 04:39, Nicholas Piggin wrote:
>> On Thu, 01 Dec 2016 02:35:54 +
>> Ben Hutchings  wrote:
>>> As I understand it, genksyms incorporates the definitions of a
>>> function's parameter and return types - not just their names - and all
>>> the types they refer to, recursively.  So a structure size change
>>> should change the version of all functions where the function and its
>>> caller pass that structure between them, however indirectly.  It finds
>>> such indirect ABI breakage for me fairly regularly, though of course I
>>> don't know that it finds everything.
>>
>> It is only the type name.
>>
>> Not only that but even if you did extend it further to structure type
>> arrangement then you still have to deal with other structures followed
>> via pointers. Or (rarer but not unheard of):
>>
>> - changes to structures without changes of the types of their members
>> - changes to arguments without changes of their type
> 
> This is already covered by genksyms. Try make V=1 with
> CONFIG_MODVERSIONS=y and add the -D option to one of the genksyms
> command. I wanted to paste the expanded signature for
> register_filesystem() as an example, but vger would probably drop the
> mail for being too big :).

It is easier to just use e.g. `make net/core/dev.symtypes` and look at
the generated file.

Bye,
Hannes

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Hannes Frederic Sowa]

On 01.12.2016 17:12, Michal Marek wrote:
> On 2016-12-01 04:39, Nicholas Piggin wrote:
>> On Thu, 01 Dec 2016 02:35:54 +
>> Ben Hutchings  wrote:
>>> As I understand it, genksyms incorporates the definitions of a
>>> function's parameter and return types - not just their names - and all
>>> the types they refer to, recursively.  So a structure size change
>>> should change the version of all functions where the function and its
>>> caller pass that structure between them, however indirectly.  It finds
>>> such indirect ABI breakage for me fairly regularly, though of course I
>>> don't know that it finds everything.
>>
>> It is only the type name.
>>
>> Not only that but even if you did extend it further to structure type
>> arrangement then you still have to deal with other structures followed
>> via pointers. Or (rarer but not unheard of):
>>
>> - changes to structures without changes of the types of their members
>> - changes to arguments without changes of their type
> 
> This is already covered by genksyms. Try make V=1 with
> CONFIG_MODVERSIONS=y and add the -D option to one of the genksyms
> command. I wanted to paste the expanded signature for
> register_filesystem() as an example, but vger would probably drop the
> mail for being too big :).

It is easier to just use e.g. `make net/core/dev.symtypes` and look at
the generated file.

Bye,
Hannes

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Arnd Bergmann]

On Thursday, December 1, 2016 10:26:07 AM CET Linus Torvalds wrote:
> On Thu, Dec 1, 2016 at 5:58 AM, Arnd Bergmann  wrote:
> >
> > WARNING: EXPORT symbol "mcount" [arch/x86/entry/built-in.ko] version 
> > generation failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "mcount" [arch/x86/built-in.ko] version generation 
> > failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> > symbol will not be versioned.
> > WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation 
> > failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> > will not be versioned.
> > WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> > symbol will not be versioned.
> > WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation 
> > failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> > will not be versioned.
> >
> > Out of 12 randconfig builds that had CONFIG_MODVERSIONS enabled, all 12
> > had this problem, though not always with all the symbols.
> 
> Well, the good news is that pretty fundamentally, if it's just the asm
> symbls, those really don't have ABI's that change (or if they change,
> it's such a fundamental change that everything else will likely have
> changed too and we don't need to worry about one asm symbol crc - the
> change will be caught by other symbols).

Yes, it's always been just the assembly symbols that broke, these were
the ones that Al's original patch changed and that ended up with
no version information.

> So I think the whole "we don't really care" approach should work fine.
> The "let's make every symbol always be versioned" may just be too much
> pain for no real gain.

I have managed to bisect the link failure to a specific binutils
commit by Alan Modra now:

d983c8c ("Strip undefined symbols from .symtab")

went into binutils-2_26 and was reverted in 

a82e3ef ("Revert "Strip undefined symbols from .symtab"")

after the release branch for 2.26 was started, so that version
ended up being fine. However, the 2.27 version never saw the revert
and causes loadable kernel modules to become unusable when they refer
to a weak symbol in vmlinux. This works with 2.26 and lower:

$ nm obj-arm/vmlinux | grep __gnu_mcount_nc
 w __crc___gnu_mcount_nc
c031ed58 T __gnu_mcount_nc
c0ef4590 r __kcrctab___gnu_mcount_nc
c0efb4f5 r __kstrtab___gnu_mcount_nc
c0ee68bc R __ksymtab___gnu_mcount_nc

and this is breaks with 2.27 and higher:
$ make modules 2>&1 | tail -n 1
WARNING: "__gnu_mcount_nc" [drivers/ata/ahci_platform.ko] has no CRC!

$ nm obj-arm/vmlinux | grep __gnu_mcount_nc
c031ed58 T __gnu_mcount_nc
c0ef4590 r __kcrctab___gnu_mcount_nc
c0efb4f5 r __kstrtab___gnu_mcount_nc
c0ee68bc R __ksymtab___gnu_mcount_nc

Applying the patch below on top of today's binutils master branch
makes it work again. The real patch will also have to fix the testsuite.

Arnd

---
commit 9a48071533f311ff1f567361874fab141bd77230
Author: Arnd Bergmann 
Date:   Fri Dec 2 11:31:34 2016 +0100

Revert "Strip undefined symbols from .symtab"

This reverts commit d983c8c5503d680c6d4955ceb610a9beebc64460.

Signed-off-by: Arnd Bergmann 

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 5f87f87..57c4d42 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -9354,9 +9354,8 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void 
*data)
  a regular file, or that we have been told to strip.  However, if
  h->indx is set to -2, the symbol is used by a reloc and we must
  output it.  */
-  strip = FALSE;
   if (h->indx == -2)
-;
+strip = FALSE;
   else if ((h->def_dynamic
|| h->ref_dynamic
|| h->root.type == bfd_link_hash_new)
@@ -9382,13 +9381,14 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void 
*data)
   && h->root.u.undef.abfd != NULL
   && (h->root.u.undef.abfd->flags & BFD_PLUGIN) != 0)
 strip = TRUE;
+  else
+strip = FALSE;
 
   type = h->type;
 
   /* If we're stripping it, and it's not a dynamic symbol, there's
- nothing else to do.   However, if it is a forced local symbol or
- an ifunc symbol we need to give the backend finish_dynamic_symbol
- function a chance to make it dynamic.  */
+ nothing else to do unless it is a forced local symbol or a
+ STT_GNU_IFUNC symbol.  */
   if (strip
   && h->dynindx == -1
   && type != STT_GNU_IFUNC
@@ -9691,18 +9691,9 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void 
*data)
}
 }
 
-  /* If the symbol is undefined, and we didn't output it to .dynsym,
- strip it from .symtab too.  Obviously we can't do this for
- relocatable output or when needed for --emit-relocs.  */
-  else if (input_sec == bfd_und_section_ptr
-  && h->indx != -2

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Arnd Bergmann]

On Thursday, December 1, 2016 10:26:07 AM CET Linus Torvalds wrote:
> On Thu, Dec 1, 2016 at 5:58 AM, Arnd Bergmann  wrote:
> >
> > WARNING: EXPORT symbol "mcount" [arch/x86/entry/built-in.ko] version 
> > generation failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "mcount" [arch/x86/built-in.ko] version generation 
> > failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> > symbol will not be versioned.
> > WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation 
> > failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> > will not be versioned.
> > WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> > symbol will not be versioned.
> > WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation 
> > failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> > will not be versioned.
> >
> > Out of 12 randconfig builds that had CONFIG_MODVERSIONS enabled, all 12
> > had this problem, though not always with all the symbols.
> 
> Well, the good news is that pretty fundamentally, if it's just the asm
> symbls, those really don't have ABI's that change (or if they change,
> it's such a fundamental change that everything else will likely have
> changed too and we don't need to worry about one asm symbol crc - the
> change will be caught by other symbols).

Yes, it's always been just the assembly symbols that broke, these were
the ones that Al's original patch changed and that ended up with
no version information.

> So I think the whole "we don't really care" approach should work fine.
> The "let's make every symbol always be versioned" may just be too much
> pain for no real gain.

I have managed to bisect the link failure to a specific binutils
commit by Alan Modra now:

d983c8c ("Strip undefined symbols from .symtab")

went into binutils-2_26 and was reverted in 

a82e3ef ("Revert "Strip undefined symbols from .symtab"")

after the release branch for 2.26 was started, so that version
ended up being fine. However, the 2.27 version never saw the revert
and causes loadable kernel modules to become unusable when they refer
to a weak symbol in vmlinux. This works with 2.26 and lower:

$ nm obj-arm/vmlinux | grep __gnu_mcount_nc
 w __crc___gnu_mcount_nc
c031ed58 T __gnu_mcount_nc
c0ef4590 r __kcrctab___gnu_mcount_nc
c0efb4f5 r __kstrtab___gnu_mcount_nc
c0ee68bc R __ksymtab___gnu_mcount_nc

and this is breaks with 2.27 and higher:
$ make modules 2>&1 | tail -n 1
WARNING: "__gnu_mcount_nc" [drivers/ata/ahci_platform.ko] has no CRC!

$ nm obj-arm/vmlinux | grep __gnu_mcount_nc
c031ed58 T __gnu_mcount_nc
c0ef4590 r __kcrctab___gnu_mcount_nc
c0efb4f5 r __kstrtab___gnu_mcount_nc
c0ee68bc R __ksymtab___gnu_mcount_nc

Applying the patch below on top of today's binutils master branch
makes it work again. The real patch will also have to fix the testsuite.

Arnd

---
commit 9a48071533f311ff1f567361874fab141bd77230
Author: Arnd Bergmann 
Date:   Fri Dec 2 11:31:34 2016 +0100

Revert "Strip undefined symbols from .symtab"

This reverts commit d983c8c5503d680c6d4955ceb610a9beebc64460.

Signed-off-by: Arnd Bergmann 

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 5f87f87..57c4d42 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -9354,9 +9354,8 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void 
*data)
  a regular file, or that we have been told to strip.  However, if
  h->indx is set to -2, the symbol is used by a reloc and we must
  output it.  */
-  strip = FALSE;
   if (h->indx == -2)
-;
+strip = FALSE;
   else if ((h->def_dynamic
|| h->ref_dynamic
|| h->root.type == bfd_link_hash_new)
@@ -9382,13 +9381,14 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void 
*data)
   && h->root.u.undef.abfd != NULL
   && (h->root.u.undef.abfd->flags & BFD_PLUGIN) != 0)
 strip = TRUE;
+  else
+strip = FALSE;
 
   type = h->type;
 
   /* If we're stripping it, and it's not a dynamic symbol, there's
- nothing else to do.   However, if it is a forced local symbol or
- an ifunc symbol we need to give the backend finish_dynamic_symbol
- function a chance to make it dynamic.  */
+ nothing else to do unless it is a forced local symbol or a
+ STT_GNU_IFUNC symbol.  */
   if (strip
   && h->dynindx == -1
   && type != STT_GNU_IFUNC
@@ -9691,18 +9691,9 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void 
*data)
}
 }
 
-  /* If the symbol is undefined, and we didn't output it to .dynsym,
- strip it from .symtab too.  Obviously we can't do this for
- relocatable output or when needed for --emit-relocs.  */
-  else if (input_sec == bfd_und_section_ptr
-  && h->indx != -2
-  && !bfd_link_relocatable 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Don Zickus]

On Thu, Dec 01, 2016 at 05:06:11PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Dec 01, 2016 at 10:40:59AM -0500, Don Zickus wrote:
> > Unfortunately, there are various drivers that will never go upstream
> > 
> > - paid storage drivers that provide bells and whistles on top of inbox
> >   driver
> 
> That's because the developer doesn't want them upstream, that's their
> fault, nothing we can do about them.
> 
> > - old drivers/fs that application has been relying on for a long time but
> >   company doesn't have resources to migrate to current technology.
> 
> That's what drivers/staging/ is for, I'll take anything that builds (and
> sometimes stuff that doesn't build) as long as people are actually using
> it.  So send the stuff that is in this category on to me and that will
> reduce your burden a _lot_.

Hi Greg,

I will forward this offer to the right folks and see who we can get to bite.
:-)  Thanks!

Cheers,
Don

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Don Zickus]

On Thu, Dec 01, 2016 at 05:06:11PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Dec 01, 2016 at 10:40:59AM -0500, Don Zickus wrote:
> > Unfortunately, there are various drivers that will never go upstream
> > 
> > - paid storage drivers that provide bells and whistles on top of inbox
> >   driver
> 
> That's because the developer doesn't want them upstream, that's their
> fault, nothing we can do about them.
> 
> > - old drivers/fs that application has been relying on for a long time but
> >   company doesn't have resources to migrate to current technology.
> 
> That's what drivers/staging/ is for, I'll take anything that builds (and
> sometimes stuff that doesn't build) as long as people are actually using
> it.  So send the stuff that is in this category on to me and that will
> reduce your burden a _lot_.

Hi Greg,

I will forward this offer to the right folks and see who we can get to bite.
:-)  Thanks!

Cheers,
Don

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Linus Torvalds]

On Thu, Dec 1, 2016 at 5:58 AM, Arnd Bergmann  wrote:
>
> WARNING: EXPORT symbol "mcount" [arch/x86/entry/built-in.ko] version 
> generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [arch/x86/built-in.ko] version generation 
> failed, symbol will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> will not be versioned.
>
> Out of 12 randconfig builds that had CONFIG_MODVERSIONS enabled, all 12
> had this problem, though not always with all the symbols.

Well, the good news is that pretty fundamentally, if it's just the asm
symbls, those really don't have ABI's that change (or if they change,
it's such a fundamental change that everything else will likely have
changed too and we don't need to worry about one asm symbol crc - the
change will be caught by other symbols).

So I think the whole "we don't really care" approach should work fine.
The "let's make every symbol always be versioned" may just be too much
pain for no real gain.

Linus

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Linus Torvalds]

On Thu, Dec 1, 2016 at 5:58 AM, Arnd Bergmann  wrote:
>
> WARNING: EXPORT symbol "mcount" [arch/x86/entry/built-in.ko] version 
> generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [arch/x86/built-in.ko] version generation 
> failed, symbol will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> will not be versioned.
>
> Out of 12 randconfig builds that had CONFIG_MODVERSIONS enabled, all 12
> had this problem, though not always with all the symbols.

Well, the good news is that pretty fundamentally, if it's just the asm
symbls, those really don't have ABI's that change (or if they change,
it's such a fundamental change that everything else will likely have
changed too and we don't need to worry about one asm symbol crc - the
change will be caught by other symbols).

So I think the whole "we don't really care" approach should work fine.
The "let's make every symbol always be versioned" may just be too much
pain for no real gain.

Linus

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-01 14:58, Arnd Bergmann wrote:
> On Tuesday, November 29, 2016 9:14:46 AM CET Linus Torvalds wrote:
>> On Tue, Nov 29, 2016 at 9:10 AM, Linus Torvalds
>>  wrote:
>>>
>>> So quite frankly, I don't want to make our kernel sources worse due to
>>> broken shit tools getting something wrong that we shouldn't even care
>>> about.
>>
>> And yes, I'm on binutils 2.26 (with no issues), so it could be that
>> it's 2.27 that triggers this.
>>
>> We could make the pr_warn_once() mention "broken binutils?" so that
>> people know why the warning happens.
> 
> I've tried to get to the bottom of this, but couldn't find anything
> related to the toolchain version. I've tried binutils 2.23, 2.24, 2.26
> and 2.27, and also gcc-7.0, gcc-5.4.1 and gcc-4.9.3, but with today's
> linux-next, I always get
> 
> WARNING: EXPORT symbol "mcount" [arch/x86/entry/built-in.ko] version 
> generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [arch/x86/built-in.ko] version generation 
> failed, symbol will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> will not be versioned.
> 
> Out of 12 randconfig builds that had CONFIG_MODVERSIONS enabled, all 12
> had this problem, though not always with all the symbols.

That we are not generating modversions for the asm exports is a known
problem, independent of the toolchain. The problem with some toolchains
(presumably, because that's the next thing to blame if the source and
the .config is identical) is that the CRCs do not appear as 0 in the
___kcrctab/___kcrctab_gpl section.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-01 14:58, Arnd Bergmann wrote:
> On Tuesday, November 29, 2016 9:14:46 AM CET Linus Torvalds wrote:
>> On Tue, Nov 29, 2016 at 9:10 AM, Linus Torvalds
>>  wrote:
>>>
>>> So quite frankly, I don't want to make our kernel sources worse due to
>>> broken shit tools getting something wrong that we shouldn't even care
>>> about.
>>
>> And yes, I'm on binutils 2.26 (with no issues), so it could be that
>> it's 2.27 that triggers this.
>>
>> We could make the pr_warn_once() mention "broken binutils?" so that
>> people know why the warning happens.
> 
> I've tried to get to the bottom of this, but couldn't find anything
> related to the toolchain version. I've tried binutils 2.23, 2.24, 2.26
> and 2.27, and also gcc-7.0, gcc-5.4.1 and gcc-4.9.3, but with today's
> linux-next, I always get
> 
> WARNING: EXPORT symbol "mcount" [arch/x86/entry/built-in.ko] version 
> generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [arch/x86/built-in.ko] version generation 
> failed, symbol will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, 
> symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol 
> will not be versioned.
> 
> Out of 12 randconfig builds that had CONFIG_MODVERSIONS enabled, all 12
> had this problem, though not always with all the symbols.

That we are not generating modversions for the asm exports is a known
problem, independent of the toolchain. The problem with some toolchains
(presumably, because that's the next thing to blame if the source and
the .config is identical) is that the CRCs do not appear as 0 in the
___kcrctab/___kcrctab_gpl section.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-01 05:13, Don Zickus wrote:
> Sorry for chiming in late, but yes RHEL is a big user of MODVERSIONS for our
> kabi protection work.  Despite our best intentions we still have lots of
> partners and customers that provide value-add out-of-tree drivers to their
> customers.  These module builders requested we have a mechanism to allow
> rolling modules forward for each of our minor RHEL updates without breaking
> their drivers.

FWIW, in SLES we use CONFIG_MODVERSION for pretty much the same reasons
you listed. We also enable it in openSUSE, but there it's not as crucial.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-01 05:13, Don Zickus wrote:
> Sorry for chiming in late, but yes RHEL is a big user of MODVERSIONS for our
> kabi protection work.  Despite our best intentions we still have lots of
> partners and customers that provide value-add out-of-tree drivers to their
> customers.  These module builders requested we have a mechanism to allow
> rolling modules forward for each of our minor RHEL updates without breaking
> their drivers.

FWIW, in SLES we use CONFIG_MODVERSION for pretty much the same reasons
you listed. We also enable it in openSUSE, but there it's not as crucial.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-01 04:39, Nicholas Piggin wrote:
> On Thu, 01 Dec 2016 02:35:54 +
> Ben Hutchings  wrote:
>> As I understand it, genksyms incorporates the definitions of a
>> function's parameter and return types - not just their names - and all
>> the types they refer to, recursively.  So a structure size change
>> should change the version of all functions where the function and its
>> caller pass that structure between them, however indirectly.  It finds
>> such indirect ABI breakage for me fairly regularly, though of course I
>> don't know that it finds everything.
> 
> It is only the type name.
> 
> Not only that but even if you did extend it further to structure type
> arrangement then you still have to deal with other structures followed
> via pointers. Or (rarer but not unheard of):
> 
> - changes to structures without changes of the types of their members
> - changes to arguments without changes of their type

This is already covered by genksyms. Try make V=1 with
CONFIG_MODVERSIONS=y and add the -D option to one of the genksyms
command. I wanted to paste the expanded signature for
register_filesystem() as an example, but vger would probably drop the
mail for being too big :).


> - changes to semantics of functions
> - data structures derived in ways other than exported symbols, e.g.,
>   fixed register for `current` on some archs

Right, this is something that genksyms has no idea about.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Michal Marek]

On 2016-12-01 04:39, Nicholas Piggin wrote:
> On Thu, 01 Dec 2016 02:35:54 +
> Ben Hutchings  wrote:
>> As I understand it, genksyms incorporates the definitions of a
>> function's parameter and return types - not just their names - and all
>> the types they refer to, recursively.  So a structure size change
>> should change the version of all functions where the function and its
>> caller pass that structure between them, however indirectly.  It finds
>> such indirect ABI breakage for me fairly regularly, though of course I
>> don't know that it finds everything.
> 
> It is only the type name.
> 
> Not only that but even if you did extend it further to structure type
> arrangement then you still have to deal with other structures followed
> via pointers. Or (rarer but not unheard of):
> 
> - changes to structures without changes of the types of their members
> - changes to arguments without changes of their type

This is already covered by genksyms. Try make V=1 with
CONFIG_MODVERSIONS=y and add the -D option to one of the genksyms
command. I wanted to paste the expanded signature for
register_filesystem() as an example, but vger would probably drop the
mail for being too big :).


> - changes to semantics of functions
> - data structures derived in ways other than exported symbols, e.g.,
>   fixed register for `current` on some archs

Right, this is something that genksyms has no idea about.

Michal

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Greg Kroah-Hartman]

On Thu, Dec 01, 2016 at 10:40:59AM -0500, Don Zickus wrote:
> Unfortunately, there are various drivers that will never go upstream
> 
> - paid storage drivers that provide bells and whistles on top of inbox
>   driver

That's because the developer doesn't want them upstream, that's their
fault, nothing we can do about them.

> - old drivers/fs that application has been relying on for a long time but
>   company doesn't have resources to migrate to current technology.

That's what drivers/staging/ is for, I'll take anything that builds (and
sometimes stuff that doesn't build) as long as people are actually using
it.  So send the stuff that is in this category on to me and that will
reduce your burden a _lot_.

thanks,

greg k-h

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Greg Kroah-Hartman]

On Thu, Dec 01, 2016 at 10:40:59AM -0500, Don Zickus wrote:
> Unfortunately, there are various drivers that will never go upstream
> 
> - paid storage drivers that provide bells and whistles on top of inbox
>   driver

That's because the developer doesn't want them upstream, that's their
fault, nothing we can do about them.

> - old drivers/fs that application has been relying on for a long time but
>   company doesn't have resources to migrate to current technology.

That's what drivers/staging/ is for, I'll take anything that builds (and
sometimes stuff that doesn't build) as long as people are actually using
it.  So send the stuff that is in this category on to me and that will
reduce your burden a _lot_.

thanks,

greg k-h

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Don Zickus]

On Thu, Dec 01, 2016 at 07:26:09AM -0800, Christoph Hellwig wrote:
> On Thu, Dec 01, 2016 at 10:20:39AM -0500, Don Zickus wrote:
> > 
> > - provide the memory allocation (instead of having the driver staticly
> >   allocate)
> > - provide functions to retrieve various internal data (instead of having the
> >   driver do direct referencing to deep internal elements)
> > - cut down on some static inlines (and use accessory functions instead),
> >   etc.
> > 
> > Those types of changes allow the OOT driver to be more ignorant of kernel
> > changes and struct modifications.
> 
> All that is counter to what we really want to have:  a well integrated
> kernel that moves forward together so that we can see and improve the
> whole situation.  No need to make things worse just to help leeches.
> Get your damn drivers upstream ASAP and let's stop this discussion..

I understand and won't disagree with you. :-)

Unfortunately, there are various drivers that will never go upstream

- paid storage drivers that provide bells and whistles on top of inbox
  driver
- old drivers/fs that application has been relying on for a long time but
  company doesn't have resources to migrate to current technology.

We have been trying over the years to do what we can to move customers in
the right direction.  It is just a slow process, sadly.

Cheers,
Don

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Don Zickus]

On Thu, Dec 01, 2016 at 07:26:09AM -0800, Christoph Hellwig wrote:
> On Thu, Dec 01, 2016 at 10:20:39AM -0500, Don Zickus wrote:
> > 
> > - provide the memory allocation (instead of having the driver staticly
> >   allocate)
> > - provide functions to retrieve various internal data (instead of having the
> >   driver do direct referencing to deep internal elements)
> > - cut down on some static inlines (and use accessory functions instead),
> >   etc.
> > 
> > Those types of changes allow the OOT driver to be more ignorant of kernel
> > changes and struct modifications.
> 
> All that is counter to what we really want to have:  a well integrated
> kernel that moves forward together so that we can see and improve the
> whole situation.  No need to make things worse just to help leeches.
> Get your damn drivers upstream ASAP and let's stop this discussion..

I understand and won't disagree with you. :-)

Unfortunately, there are various drivers that will never go upstream

- paid storage drivers that provide bells and whistles on top of inbox
  driver
- old drivers/fs that application has been relying on for a long time but
  company doesn't have resources to migrate to current technology.

We have been trying over the years to do what we can to move customers in
the right direction.  It is just a slow process, sadly.

Cheers,
Don

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Nicholas Piggin  a écrit:

[...]

> On Thu, 1 Dec 2016 11:48:09 +0100
> Stanislav Kozina  wrote:
>
>> On 12/01/2016 05:13 AM, Don Zickus wrote:
>> 
>> ...
>> 
>> > I think GregKH pointed to one such tool, libabigail?  We are working on
>> > others too.  
>> 
>> I should mention one of the others here:
>> https://github.com/skozina/kabi-dw

[...]

> Now this seems much better for distro ABI checking.

Right, Incidentally, Fedora does distro-wide ABI verfication for
userspace libraries updates in a given stable distribution.  You can
look at an example of output of the libabigail-based tool that compares
a new version of an ELF library to it's latest stable version:

https://taskotron.fedoraproject.org/artifacts/all/a55cbac8-ab64-11e6-94e0-525400120b80/task_output/curl-7.51.0-2.fc25.log

The results of those ABI comparison can browsed at 
https://taskotron.fedoraproject.org/resultsdb/results?testcase_name=dist.abicheck.

Of course, you can run the comparison yourself by using a
libabigail-based tool like
https://sourceware.org/libabigail/manual/abipkgdiff.html which takes
.deb and .rpm packages.

We are currently working on making libabigail-based tools understand
Linux kernel specifities so that we can run that kind of analysis on
Kernel updates too.  Ideally, when this is done, you should be able to
just use abipkgdiff on two Linux Kernel packages and get the same kind
of output.

> The next question is, do they need any kernel support for rare cases
> where they do have to break the ABI of an export? Simple rename of the
> function with a _v2 postfix might be enough. We could retain some per
> symbol versioning in the kernel if needed, but how much would it
> actually help?

As a reviewer of the ABI change report emitted by the tool, if what you
want is to say "I reviewed that ABI change and it's OK, so please do not
show it to me next time", then you can feed the tool with a "suppression
specification".  It's a text file in the INI syntax in which you can
specify the kind of change you want the tool to suppress from its
output: https://sourceware.org/libabigail/manual/libabigail-concepts.html.

So I don't think you need to do anything to the source code of the
Kernel in the cases where you need to change the ABI.  Just tell the
tool about that change.

Cheers,

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Dodji Seketeli]

Nicholas Piggin  a écrit:

[...]

> On Thu, 1 Dec 2016 11:48:09 +0100
> Stanislav Kozina  wrote:
>
>> On 12/01/2016 05:13 AM, Don Zickus wrote:
>> 
>> ...
>> 
>> > I think GregKH pointed to one such tool, libabigail?  We are working on
>> > others too.  
>> 
>> I should mention one of the others here:
>> https://github.com/skozina/kabi-dw

[...]

> Now this seems much better for distro ABI checking.

Right, Incidentally, Fedora does distro-wide ABI verfication for
userspace libraries updates in a given stable distribution.  You can
look at an example of output of the libabigail-based tool that compares
a new version of an ELF library to it's latest stable version:

https://taskotron.fedoraproject.org/artifacts/all/a55cbac8-ab64-11e6-94e0-525400120b80/task_output/curl-7.51.0-2.fc25.log

The results of those ABI comparison can browsed at 
https://taskotron.fedoraproject.org/resultsdb/results?testcase_name=dist.abicheck.

Of course, you can run the comparison yourself by using a
libabigail-based tool like
https://sourceware.org/libabigail/manual/abipkgdiff.html which takes
.deb and .rpm packages.

We are currently working on making libabigail-based tools understand
Linux kernel specifities so that we can run that kind of analysis on
Kernel updates too.  Ideally, when this is done, you should be able to
just use abipkgdiff on two Linux Kernel packages and get the same kind
of output.

> The next question is, do they need any kernel support for rare cases
> where they do have to break the ABI of an export? Simple rename of the
> function with a _v2 postfix might be enough. We could retain some per
> symbol versioning in the kernel if needed, but how much would it
> actually help?

As a reviewer of the ABI change report emitted by the tool, if what you
want is to say "I reviewed that ABI change and it's OK, so please do not
show it to me next time", then you can feed the tool with a "suppression
specification".  It's a text file in the INI syntax in which you can
specify the kind of change you want the tool to suppress from its
output: https://sourceware.org/libabigail/manual/libabigail-concepts.html.

So I don't think you need to do anything to the source code of the
Kernel in the cases where you need to change the ABI.  Just tell the
tool about that change.

Cheers,

-- 
Dodji

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Christoph Hellwig]

On Thu, Dec 01, 2016 at 10:20:39AM -0500, Don Zickus wrote:
> 
> - provide the memory allocation (instead of having the driver staticly
>   allocate)
> - provide functions to retrieve various internal data (instead of having the
>   driver do direct referencing to deep internal elements)
> - cut down on some static inlines (and use accessory functions instead),
>   etc.
> 
> Those types of changes allow the OOT driver to be more ignorant of kernel
> changes and struct modifications.

All that is counter to what we really want to have:  a well integrated
kernel that moves forward together so that we can see and improve the
whole situation.  No need to make things worse just to help leeches.
Get your damn drivers upstream ASAP and let's stop this discussion..

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Christoph Hellwig]

On Thu, Dec 01, 2016 at 10:20:39AM -0500, Don Zickus wrote:
> 
> - provide the memory allocation (instead of having the driver staticly
>   allocate)
> - provide functions to retrieve various internal data (instead of having the
>   driver do direct referencing to deep internal elements)
> - cut down on some static inlines (and use accessory functions instead),
>   etc.
> 
> Those types of changes allow the OOT driver to be more ignorant of kernel
> changes and struct modifications.

All that is counter to what we really want to have:  a well integrated
kernel that moves forward together so that we can see and improve the
whole situation.  No need to make things worse just to help leeches.
Get your damn drivers upstream ASAP and let's stop this discussion..

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Don Zickus]

On Thu, Dec 01, 2016 at 03:32:15PM +1100, Nicholas Piggin wrote:
> > Anyway, MODVERSIONS is our way of protecting our kabi for the last 10 years.
> > It isn't perfect and we have fixed the genksyms tool over the years, but so
> > far it mostly works fine.
> 
> Okay. It would be good to get all the distros in on this.
> 
> What I want to do is work out exactly what it is that modversions is
> giving you.
> 
> We know it's fairly nasty code to maintain and it does not detect ABI
> changes very well. But it's not such a burden that we can't maintain
> it if there are good reasons to keep it.

Hi Nick,

I won't disagree with you there. :-)

modversions is a pretty heavy handed approach that basically says if all the
symbols and types haven't changed for a given EXPORT_SYMBOL (recursively
checked), then there is a high degree of confidence the OOT driver will not
only load, but run correctly.

The question is how to provide a similar guarantee if a different way?

We have plenty of customers with 10 year old drivers, where the expertise
has long left the company.  The engineers still around, recompile and make
tweaks to get things working on the latest RHEL.  Verify it passes testing
and release it.  Then they hope to not touch it again for a few years until
the next RHEL comes along.

Scary, huh? :-)

Common examples, filesystems and storage drivers.


There is no way that I see to provide a 100% guarantee, but if we do enough
checks, we should be able to have a high degree of confidence the driver
won't blow up.

On the flip side, easy things in the kernel to do is:

- provide the memory allocation (instead of having the driver staticly
  allocate)
- provide functions to retrieve various internal data (instead of having the
  driver do direct referencing to deep internal elements)
- cut down on some static inlines (and use accessory functions instead),
  etc.

Those types of changes allow the OOT driver to be more ignorant of kernel
changes and struct modifications.


Look to Stanislav's responses for his ideas on new tooling.

Thanks for helping!

Cheers,
Don


> 
> > I am not sure what 'control vermagic' is, but it sounds like a string check,
> > which won't protect against the boatload of backports we do to structs,
> > enums, and functions.
> 
> Basically vermagic is the string all modules and the kernel get, which
> must match in order to load modules. If you have modversions disabled,
> then vermagic includes the kernel version. If modversions is enabled,
> then vermagic does not include the kernel version but the CRCs have to
> also match.
> 
> Controlling it explicitly is just a couple of lines where a distro can
> control it (so they can update their kernel version without breaking).
> It's not meant to solve everything, just the first one.
>  
> > Currently we are exploring various ways to get smarter here.  The genksyms
> > tool has its limitations and handling kabi hacks in RHEL is getting
> > tiresome.
> > 
> > I think GregKH pointed to one such tool, libabigail?  We are working on
> > others too.
> > 
> > 
> > Circling back to enabling MODVERSIONS in Fedora, that was to start the
> > process of syncing Fedora with RHEL stuff in preparation for smarter tools.
> > 
> > 
> > If you take away MODVERSIONS, that would put a damper in our work, but
> > easily carried privately (much like MODSIGNING for 8 years until it went
> > upstream :-) ).
> 
> I don't think that's necessary. A feature requirement for a distro is just
> as valid as any other user of upstream. I don't want to hinder any distro,
> I'm just still not quite seeing the big picture of exactly what functionality
> you need from the kernel.
> 
> Thanks,
> Nick

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Don Zickus]

On Thu, Dec 01, 2016 at 03:32:15PM +1100, Nicholas Piggin wrote:
> > Anyway, MODVERSIONS is our way of protecting our kabi for the last 10 years.
> > It isn't perfect and we have fixed the genksyms tool over the years, but so
> > far it mostly works fine.
> 
> Okay. It would be good to get all the distros in on this.
> 
> What I want to do is work out exactly what it is that modversions is
> giving you.
> 
> We know it's fairly nasty code to maintain and it does not detect ABI
> changes very well. But it's not such a burden that we can't maintain
> it if there are good reasons to keep it.

Hi Nick,

I won't disagree with you there. :-)

modversions is a pretty heavy handed approach that basically says if all the
symbols and types haven't changed for a given EXPORT_SYMBOL (recursively
checked), then there is a high degree of confidence the OOT driver will not
only load, but run correctly.

The question is how to provide a similar guarantee if a different way?

We have plenty of customers with 10 year old drivers, where the expertise
has long left the company.  The engineers still around, recompile and make
tweaks to get things working on the latest RHEL.  Verify it passes testing
and release it.  Then they hope to not touch it again for a few years until
the next RHEL comes along.

Scary, huh? :-)

Common examples, filesystems and storage drivers.


There is no way that I see to provide a 100% guarantee, but if we do enough
checks, we should be able to have a high degree of confidence the driver
won't blow up.

On the flip side, easy things in the kernel to do is:

- provide the memory allocation (instead of having the driver staticly
  allocate)
- provide functions to retrieve various internal data (instead of having the
  driver do direct referencing to deep internal elements)
- cut down on some static inlines (and use accessory functions instead),
  etc.

Those types of changes allow the OOT driver to be more ignorant of kernel
changes and struct modifications.


Look to Stanislav's responses for his ideas on new tooling.

Thanks for helping!

Cheers,
Don


> 
> > I am not sure what 'control vermagic' is, but it sounds like a string check,
> > which won't protect against the boatload of backports we do to structs,
> > enums, and functions.
> 
> Basically vermagic is the string all modules and the kernel get, which
> must match in order to load modules. If you have modversions disabled,
> then vermagic includes the kernel version. If modversions is enabled,
> then vermagic does not include the kernel version but the CRCs have to
> also match.
> 
> Controlling it explicitly is just a couple of lines where a distro can
> control it (so they can update their kernel version without breaking).
> It's not meant to solve everything, just the first one.
>  
> > Currently we are exploring various ways to get smarter here.  The genksyms
> > tool has its limitations and handling kabi hacks in RHEL is getting
> > tiresome.
> > 
> > I think GregKH pointed to one such tool, libabigail?  We are working on
> > others too.
> > 
> > 
> > Circling back to enabling MODVERSIONS in Fedora, that was to start the
> > process of syncing Fedora with RHEL stuff in preparation for smarter tools.
> > 
> > 
> > If you take away MODVERSIONS, that would put a damper in our work, but
> > easily carried privately (much like MODSIGNING for 8 years until it went
> > upstream :-) ).
> 
> I don't think that's necessary. A feature requirement for a distro is just
> as valid as any other user of upstream. I don't want to hinder any distro,
> I'm just still not quite seeing the big picture of exactly what functionality
> you need from the kernel.
> 
> Thanks,
> Nick