Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.kernel > #55878 > unrolled thread

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

Back to article view | Back to linux.debian.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Ben Hutchings <ben@decadent.org.uk> - 2016-11-29 02:20 +0100
    Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-11-29 03:40 +0100
      Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-11-29 10:20 +0100
    Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-29 05:10 +0100
      Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Adam Borowski <kilobyte@angband.pl> - 2016-11-29 14:20 +0100
        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Ingo Molnar <mingo@kernel.org> - 2016-11-29 14:40 +0100
          Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Adam Borowski <kilobyte@angband.pl> - 2016-11-29 15:30 +0100
        [PATCH] x86/kbuild: enable modversions for symbols exported from asm Adam Borowski <kilobyte@angband.pl> - 2016-11-29 15:00 +0100
          Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-29 16:50 +0100
            Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-11-29 17:20 +0100
              Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-29 17:40 +0100
                Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Ben Hutchings <ben@decadent.org.uk> - 2016-11-29 21:00 +0100
                  Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-29 22:00 +0100
                    Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-30 20:00 +0100
                      Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Ben Hutchings <ben@decadent.org.uk> - 2016-11-30 22:40 +0100
                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-01 03:20 +0100
                          Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Ben Hutchings <ben@decadent.org.uk> - 2016-12-01 03:40 +0100
                            Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-01 05:00 +0100
                              Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-12-01 17:40 +0100
                                Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-12-02 16:00 +0100
                                Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-09 05:00 +0100
                                  Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Ian Campbell <ijc@hellion.org.uk> - 2016-12-09 16:30 +0100
                                    Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-09 17:40 +0100
                                      Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-12-10 14:00 +0100
                                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-12 05:10 +0100
                                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Ian Campbell <ijc@hellion.org.uk> - 2016-12-12 10:10 +0100
                                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Don Zickus <dzickus@redhat.com> - 2016-12-14 19:20 +0100
                                      Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Stanislav Kozina <skozina@redhat.com> - 2016-12-13 02:30 +0100
                                      Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-12-14 00:10 +0100
                                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Dodji Seketeli <dodji@seketeli.org> - 2016-12-14 10:20 +0100
                                          Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-12-14 10:40 +0100
                                            Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-12-14 11:00 +0100
                                            Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Dodji Seketeli <dodji@seketeli.org> - 2016-12-14 11:00 +0100
                                              Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-12-14 11:10 +0100
                                                Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Dodji Seketeli <dodji@seketeli.org> - 2016-12-14 11:20 +0100
                                                  Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-12-14 11:40 +0100
                                              Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Dodji Seketeli <dodji@seketeli.org> - 2016-12-14 11:20 +0100
                      Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Don Zickus <dzickus@redhat.com> - 2016-12-01 05:50 +0100
                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-01 05:50 +0100
                          Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Don Zickus <dzickus@redhat.com> - 2016-12-01 16:40 +0100
                            Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Don Zickus <dzickus@redhat.com> - 2016-12-01 17:00 +0100
                              Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-12-01 17:40 +0100
                                Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Don Zickus <dzickus@redhat.com> - 2016-12-01 20:10 +0100
                            Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Christoph Hellwig <hch@infradead.org> - 2016-12-01 17:10 +0100
                            Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-09 05:10 +0100
                              Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Stanislav Kozina <skozina@redhat.com> - 2016-12-09 09:20 +0100
                                Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-09 09:40 +0100
                                  Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Stanislav Kozina <skozina@redhat.com> - 2016-12-09 16:00 +0100
                                    Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-09 17:20 +0100
                                      Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-12-09 17:30 +0100
                                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Stanislav Kozina <skozina@redhat.com> - 2016-12-12 11:10 +0100
                                          Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-13 08:50 +0100
                                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Hannes Frederic Sowa <hannes@redhat.com> - 2016-12-14 15:40 +0100
                                          Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-15 03:30 +0100
                                            Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Hannes Frederic Sowa <hannes@redhat.com> - 2016-12-15 12:40 +0100
                                              Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-15 13:30 +0100
                                                Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Hannes Frederic Sowa <hannes@redhat.com> - 2016-12-15 14:40 +0100
                                                  Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-15 15:40 +0100
                                                    Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Hannes Frederic Sowa <hannes@redhat.com> - 2016-12-15 16:40 +0100
                                                Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Stanislav Kozina <skozina@redhat.com> - 2016-12-15 15:00 +0100
                              Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Don Zickus <dzickus@redhat.com> - 2016-12-09 17:40 +0100
                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Stanislav Kozina <skozina@redhat.com> - 2016-12-01 12:20 +0100
                          Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-01 12:30 +0100
                            Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Stanislav Kozina <skozina@redhat.com> - 2016-12-01 13:00 +0100
                              Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-12-01 14:00 +0100
                            Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Dodji Seketeli <dodji@seketeli.org> - 2016-12-01 16:50 +0100
                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-12-01 17:40 +0100
            Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Adam Borowski <kilobyte@angband.pl> - 2016-11-29 18:10 +0100
              Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-29 18:30 +0100
                Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-29 18:40 +0100
                  Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Arnd Bergmann <arnd@arndb.de> - 2016-12-01 15:20 +0100
                    Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-12-01 17:40 +0100
                    Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-01 19:50 +0100
                      Re: [RFC, PATCH, v3.9] default exported asm symbols to zero Geert Uytterhoeven <geert@linux-m68k.org> - 2016-12-02 14:00 +0100
                        Re: [RFC, PATCH, v3.9] default exported asm symbols to zero Arnd Bergmann <arnd@arndb.de> - 2016-12-02 16:10 +0100
                      Re: [RFC, PATCH, v3.9] default exported asm symbols to zero Adam Borowski <kilobyte@angband.pl> - 2016-12-02 16:40 +0100
                      Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-02 18:30 +0100
                      Re: [RFC, PATCH, v3.9] default exported asm symbols to zero Ben Hutchings <ben@decadent.org.uk> - 2016-12-03 05:40 +0100
                        Re: [RFC, PATCH, v3.9] default exported asm symbols to zero Arnd Bergmann <arnd@arndb.de> - 2016-12-03 12:10 +0100
                      Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Alan Modra <amodra@gmail.com> - 2016-12-04 09:20 +0100
                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-04 22:10 +0100
                Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-11-29 22:50 +0100
          Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Ben Hutchings <ben@decadent.org.uk> - 2016-12-02 03:10 +0100
            Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Adam Borowski <kilobyte@angband.pl> - 2016-12-02 12:30 +0100
              Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Ben Hutchings <ben@decadent.org.uk> - 2016-12-02 16:10 +0100

