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

[Nicholas Piggin]

On Tue, 29 Nov 2016 01:15:48 +
Ben Hutchings <b...@decadent.org.uk> wrote:

> [I've had to guess at the cc list for this, because we no longer have
> mail archives that preserve them.]

You got it about right.

> On Fri, 2016-11-25 at 10:01 -0800, Linus Torvalds wrote:
> > On Thu, Nov 24, 2016 at 4:40 PM, Nicholas Piggin <npig...@gmail.com> wrote: 
> >  
> > > > 
> > > > Yes, manual "marking" is never going to be a viable solution.  
> > > 
> > > I guess it really depends on how exactly you want to use it. For distros
> > > that do stable ABI but rarely may have to break something for security
> > > reasons, it should work and give exact control.  
> 
> This is roughly how Debian handles the kernel module ABI during a
> stable release.
> 
> > No. Because nobody else will care, so unless it's like a single symbol
> > or something, it will just be a maintenance nightmare.  
> 
> I agree with this.  We can explicitly "version" individual symbols
> anyway by doing something like:
> 
> -int foo(void);
> +#define foo foo_2
> +int foo_2(int);

Yeah... Benefit being it's very simple and everybody can see exactly
what it does and knows how it will work.

> 
> > > What else do people *actually* use it for? Preventing mismatched modules
> > > when .git version is not attached and release version of the kernel has
> > > not been bumped. Is that it?  
> > 
> > It used to be very useful for avoiding loading stale modules and then
> > wasting days on debugging something that wasn't the case when you had
> > forgotten to do "make modules_install". Change some subtle internal
> > ABI issue (add/remove a parameter, whatever) and it would really help.
> > 
> > These days, for me, LOCALVERSION_AUTO and module signing are what I
> > personally tend to use.
> >
> > The modversions stuff may just be too painful to bother with. Very few
> > people probably use it, and the ones that do likely don't have any
> > overriding reason why.  
> [...]
> 
> Debian has some strong reasons:
> 
> 1. Changing the release string requires any out-of-tree modules to be
> upgraded (at least rebuilt) on end-user systems.  So we try to avoid
> doing that during the lifetime of a stable release, i.e. we don't let
> the release string change.  Also, the release string is reflected in
> package names (e.g. linux-image-4.8.0-1-amd64), and introducing new
> package names requires manual approval by the Debian archive team.

This is something I've noticed. Would it be better if the module loader
ignores the kernel version and instead used some internal ABI version
string to check against? Otherwise (AFAICT) you always have 4.8.0 versions
despite being 4.8.7 kernel, and you can't upgrade a point release without
overwriting your old kernel and modules.

That is something we could potentially replace modversions with. It would
be a far more reasonable complexity to carry for downstream distros than
modversions. Though not something we can add for 4.9.

> 2. We want to allow ABI breaks for "internal" symbols used only by in-
> tree modules, as those breaks will be resolved by rebooting to complete
> the upgrade.  But we need a run-time check to prevent loading an
> incompatible module before the reboot.
> 
> 3. So far as I can see, module signing doesn't work for a distribution
> kernel with out-of-tree modules as there has to be a trust path from a
> built-in certificate to the module signing certificate.  So signature
> enforcement will have to be disabled on systems that use out-of-tree
> modules, thus it's not a substitute for modversions.
> 
> We expect Linux 4.9 to be the basis for a longterm stable branch and on
> that basis intend to include it in the next Debian stable release. 
> Even if the decision is to get rid of modversions, it would be very
> helpful if they could be revived for 4.9 so that we have some time to
> adapt our packaging practices to work without them in future.

It would be nice to get upstream to the point where 4.9 modversions
works if you just patch out depends BROKEN. That would require reverting
a few more of Al's arch patches.

Then in 4.10 we can re-add all those arch patches (which are less
controversial without the asm-prototypes.h workaround), and implement a
simple stable ABI version string check, and then in 4.11 we can remove
modversions.

Thanks,
Nick

Bug#844530: [1/2] kbuild: provide include/asm/asm-prototypes.h for ARM

[Nicholas Piggin]

On Mon, 21 Nov 2016 19:13:55 +
Russell King - ARM Linux  wrote:

