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 2 of 5 — ← Prev page 1 [2] 3 4 5  Next page →

#56058

On Thu, 1 Dec 2016 17:12:41 +0100
Michal Marek <mmarek@suse.com> wrote:

> On 2016-12-01 04:39, Nicholas Piggin wrote:
> > On Thu, 01 Dec 2016 02:35:54 +0000
> > Ben Hutchings <ben@decadent.org.uk> 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
00000000a0cf13a0 A __crc_foo

Now change to

struct blah {
  int y;
  int x;
};

$ nm vmlinux | grep __crc_foo
00000000a0cf13a0 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

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

#56069

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
> 00000000a0cf13a0 A __crc_foo
> 
> Now change to
> 
> struct blah {
>   int y;
>   int x;
> };
> 
> $ nm vmlinux | grep __crc_foo
> 00000000a0cf13a0 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 * ) 
    000000000cd0312e 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 * ) 
    00000000eda220c6 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 * ) 
    00000000a0cf13a0 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.

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

#56074

On Fri, 09 Dec 2016 15:21:33 +0000
Ian Campbell <ijc@hellion.org.uk> 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
> > 00000000a0cf13a0 A __crc_foo
> > 
> > Now change to
> > 
> > struct blah {
> >   int y;
> >   int x;
> > };
> > 
> > $ nm vmlinux | grep __crc_foo
> > 00000000a0cf13a0 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 * ) 
>     000000000cd0312e 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 * ) 
>     00000000eda220c6 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 * ) 
>     00000000a0cf13a0 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

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

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

On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote:
> Hello,
> 
> Nicholas Piggin <npiggin@gmail.com> 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

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

#56105

On Sat, 10 Dec 2016 13:41:03 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote:
> > Hello,
> > 
> > Nicholas Piggin <npiggin@gmail.com> 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 :)

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

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

#56108

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.

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

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

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 <npiggin@gmail.com> 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.)

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

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

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

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

real    0m29.057s
user    0m13.929s
sys    0m14.862s
$ time ~/Code/kabi-dw/kabi-dw generate -o abi2 vmlinux.abi2
Generating symbol defs from vmlinux.abi2...

real    0m29.134s
user    0m13.961s
sys    0m14.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;


real    0m0.176s
user    0m0.135s
sys    0m0.040s

The size of the generated text files with all the relevant type 
information is as follows:
$ du -hs abi1
16M    abi1
$ find abi1 -type f | wc -l
3162

Warm regards,
-Stanislav

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

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

Dne 9.12.2016 v 23:46 Dodji Seketeli napsal(a):
> Hello,
> 
> Nicholas Piggin <npiggin@gmail.com> 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

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

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

Michal Marek <mmarek@suse.com> 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

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

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

On 2016-12-14 09:58, Dodji Seketeli wrote:
> Michal Marek <mmarek@suse.com> 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

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

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

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 "<built-in>"
# 1 "<command-line>"
# 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 "<built-in>"
# 1 "<command-line>"
# 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 <t1.i 
Export foo == <void foo ( struct s3 { struct s1 { UNKNOWN } * ptr1 ; struct s2 { int i ; } * ptr2 ; } * ) >
__crc_foo = 0xf731cef8 ;

$ ./scripts/genksyms/genksyms -D <t2.i 
Export foo == <void foo ( struct s3 { struct s1 { int j ; } * ptr1 ; struct s2 { UNKNOWN } * ptr2 ; } * ) >
__crc_foo = 0xc925dae5 ;

Michal

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

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

Michal Marek <mmarek@suse.com> 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

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

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

On 2016-12-14 10:36, Dodji Seketeli wrote:
> Michal Marek <mmarek@suse.com> 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

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

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

Michal Marek <mmarek@suse.com> 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

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

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

On 2016-12-14 11:02, Dodji Seketeli wrote:
> Michal Marek <mmarek@suse.com> 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

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

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

Dodji Seketeli <dodji@seketeli.org> 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

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

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

On Wed, Nov 30, 2016 at 10:40:02AM -0800, Linus Torvalds wrote:
> On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > Here's an initial rough hack at removing modversions. It gives an idea
> > of the complexity we're carrying for this feature (keeping in mind most
> > of the lines removed are generated parser).
> 
> You definitely don't have to try to convince me. We've had many issues
> with modversions over the years. This was just the "last drop" as far
> as I'm concerned, we've had random odd crc generation failures due to
> some build races too.
> 
> > In its place I just added a simple config option to override vermagic
> > so distros can manage it entirely themselves.
> 
> So at least Fedora doesn't even enable CONFIG_MODVERSIONS as-is. I'm
> _hoping_ it's just Debian that wants this, and we'd need to get some
> input from the Debian people whether that "control vermagic" is
> sufficient? I suspect it isn't, but I can't come up with any simple
> alternate model either..

Oddly, I just posted a patch to enable this for Fedora and then someone
pointed me at this thread. :-/

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.

They requested this to save time and money on rebuilding and retesting.  It
also helps deal with situations where RHEL puts out a security fix or new
minor release and the provider of OOT driver has not released the
appropriate update.  Customers like the ability to roll their special
drivers forward quickly to their schedule.

Now we don't protect every symbol, just a select few that our meets our
customers needs (and developers willing to support it).

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.

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.

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


We would prefer to work with various folks to figure out a better solution
to solve our/others needs.  Anyone interested in working with Red Hat should
contact Stanislav Kozina (skozina@redhat.com) (cc'd above) and cc myself.

Cheers,
Don



> 
> I'm also somewhat surprised that it's Debian that has this problem,
> considering how Debian is usually the distro that is _least_ receptive
> to various non-free binaries.
> 
>             Linus

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

#55908

On Wed, 30 Nov 2016 23:13:25 -0500
Don Zickus <dzickus@redhat.com> wrote:

> On Wed, Nov 30, 2016 at 10:40:02AM -0800, Linus Torvalds wrote:
> > On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <npiggin@gmail.com> wrote:  
> > >
> > > Here's an initial rough hack at removing modversions. It gives an idea
> > > of the complexity we're carrying for this feature (keeping in mind most
> > > of the lines removed are generated parser).  
> > 
> > You definitely don't have to try to convince me. We've had many issues
> > with modversions over the years. This was just the "last drop" as far
> > as I'm concerned, we've had random odd crc generation failures due to
> > some build races too.
> >   
> > > In its place I just added a simple config option to override vermagic
> > > so distros can manage it entirely themselves.  
> > 
> > So at least Fedora doesn't even enable CONFIG_MODVERSIONS as-is. I'm
> > _hoping_ it's just Debian that wants this, and we'd need to get some
> > input from the Debian people whether that "control vermagic" is
> > sufficient? I suspect it isn't, but I can't come up with any simple
> > alternate model either..  
> 
> Oddly, I just posted a patch to enable this for Fedora and then someone
> pointed me at this thread. :-/
> 
> 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.
> 
> They requested this to save time and money on rebuilding and retesting.  It
> also helps deal with situations where RHEL puts out a security fix or new
> minor release and the provider of OOT driver has not released the
> appropriate update.  Customers like the ability to roll their special
> drivers forward quickly to their schedule.
> 
> Now we don't protect every symbol, just a select few that our meets our
> customers needs (and developers willing to support it).
> 
> 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.

> 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

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

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

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

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

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

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

csiph-web