Page 3 of 5 — ← Prev page 1 2 [3] 4 5  Next page →

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#56059

On Thu, 1 Dec 2016 10:20:39 -0500
Don Zickus <dzickus@redhat.com> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#56062

On Fri, 9 Dec 2016 08:55:51 +0100
Stanislav Kozina <skozina@redhat.com> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#56070

On Fri, 9 Dec 2016 15:36:04 +0100
Stanislav Kozina <skozina@redhat.com> 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

[toc] | [prev] | [next] | [standalone]

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

On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:
> On Fri, 9 Dec 2016 15:36:04 +0100
> Stanislav Kozina <skozina@redhat.com> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#56126

On Mon, 12 Dec 2016 10:48:47 +0100
Stanislav Kozina <skozina@redhat.com> 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

[toc] | [prev] | [next] | [standalone]

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

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 <skozina@redhat.com> 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)

[toc] | [prev] | [next] | [standalone]

#56182

On Wed, 14 Dec 2016 15:04:36 +0100
Hannes Frederic Sowa <hannes@redhat.com> 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 <skozina@redhat.com> 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

[toc] | [prev] | [next] | [standalone]

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

On 15.12.2016 03:06, Nicholas Piggin wrote:
> On Wed, 14 Dec 2016 15:04:36 +0100
> Hannes Frederic Sowa <hannes@redhat.com> 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 <skozina@redhat.com> 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 opinion on this is.. As I understood that he
wants to maintain a super-long-term stable kernel with kabi guarantees.

Note, what we want is to weaken the check for kabi, by excluding parts
of the struct from genksyms with libabigail. For Red Hat genksyms is too
strict in the checks.

Bye,
Hannes

[toc] | [prev] | [next] | [standalone]

#56201

On Thu, 15 Dec 2016 12:19:02 +0100
Hannes Frederic Sowa <hannes@redhat.com> wrote:

> On 15.12.2016 03:06, Nicholas Piggin wrote:
> > On Wed, 14 Dec 2016 15:04:36 +0100
> > Hannes Frederic Sowa <hannes@redhat.com> 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 <skozina@redhat.com> 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
> >> 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.

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.

> Also I wonder what Ben's opinion on this is.. As I understood that he
> wants to maintain a super-long-term stable kernel with kabi guarantees.
> 
> Note, what we want is to weaken the check for kabi, by excluding parts
> of the struct from genksyms with libabigail. For Red Hat genksyms is too
> strict in the checks.

Sure, that makes sense.

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.

Thanks,
Nick

[toc] | [prev] | [next] | [standalone]

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

On 15.12.2016 13:03, Nicholas Piggin wrote:
> On Thu, 15 Dec 2016 12:19:02 +0100
> Hannes Frederic Sowa <hannes@redhat.com> wrote:
> 
>> On 15.12.2016 03:06, Nicholas Piggin wrote:
>>> On Wed, 14 Dec 2016 15:04:36 +0100
>>> Hannes Frederic Sowa <hannes@redhat.com> 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 <skozina@redhat.com> 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 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.
> 
> 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.

Yes, I think this needs more experimentation and thought right now
before we can make a decision.

>> Also I wonder what Ben's opinion on this is.. As I understood that he
>> wants to maintain a super-long-term stable kernel with kabi guarantees.
>>
>> Note, what we want is to weaken the check for kabi, by excluding parts
>> of the struct from genksyms with libabigail. For Red Hat genksyms is too
>> strict in the checks.
> 
> Sure, that makes sense.
> 
> 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.