> On Mon, Nov 21, 2016 at 07:46:44PM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Mon, Oct 24, 2016 at 05:05:26PM +0200, Arnd Bergmann wrote:  
> > > This adds an asm/asm-prototypes.h header for ARM to fix the
> > > broken symbol versioning for symbols exported from assembler
> > > files.
> > > 
> > > In addition to the header, we have to do these other small
> > > changes:
> > > 
> > > - move the exports from bitops.h to {change,clear,set,...}bit.S
> > > - move the exports from csumpartialgeneric.S into the files
> > >   including it
> > > 
> > > I couldn't find the correct prototypes for the compiler builtins,
> > > so I went with the fake 'void f(void)' prototypes that we had
> > > before.
> > > 
> > > This leaves the mmioset/mmiocpy function for now, as it's not
> > > obvious how to best handle them.
> > > 
> > > Signed-off-by: Arnd Bergmann   
> > 
> > In my test builds of 4.9-rc5 plus
> > 
> > 4efca4ed05cb ("kbuild: modversions for EXPORT_SYMBOL() for asm")
> > cc6acc11cad1 ("kbuild: be more careful about matching preprocessed asm 
> > ___EXPORT_SYMBOL")
> > 
> > (which are in -rc6) I got many warnings à la:
> > 
> > WARNING: "memset" [drivers/media/usb/airspy/airspy.ko] has no CRC!
> > 
> > and booting the resulting kernel failed with messages of the type:
> > 
> > [3.024126] usbcore: no symbol version for __memzero
> > [3.029107] usbcore: Unknown symbol __memzero (err -22)
> > 
> > so hardly any module could be loaded. modprobe -f works however, but
> > that's not what my initramfs does.
> > 
> > With this patch and https://patchwork.kernel.org/patch/9392291/ ("ARM:
> > move mmiocpy/mmioset exports to io.c") I could compile a kernel without
> > CRC warnings and it boots fine. So it would be great to get these two
> > patches into 4.9.  
> 
> Yea, many things would be nice, but I've been unable to track the
> issues here - it really didn't help _not_ being copied on the
> original set of patches which introduced this mess.

Quick overview:

- asm exports changes allow EXPORT_SYMBOL to be moved into .S files,
  but they would not get modversion CRCs generated.

- The core kbuild patches to add modversions support for asm exports
  is now merged in Linus's tree from the recent kbuild tree pull.
  asm/asm-prototypes.h must contain C style declarations of the symbol
  for this to work.

- Architectures can now add their asm/asm-prototypes.h and things
  *should* start working.

- Dependency is not a hard one. If you add asm-prototypes.h before
  merging the core patches then it should not introduce any problems.


> I've merged Nicolas' patch, so now we need to work out what to do
> with the remaining bits - which I guess are the asm-prototypes.h
> and the mmio* bits.  I'm not aware of what's happening with the
> patches that they depend on (which is why I recently asked the
> question - again, I seem to be completely out of the loop due to
> lack of Cc's).
> 
> So I'm just throwing my hands up and saying "I don't know what to
> do" at this stage.
> 

I don't think you have missed much since last it came up, it's just
taken a bit of time to get the details right and get it merged.

Not sure what your tree looks like, but if you merge this patch 1/2
plus 2a/2 or 2b/2 into upstream, then ARM should be working.

Thanks,
Nick

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

[Nicholas Piggin]

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

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

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

Thanks,
Nick

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

[Nicholas Piggin]

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

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

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

Thanks,
Nick

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

[Nicholas Piggin]

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

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

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

Thanks,
Nick

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

[Nicholas Piggin]

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

> On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote:
> > Hello,
> > 
> > Nicholas Piggin <npig...@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)
> > 
> > 
> > 
> > real0m2.595s
> > user0m2.489s
> > sys 0m0.108s
> > $ 
> > 
> > I kept the timing information to give you an idea of the time it takes
> > on a non-optimized build of abidiff.
> > 
> > One could for instance want that types that are not defined in header
> > files be kept out of the change report.  In that case it's possible to
> > write a little suppression specification file like this one:
> > 
> > $ cat vmlinux.abignore 
> > [suppress_type]
> >   source_location_not_regexp = .*\\.h
> > $
> > 
> > You can then pass that suppression file to the tool:
> > 
> > $ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr 
> > vmlinux.abignore vmlinux.abi1.abi vmlinux.abi2.abi
> > Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 
> > Added function
> > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> > 
> > 
> > real0m2.574s
> > user0m2.473s
> > sys 0m0.102s
> > $
> > 
> > So this is the kind of interface change analysis tool we are working on
> > at the moment.
> > 
> > One could also imagine a tool that would compute a CRC that takes the
> > very same suppression specification files into account, letting people
> > to decide that some interface changes are OK.  That CRC would thus be
> > added to the special ELF sections we already have today.  We could keep
> > the modversion machinery, but with a greater dose of flexibility.
> > Whenever modversion detects a change, abidiff would tell people what the
> > change is exactly.
> > 
> > What do you guys think?  
> 
> YES YES YES!!!
> 
> Now I don't work on a distro anymore, but I would think that something
> like this would be really useful, pointing out exactly what changed is
> very important for distro maintainers to determine what they want to do
> (either fix up the abi change with strange hacks, or ignore it due to
> the change being in an area they don't care at all about, i.e. a random
> driver subsystem.)
> 
> So yes, I think this is really good stuff.  But if the distro
> maintainers correct me and think it's useless, then I need to revisit my
> view of exactly what they do for their customers :)

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

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

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

Thanks,
Nick

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

[Nicholas Piggin]

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

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

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

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

Thanks,
Nick

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

[Nicholas Piggin]

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

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

[Nicholas Piggin]

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

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

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

[Nicholas Piggin]

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

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

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

[Nicholas Piggin]

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

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

Now change to

struct blah {
  int y;
  int x;
};

$ nm vmlinux | grep __crc_foo
a0cf13a0 A __crc_foo

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

Thanks,
Nick

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

[Nicholas Piggin]

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

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

[Nicholas Piggin]

On Tue, 29 Nov 2016 12:35:57 -0800
Linus Torvalds  wrote:

> On Tue, Nov 29, 2016 at 11:57 AM, Ben Hutchings  wrote:
> >
> > If the modversion is missing then the fallback should be to a full
> > vermagic match, i.e. including the release string.  Something like
> > this (untested):  
> 
> This really seems way too complicated for this situation.
> 
> And it's wrong too. The whole point of modversions was that you didn't
> want to do the full version check.
> 
> We already know there were *some* crc's (we checked that at the top of
> check_version(), but we've also checked it in "same_magic()" - it's
> what makes us ignore the exact version number), but this particular
> symbol doesn't have a crc. Just let it through, because we have bugs
> in binutils.
> 
> So your extra complexity logic seems actively wrong. It makes
> MODVERSIONS not work at all, rather than limp along. You're better off
> just not having MODVERSIONS.

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

Note I removed the `rm -r scripts/genksyms` hunks to reduce mail size.

In its place I just added a simple config option to override vermagic
so distros can manage it entirely themselves.

Thanks,
Nick

---
 .gitignore   |1 -
 Documentation/dontdiff   |2 -
 Documentation/kbuild/modules.txt |4 -
 Makefile |7 -
 arch/arm64/include/asm/module.h  |4 -
 arch/avr32/boot/images/Makefile  |2 -
 arch/powerpc/include/asm/module.h|4 -
 arch/powerpc/kernel/module_64.c  |   24 +-
 arch/x86/tools/relocs.c  |1 -
 drivers/char/ipmi/ipmi_ssif.c|5 -
 drivers/firmware/efi/libstub/Makefile|4 +-
 drivers/message/fusion/mptlan.h  |3 -
 include/asm-generic/export.h |8 -
 include/asm-generic/vmlinux.lds.h|   35 -
 include/linux/export.h   |   17 +-
 include/linux/module.h   |   12 -
 include/linux/vermagic.h |   12 +-
 init/Kconfig |   28 +-
 kernel/module.c  |  213 +--
 scripts/Makefile |1 -
 scripts/Makefile.build   |  113 --
 scripts/Makefile.lib |3 +-
 scripts/Makefile.modpost |2 +-
 scripts/adjust_autoksyms.sh  |5 -
 scripts/export_report.pl |   30 +-
 scripts/genksyms/.gitignore  |5 -
 scripts/genksyms/Makefile|   14 -
 scripts/genksyms/genksyms.c  |  880 ---
 scripts/genksyms/genksyms.h  |   94 --
 scripts/genksyms/keywords.gperf  |   60 -
 scripts/genksyms/keywords.hash.c_shipped |  229 ---
 scripts/genksyms/lex.l   |  481 --
 scripts/genksyms/lex.lex.c_shipped   | 2291 
 scripts/genksyms/parse.tab.c_shipped | 2396 --
 scripts/genksyms/parse.tab.h_shipped |  118 --
 scripts/genksyms/parse.y |  513 ---
 scripts/mksysmap |6 +-
 scripts/mod/modpost.c|  122 +-
 scripts/module-common.lds|5 -
 scripts/namespace.pl |2 -
 40 files changed, 60 insertions(+), 7696 deletions(-)
 delete mode 100644 scripts/genksyms/.gitignore
 delete mode 100644 scripts/genksyms/Makefile
 delete mode 100644 scripts/genksyms/genksyms.c
 delete mode 100644 scripts/genksyms/genksyms.h
 delete mode 100644 scripts/genksyms/keywords.gperf
 delete mode 100644 scripts/genksyms/keywords.hash.c_shipped
 delete mode 100644 scripts/genksyms/lex.l
 delete mode 100644 scripts/genksyms/lex.lex.c_shipped
 delete mode 100644 scripts/genksyms/parse.tab.c_shipped
 delete mode 100644 scripts/genksyms/parse.tab.h_shipped
 delete mode 100644 scripts/genksyms/parse.y