Agreed!

Thanks also,
Hannes

[toc] | [prev] | [next] | [standalone]

#56209

On Thu, 15 Dec 2016 14:15:31 +0100
Hannes Frederic Sowa <hannes@redhat.com> wrote:

> On 15.12.2016 13:03, Nicholas Piggin wrote:
> > On Thu, 15 Dec 2016 12:19:02 +0100
> > Hannes Frederic Sowa <hannes@redhat.com> wrote:
> >   
> >> On 15.12.2016 03:06, Nicholas Piggin wrote:  
> >>> On Wed, 14 Dec 2016 15:04:36 +0100
> >>> Hannes Frederic Sowa <hannes@redhat.com> 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 <skozina@redhat.com> 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 that distros are
happy with of course, but I just don't see why it would be a lot of
overhead. Particularly if you just scripted everything.

How frequently do symbols become incompatible within an ostensibly ABI-
stable release?

> >> 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.  
> > 
> > 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.  
> 
> Yes, I think this needs more experimentation and thought right now
> before we can make a decision.

Sure, I wanted to mention it in case people had a concern about out
of tree tools. It will depend on what distros end up settling with.

> >> Also I wonder what Ben's opinion on this is.. As I understood that he
> >> wants to maintain a super-long-term stable kernel with kabi guarantees.
> >>
> >> Note, what we want is to weaken the check for kabi, by excluding parts
> >> of the struct from genksyms with libabigail. For Red Hat genksyms is too
> >> strict in the checks.  
> > 
> > Sure, that makes sense.
> > 
> > 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.  
> 
> Agreed!

Thanks,
Nick

[toc] | [prev] | [next] | [standalone]

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

On 15.12.2016 15:15, Nicholas Piggin wrote:
> On Thu, 15 Dec 2016 14:15:31 +0100
> Hannes Frederic Sowa <hannes@redhat.com> wrote:
> 
>> On 15.12.2016 13:03, Nicholas Piggin wrote:
>>> On Thu, 15 Dec 2016 12:19:02 +0100
>>> Hannes Frederic Sowa <hannes@redhat.com> wrote:
>>>   
>>>> On 15.12.2016 03:06, Nicholas Piggin wrote:  
>>>>> On Wed, 14 Dec 2016 15:04:36 +0100
>>>>> Hannes Frederic Sowa <hannes@redhat.com> 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 <skozina@redhat.com> 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 that distros are
> happy with of course, but I just don't see why it would be a lot of
> overhead. Particularly if you just scripted everything.

Yes, me neither, of course. I just would favor to get away with the
scripting if possible. ;)

> How frequently do symbols become incompatible within an ostensibly ABI-
> stable release?

Given how genksyms works right now, I would say quite frequently. I
would even go that far to say that every larger backport I had to deal
with in networking had those problems because of some very important top
level structs that make everything reachable (struct net_device or
struct sock), so each change to those structs basically cause a kabi change.

Even if we replace padding in a structure we added before a release,
this is actually considered a kabi breakage, we must annotate this
appropriately because of genksyms.

E.g. I also thought about reordering the structs. As already discussed
in this thread, genksyms takes ordering of its definition vs.
declarations into account, e.g.:

struct foo;
struct bar {
	struct foo *f;
};
<< private >>
struct foo {
};

wouldn't actually include struct foo as part of the symvers, but the
other way around it would.

Instead of burden upstream with this kind of reordering, a suppression
engine would be favored by me which we can drive by review and
discussions. Thus upstream doesn't have any burden on that, but it is
solely the burden of the distributions, where the burden belongs. ;)

>>>> 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.  
>>>
>>> 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.  
>>
>> Yes, I think this needs more experimentation and thought right now
>> before we can make a decision.
> 
> Sure, I wanted to mention it in case people had a concern about out
> of tree tools. It will depend on what distros end up settling with.

The possibilities I see right now:

* we leave genksyms where it is for the time being and add a KCONFIG
knob to be able to call a replacement tool with well formed command line
syntax and stdout semantics.

* we import one of the alternatives instead of genksyms if it fits to
the kernel. I am still a bit concerned about differences in dwarf vs.
source code parsing. In the past I had twice bugs in dwarf generation,
but this probably can be dealt with, but just needs a bit more
experience in my opinion.

* we remove genksyms and just use one versioning vector for upstream but
still allow some kind of modversions per symbol being loaded from a
third party program like kabi-dw or libabigail.

Personally, because I see genksyms working quite well right now, I would
stick to it until we (the distributions) came to a conclusion.

Bye,
Hannes

[toc] | [prev] | [next] | [standalone]

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

> 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

[toc] | [prev] | [next] | [standalone]

Page 3 of 5 — ← Prev page 1 2 [3] 4 5  Next page →

Back to top | Article view | linux.debian.kernel

csiph-web