diff --git a/.gitignore b/.gitignore
index c2ed4ec..cde8773 100644
--- a/.gitignore
+++ b/.gitignore
@@ -20,7 +20,6 @@
 *.mod.c
 *.i
 *.lst
-*.symtypes
 *.order
 *.elf
 *.bin
diff --git a/Documentation/dontdiff b/Documentation/dontdiff
index 5385cba..40eb3e0 100644
--- a/Documentation/dontdiff
+++ b/Documentation/dontdiff
@@ -47,7 +47,6 @@
 *.sgml
 *.so
 *.so.dbg
-*.symtypes
 *.tab.c
 *.tab.h
 *.tex
@@ -180,7 +179,6 @@ mktree
 modpost
 modules.builtin
 modules.order
-modversions.h*
 nconf
 ncscope.*
 offset.h
diff --git a/Documentation/kbuild/modules.txt b/Documentation/kbuild/modules.txt
index 3fb39e0..1568b8a 100644
--- a/Documentation/kbuild/modules.txt
+++ b/Documentation/kbuild/modules.txt
@@ -61,10 +61,6 @@ make sure the kernel contains the information required. The 
target
 exists solely as a simple way to prepare a kernel source tree for
 

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

[Nicholas Piggin]

On Wed, 30 Nov 2016 23:13:25 -0500
Don Zickus <dzic...@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 <npig...@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

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

[Nicholas Piggin]

On Thu, 01 Dec 2016 02:35:54 +
Ben Hutchings <b...@decadent.org.uk> wrote:

> On Thu, 2016-12-01 at 12:55 +1100, Nicholas Piggin wrote:
> > On Wed, 30 Nov 2016 21:33:01 +  
> > > Ben Hutchings <b...@decadent.org.uk> wrote:  
> >   
> > > On Wed, 2016-11-30 at 10:40 -0800, Linus Torvalds wrote:  
> > > > > On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <npig...@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,    
> > > 
> > > The last time I looked, RHEL and SLE did.  They change the release
> > > string for each new kernel version, but they will copy/link old out-of-
> > > tree modules into the new version's "weak-updates" module subdirectory
> > > if the symbol versions still match.
> > >   
> > > > 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..    
> > > 
> > > Allowing the vermagic to be changed separately doesn't help us, as we
> > > already control the release string.  If we were to change some of the
> > > module tools to consider vermagic then it would allow us to report the
> > > full version in the release string while not forcing rebuilds on every
> > > kernel upgrade - but that's not a pressing problem.  
> > 
> > Okay, but existing modversions AFAIKS does not solve your problems described
> > in yor your earlier mail either. Modversions hardly catches ABI breakage at
> > all, you can't rely on it that way. It's far more likely that some structure
> > size changes deep in the kernel than an exported function type signature
> > changes.  
> 
> 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
- changes to semantics of functions
- data structures derived in ways other than exported symbols, e.g.,
  fixed register for `current` on some archs

This is actually a big problem with it, that it provides a false sense
of security. It simply can't be used to verify your ABI stability.

 [Aside: something like the tool Greg linked earlier,

 
https://kernel-recipes.org/en/2016/talks/would-an-abi-changes-visualization-tool-be-useful-to-linux-kernel-maintenance/

 Would be great if that worked with the kernel. Not necessarily as part
 of the build system, but at least a tool that distros could use to
 analyze ABI changes. It wouldn't catch everything, but it would be far
 better than modversions.]

> > I'm not sure how you know which exports are used only by in-tree modules
> > and which are used out of tree, but if you know that then you can version
> > them manually as we said by adding _v2 in the rare case you need to change
> > a behaviour.  
> 
> That's fine for individual functions.
> 
> > So I'm still having trouble understanding what modversions is giving you.  
> 
> Where there is a family of driver modules (e.g. foo-core, foo-pci, foo-
> usb), 

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

[Nicholas Piggin]

On Wed, 30 Nov 2016 21:33:01 +
Ben Hutchings <b...@decadent.org.uk> wrote:

> On Wed, 2016-11-30 at 10:40 -0800, Linus Torvalds wrote:
> > > On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <npig...@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,  
> 
> The last time I looked, RHEL and SLE did.  They change the release
> string for each new kernel version, but they will copy/link old out-of-
> tree modules into the new version's "weak-updates" module subdirectory
> if the symbol versions still match.
> 
> > 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..  
> 
> Allowing the vermagic to be changed separately doesn't help us, as we
> already control the release string.  If we were to change some of the
> module tools to consider vermagic then it would allow us to report the
> full version in the release string while not forcing rebuilds on every
> kernel upgrade - but that's not a pressing problem.

Okay, but existing modversions AFAIKS does not solve your problems described
in yor your earlier mail either. Modversions hardly catches ABI breakage at
all, you can't rely on it that way. It's far more likely that some structure
size changes deep in the kernel than an exported function type signature
changes.

I'm not sure how you know which exports are used only by in-tree modules
and which are used out of tree, but if you know that then you can version
them manually as we said by adding _v2 in the rare case you need to change
a behaviour.

So I'm still having trouble understanding what modversions is giving you.

> One thing that could work for us would be:
> 
> - Stricter version matching for in-tree modules (maybe some extra
>   part in vermagic that is skipped for out-of-tree modules)
> - Ability to blacklist use of a symbol, or all symbols in a module,
>   by out-of-tree modules
> 
> where the blacklist would be a matter of distribution policy.  But this
> would still require a fair amount of work by someone, and I doubt you'd
> want this upstream.

I don't think people are adverse to carrying some upstream complexity for
ditsros. Although for this fancy blacklist case, can it just be done in
userspace?

Thanks,
Nick

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

[Nicholas Piggin]

On Thu, 1 Dec 2016 11:48:09 +0100
Stanislav Kozina  wrote:

> On 12/01/2016 05:13 AM, Don Zickus wrote:
> 
> ...
> 
> > I think GregKH pointed to one such tool, libabigail?  We are working on
> > others too.  
> 
> I should mention one of the others here:
> https://github.com/skozina/kabi-dw
> 
> It's quite comparable to libabigail in the way it works, the main 
> differences are:
>   - written in pure C
>   - depends only on elf-utils and flex/yacc
>   - it's much simpler (4k LOC)
>   - stores the type information in the text files and compares those 
> instead of directly comparing two sets of DWARF data

Now this seems much better for distro ABI checking.

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

Thanks,
Nick

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

[Nicholas Piggin]

On Thu, 1 Dec 2016 12:33:02 +0100
Stanislav Kozina <skoz...@redhat.com> wrote:

> On 12/01/2016 12:09 PM, Nicholas Piggin wrote:
> > On Thu, 1 Dec 2016 11:48:09 +0100
> > Stanislav Kozina <skoz...@redhat.com> wrote:
> >  
> >> On 12/01/2016 05:13 AM, Don Zickus wrote:
> >>
> >> ...
> >>  
> >>> I think GregKH pointed to one such tool, libabigail?  We are working on
> >>> others too.  
> >> I should mention one of the others here:
> >> https://github.com/skozina/kabi-dw
> >>
> >> It's quite comparable to libabigail in the way it works, the main
> >> differences are:
> >>- written in pure C
> >>- depends only on elf-utils and flex/yacc
> >>- it's much simpler (4k LOC)
> >>- stores the type information in the text files and compares those
> >> instead of directly comparing two sets of DWARF data  
> > Now this seems much better for distro ABI checking.
> >
> > The next question is, do they need any kernel support for rare cases
> > where they do have to break the ABI of an export? Simple rename of the
> > function with a _v2 postfix might be enough. We could retain some per
> > symbol versioning in the kernel if needed, but how much would it
> > actually help?  
> 
> The biggest pain point AFAICT is to identify what types (functions, 
> structs, enums, ...) should be considered a part of the stable ABI.

Sure. This is something an automated checker can't solve completely.
Any changes would have to be considered in terms of their impact to
the ABI. It's not just data but also instruction changes involved.

This is policy that should not be mandated by the kernel. Which is
why I'm in favor of using tools like this and just providing mechanism
so distros can implement their own polices.

> And 
> the problem with modversions is that it pulls in just everything which 
> gets (accidentally?) #included in the source file.

I think that's SRCVERSION which is something else. But modversions
has problems too.

> The actual ABI maintenance is a different problem, but there are many 
> possible approaches, the _v2 suffix being one of them.

Would be good to get a consensus on that too.

Thanks,
Nick

Re: a bug in genksysms/CONFIG_MODVERSIONS w/ __attribute__((foo))?

[Nicholas Piggin]

Ben Hutchings's on August 28, 2019 1:34 am:
> On Tue, 2019-08-27 at 22:42 +1000, Nicholas Piggin wrote:
>> Masahiro Yamada's on August 27, 2019 8:49 pm:
>> > Hi.
>> > 
>> > On Tue, Aug 27, 2019 at 6:59 PM Nicholas Piggin  wrote:
>> > > Nick Desaulniers's on August 27, 2019 8:57 am:
>> > > > On Mon, Aug 26, 2019 at 2:22 PM Nick Desaulniers
>> > > >  wrote:
>> > > > > I'm looking into a linkage failure for one of our device kernels, and
>> > > > > it seems that genksyms isn't producing a hash value correctly for
>> > > > > aggregate definitions that contain __attribute__s like
>> > > > > __attribute__((packed)).
>> > > > > 
>> > > > > Example:
>> > > > > $ echo 'struct foo { int bar; };' | ./scripts/genksyms/genksyms -d
>> > > > > Defn for struct foo == 
>> > > > > Hash table occupancy 1/4096 = 0.000244141
>> > > > > $ echo 'struct __attribute__((packed)) foo { int bar; };' |
>> > > > > ./scripts/genksyms/genksyms -d
>> > > > > Hash table occupancy 0/4096 = 0
>> > > > > 
>> > > > > I assume the __attribute__ part isn't being parsed correctly (looks
>> > > > > like genksyms is a lex/yacc based C parser).
>> > > > > 
>> > > > > The issue we have in our out of tree driver (*sadface*) is basically 
>> > > > > a
>> > > > > EXPORT_SYMBOL'd function whose signature contains a packed struct.
>> > > > > 
>> > > > > Theoretically, there should be nothing wrong with exporting a 
>> > > > > function
>> > > > > that requires packed structs, and this is just a bug in the lex/yacc
>> > > > > based parser, right?  I assume that not having CONFIG_MODVERSIONS
>> > > > > coverage of packed structs in particular could lead to potentially
>> > > > > not-fun bugs?  Or is using packed structs in exported function 
>> > > > > symbols
>> > > > > with CONFIG_MODVERSIONS forbidden in some documentation somewhere I
>> > > > > missed?
>> > > > 
>> > > > Ah, looks like I'm late to the party:
>> > > > https://lwn.net/Articles/707520/
>> > > 
>> > > Yeah, would be nice to do something about this.
>> > 
>> > modversions is ugly, so it would be great if we could dump it.
>> > 
>> > > IIRC (without re-reading it all), in theory distros would be okay
>> > > without modversions if they could just provide their own explicit
>> > > versioning. They take care about ABIs, so they can version things
>> > > carefully if they had to change.
> 
> Debian doesn't currently have any other way of detecting ABI changes
> (other than eyeballing diffs).
> 
> I know there have been proposals of using libabigail for this instead,
> but I'm not sure how far those progressed.
> 
>> > We have not provided any alternative solution for this, haven't we?
>> > 
>> > In your patch (https://lwn.net/Articles/707729/),
>> > you proposed CONFIG_MODULE_ABI_EXPLICIT.
>> 
>> Right, that was just my first proposal, but I am not confident that I
>> understood everybody's requirements. I don't think the distro people
>> had much time to to test things out.
>> 
>> One possible shortcoming with that patch is no per-symbol version. The 
>> distro may break an ABI for a security fix, but they don't want to break
>> all out of tree modules if it's an obscure ABI.
> 
> Right, for example the KVM kABI is only meant for in-tree modules (like
> kvm_intel) and in Debian we do not change the "ABI version" and require
> rebuilding out-of-tree modules just because that ABI changes. 
> Currently we maintain explicit lists of exported symbols and exporting
> modules for which we ignore ABI changes at build time.
> 
>> The counter argument to 
>> that is they should just rename the symbol in their kernel for such 
>> cases, so I didn't implement it without somebody describing a good
>> requirement.
> [...]
> 
> Sometimes it is just a single function that changes, but often a
> structure change can affect large numbers of functions.  For example,
> if KVM adds a member to an operations struct that can indirectly change
> the ABI for most of its exported functions.  We wouldn't want to change
> the ABI version but would still want to prevent loading mismatched kvm
> and kvm_intel v

Re: a bug in genksysms/CONFIG_MODVERSIONS w/ __attribute__((foo))?

[Nicholas Piggin]

Masahiro Yamada's on August 27, 2019 8:49 pm:
> Hi.
> 
> On Tue, Aug 27, 2019 at 6:59 PM Nicholas Piggin  wrote:
>>
>> Nick Desaulniers's on August 27, 2019 8:57 am:
>> > On Mon, Aug 26, 2019 at 2:22 PM Nick Desaulniers
>> >  wrote:
>> >>
>> >> I'm looking into a linkage failure for one of our device kernels, and
>> >> it seems that genksyms isn't producing a hash value correctly for
>> >> aggregate definitions that contain __attribute__s like
>> >> __attribute__((packed)).
>> >>
>> >> Example:
>> >> $ echo 'struct foo { int bar; };' | ./scripts/genksyms/genksyms -d
>> >> Defn for struct foo == 
>> >> Hash table occupancy 1/4096 = 0.000244141
>> >> $ echo 'struct __attribute__((packed)) foo { int bar; };' |
>> >> ./scripts/genksyms/genksyms -d
>> >> Hash table occupancy 0/4096 = 0
>> >>
>> >> I assume the __attribute__ part isn't being parsed correctly (looks
>> >> like genksyms is a lex/yacc based C parser).
>> >>
>> >> The issue we have in our out of tree driver (*sadface*) is basically a
>> >> EXPORT_SYMBOL'd function whose signature contains a packed struct.
>> >>
>> >> Theoretically, there should be nothing wrong with exporting a function
>> >> that requires packed structs, and this is just a bug in the lex/yacc
>> >> based parser, right?  I assume that not having CONFIG_MODVERSIONS
>> >> coverage of packed structs in particular could lead to potentially
>> >> not-fun bugs?  Or is using packed structs in exported function symbols
>> >> with CONFIG_MODVERSIONS forbidden in some documentation somewhere I
>> >> missed?
>> >
>> > Ah, looks like I'm late to the party:
>> > https://lwn.net/Articles/707520/
>>
>> Yeah, would be nice to do something about this.
> 
> modversions is ugly, so it would be great if we could dump it.
> 
>> IIRC (without re-reading it all), in theory distros would be okay
>> without modversions if they could just provide their own explicit
>> versioning. They take care about ABIs, so they can version things
>> carefully if they had to change.
> 
> We have not provided any alternative solution for this, haven't we?
> 
> In your patch (https://lwn.net/Articles/707729/),
> you proposed CONFIG_MODULE_ABI_EXPLICIT.

Right, that was just my first proposal, but I am not confident that I
understood everybody's requirements. I don't think the distro people
had much time to to test things out.

One possible shortcoming with that patch is no per-symbol version. The 
distro may break an ABI for a security fix, but they don't want to break
all out of tree modules if it's an obscure ABI. The counter argument to 
that is they should just rename the symbol in their kernel for such 
cases, so I didn't implement it without somebody describing a good
requirement.

> If it is good enough for distros, we merge it first,
> give them time to migrate over to it, then finally remove modversions??

I guess. Do we really need to merge and wait? If they _really_ want it,
and won't put in effort to convert their kernel packaging, then they
can carry the patch and support it quite easily. The code doesn't
change frequently so it should not be a big roadblock

I'm more concerned about developer and hobbyists etc who don't have the
resources. But IIRC we are satisfied that git version has superseded
the benefits of modversions for that case now.

Thanks,
Nick