I pulled Linus' tree this morning to make sure there wasn't a fix
since -rc6, but I still get "no symbol version" for modules on boot.
I'm running Debian stretch (gcc 6.2.0-13) on amd64 and build the
kernel with "make -j 8 bindeb-pkg".
I'm using CONFIG_VMAP_STACK=y
If I set CONFIG_MODVERSIONS=n, then it boots fine.
I have LVM on top of LUKS.
There is supposedly a fix that was applied before -rc6 that was
supposed to resolve it, but it is still broken for me. I wanted to
raise the issue and offer help in debugging/testing patches to fix the
issue if it was unknown that this is still a problem.
Thank you.
----------------
Robert LeBlanc
PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1
Hi Robert,
you have to apply following patch also:
provide-asm-prototypes.h-for-x86.patch:
https://patchwork.kernel.org/patch/9408985/raw/
@Adam, Nick: Was this patch not yet sent to Linus?
greez, Phil
Am 01.11.2016 um 14:46 schrieb Nicholas Piggin:
> On Tue, 1 Nov 2016 13:48:59 +0100
> Philip Müller <[email protected]> wrote:
>
>> Hi Nicholas, hi Michal,
>>
>> due to following commit it seems the 64bit architecture of linux 4.9-rc
>> is not able to boot at all, as it is unable to find its root device:
>>
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=84d69848c97faab0c25aa2667b273404d2e2a64a
>>
>> This got reported by several users:
>>
>> https://forum.manjaro.org/t/linux-4-9-rc2-does-not-boot/11434
>>
>> Even after applying that patch it seems still to be unbootable:
>>
>> https://patchwork.kernel.org/patch/9406823/raw/
>>
>> It is claimed it would boot by disabling CONFIG_MODVERSIONS for now. I
>> still wonder why the 32bit version of the kernel boots without an issue.
>>
>> Any thoughts/plans on the matter from your end? We are happy to test
>> some patches if needed.
>>
>> kind regards, Philip
>
>
> Hi Philip,
>
> Thanks for reporting. You likely need to apply the x86 specific
> patch as well as the above one.
>
> http://marc.info/?l=linux-kernel&m=147669851906489&w=2
>
> Thanks,
> Nick
>
Phil,
I don't have those files. I'll patch and test.
Thanks,
----------------
Robert LeBlanc
PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1
On Wed, Nov 23, 2016 at 1:08 PM, Philip Müller <[email protected]> wrote:
> Hi Robert,
>
> you have to apply following patch also:
>
> provide-asm-prototypes.h-for-x86.patch:
> https://patchwork.kernel.org/patch/9408985/raw/
>
> @Adam, Nick: Was this patch not yet sent to Linus?
>
> greez, Phil
>
> Am 01.11.2016 um 14:46 schrieb Nicholas Piggin:
>> On Tue, 1 Nov 2016 13:48:59 +0100
>> Philip Müller <[email protected]> wrote:
>>
>>> Hi Nicholas, hi Michal,
>>>
>>> due to following commit it seems the 64bit architecture of linux 4.9-rc
>>> is not able to boot at all, as it is unable to find its root device:
>>>
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=84d69848c97faab0c25aa2667b273404d2e2a64a
>>>
>>> This got reported by several users:
>>>
>>> https://forum.manjaro.org/t/linux-4-9-rc2-does-not-boot/11434
>>>
>>> Even after applying that patch it seems still to be unbootable:
>>>
>>> https://patchwork.kernel.org/patch/9406823/raw/
>>>
>>> It is claimed it would boot by disabling CONFIG_MODVERSIONS for now. I
>>> still wonder why the 32bit version of the kernel boots without an issue.
>>>
>>> Any thoughts/plans on the matter from your end? We are happy to test
>>> some patches if needed.
>>>
>>> kind regards, Philip
>>
>>
>> Hi Philip,
>>
>> Thanks for reporting. You likely need to apply the x86 specific
>> patch as well as the above one.
>>
>> http://marc.info/?l=linux-kernel&m=147669851906489&w=2
>>
>> Thanks,
>> Nick
>>
>
Hi Robert,
Michal's and Nick's patch is already added to 4.9-rc6, however Adam's
patch is still missing for x86:
https://patchwork.kernel.org/patch/9408985/raw/
You can check on how I patch it on Manjaro here (line 75):
https://raw.githubusercontent.com/manjaro/packages-core/master/linux49/PKGBUILD
greez, Phil
On Wed, Nov 23, 2016 at 09:08:28PM +0100, Philip M?ller wrote:
> > due to following commit it seems the 64bit architecture of linux 4.9-rc
> > is not able to boot at all, as it is unable to find its root device:
> you have to apply following patch also:
>
> provide-asm-prototypes.h-for-x86.patch:
> https://patchwork.kernel.org/patch/9408985/raw/
>
> @Adam, Nick: Was this patch not yet sent to Linus?
The patch stewed in a kbuild-targetted thread since the morning after -rc1,
Nick has recently requested that it should go through x86 maintainers
instead. I've sent it there, lemme ping them, as the regression is severe
and 4.9-final is close.
Apologies if I'm doing something wrong, I'm not a real kernel dev and merely
was the person who came here looking for a fix, saw Nick's instructions
and did the legwork implementing them.
Last version (rewritten description) is at:
https://patchwork.kernel.org/patch/9439501/
(needs s/oeter/peter/ for a typo in Peter Wu's address)
Meow!
--
The bill declaring Jesus as the King of Poland fails to specify whether
the addition is at the top or end of the list of kings. What should the
historians do?
Confirmed that this patch does resolve the boot issue with
CONFIG_MODVERSIONS=y for me. Thank you for the patch and hopefully it
will get included in -rc7.
Thanks.
----------------
Robert LeBlanc
PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1
On Wed, Nov 23, 2016 at 1:53 PM, Adam Borowski <[email protected]> wrote:
> On Wed, Nov 23, 2016 at 09:08:28PM +0100, Philip Müller wrote:
>> > due to following commit it seems the 64bit architecture of linux 4.9-rc
>> > is not able to boot at all, as it is unable to find its root device:
>
>> you have to apply following patch also:
>>
>> provide-asm-prototypes.h-for-x86.patch:
>> https://patchwork.kernel.org/patch/9408985/raw/
>>
>> @Adam, Nick: Was this patch not yet sent to Linus?
>
> The patch stewed in a kbuild-targetted thread since the morning after -rc1,
> Nick has recently requested that it should go through x86 maintainers
> instead. I've sent it there, lemme ping them, as the regression is severe
> and 4.9-final is close.
>
> Apologies if I'm doing something wrong, I'm not a real kernel dev and merely
> was the person who came here looking for a fix, saw Nick's instructions
> and did the legwork implementing them.
>
> Last version (rewritten description) is at:
> https://patchwork.kernel.org/patch/9439501/
> (needs s/oeter/peter/ for a typo in Peter Wu's address)
>
>
> Meow!
> --
> The bill declaring Jesus as the King of Poland fails to specify whether
> the addition is at the top or end of the list of kings. What should the
> historians do?
Commit 4efca4ed ("kbuild: modversions for EXPORT_SYMBOL() for asm") adds
modversion support for symbols exported from asm files. Architectures
must include C-style declarations for those symbols in asm/asm-prototypes.h
in order for them to be versioned.
Add these declarations for x86, and an architecture-independent file that
can be used for common symbols.
User impact: kernels may fail to load modules at all when
CONFIG_MODVERSIONS=y.
Signed-off-by: Adam Borowski <[email protected]>
Tested-by: Kalle Valo <[email protected]>
Acked-by: Nicholas Piggin <[email protected]>
Tested-by: Peter Wu <[email protected]>
Tested-by: Oliver Hartkopp <[email protected]>
---
Changes: corrected Peter Wu's address, added Tested-by: Oliver.
This is an unsplit version (x86/include/ and include/ together).
arch/x86/include/asm/asm-prototypes.h | 12 ++++++++++++
include/asm-generic/asm-prototypes.h | 7 +++++++
2 files changed, 19 insertions(+)
create mode 100644 arch/x86/include/asm/asm-prototypes.h
create mode 100644 include/asm-generic/asm-prototypes.h
diff --git a/arch/x86/include/asm/asm-prototypes.h b/arch/x86/include/asm/asm-prototypes.h
new file mode 100644
index 0000000..ae87224
--- /dev/null
+++ b/arch/x86/include/asm/asm-prototypes.h
@@ -0,0 +1,12 @@
+#include <asm/ftrace.h>
+#include <asm/uaccess.h>
+#include <asm/string.h>
+#include <asm/page.h>
+#include <asm/checksum.h>
+
+#include <asm-generic/asm-prototypes.h>
+
+#include <asm/page.h>
+#include <asm/pgtable.h>
+#include <asm/special_insns.h>
+#include <asm/preempt.h>
diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
new file mode 100644
index 0000000..df13637
--- /dev/null
+++ b/include/asm-generic/asm-prototypes.h
@@ -0,0 +1,7 @@
+#include <linux/bitops.h>
+extern void *__memset(void *, int, __kernel_size_t);
+extern void *__memcpy(void *, const void *, __kernel_size_t);
+extern void *__memmove(void *, const void *, __kernel_size_t);
+extern void *memset(void *, int, __kernel_size_t);
+extern void *memcpy(void *, const void *, __kernel_size_t);
+extern void *memmove(void *, const void *, __kernel_size_t);
--
2.10.2
Am 23.11.2016 um 21:53 schrieb Adam Borowski:
> Last version (rewritten description) is at:
> https://patchwork.kernel.org/patch/9439501/
> (needs s/oeter/peter/ for a typo in Peter Wu's address)
Hi Adam,
good to know. I kept track on it. For me it is easy to use patches.
However it would be also good having it in the final 4.9 release.
You can also add me as ack and tested:
Acked-by: Philip Mueller <[email protected]>
Tested-by: Philip Mueller <[email protected]>
Thx for the patch again.
Greez, Phil
Thx Adam for the cleaned up patch ...
Acked-by: Philip Mueller <[email protected]>
Tested-by: Philip Mueller <[email protected]>
* Adam Borowski <[email protected]> wrote:
> Commit 4efca4ed ("kbuild: modversions for EXPORT_SYMBOL() for asm") adds
> modversion support for symbols exported from asm files. Architectures
> must include C-style declarations for those symbols in asm/asm-prototypes.h
> in order for them to be versioned.
>
> Add these declarations for x86, and an architecture-independent file that
> can be used for common symbols.
>
> User impact: kernels may fail to load modules at all when
> CONFIG_MODVERSIONS=y.
>
> Signed-off-by: Adam Borowski <[email protected]>
> Tested-by: Kalle Valo <[email protected]>
> Acked-by: Nicholas Piggin <[email protected]>
> Tested-by: Peter Wu <[email protected]>
> Tested-by: Oliver Hartkopp <[email protected]>
> ---
> Changes: corrected Peter Wu's address, added Tested-by: Oliver.
> This is an unsplit version (x86/include/ and include/ together).
>
> arch/x86/include/asm/asm-prototypes.h | 12 ++++++++++++
> include/asm-generic/asm-prototypes.h | 7 +++++++
> 2 files changed, 19 insertions(+)
> create mode 100644 arch/x86/include/asm/asm-prototypes.h
> create mode 100644 include/asm-generic/asm-prototypes.h
Michal, I'm quite unhappy about how the offending commit that broke modversions
for essentially _everyone_ who does more complex modular builds on x86 ended up
upstream:
commit 4efca4ed05cbdfd13ec3e8cb623fb77d6e4ab187
Author: Nicholas Piggin <[email protected]>
AuthorDate: Tue Nov 1 12:46:19 2016 +1100
Commit: Michal Marek <[email protected]>
CommitDate: Tue Nov 1 16:20:17 2016 +0100
kbuild: modversions for EXPORT_SYMBOL() for asm
Allow architectures to create asm/asm-prototypes.h file that
provides C prototypes for exported asm functions, which enables
proper CRC versions to be generated for them.
scripts/Makefile.build | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 72 insertions(+), 6 deletions(-)
It was applied 4 hours after it was sent in the -rc3 timeframe, and then it went
upstream in -rc5:
"Here are some regression fixes for kbuild:
- modversion support for exported asm symbols (Nick Piggin). The
affected architectures need separate patches adding
asm-prototypes.h.
... the fine merge log even says that the commit 'needs separate patches'!
It's still totally broken upstream and it didn't fix any regressions AFAICS (or if
it did then its changelog was very silent on that fact).
Why was such a complex patch applied and why isn't it reverted or fixed upstream?
Thanks,
Ingo
On Thu, 24 Nov 2016 05:40:28 +0100
Ingo Molnar <[email protected]> wrote:
> * Adam Borowski <[email protected]> wrote:
>
> > Commit 4efca4ed ("kbuild: modversions for EXPORT_SYMBOL() for asm") adds
> > modversion support for symbols exported from asm files. Architectures
> > must include C-style declarations for those symbols in asm/asm-prototypes.h
> > in order for them to be versioned.
> >
> > Add these declarations for x86, and an architecture-independent file that
> > can be used for common symbols.
> >
> > User impact: kernels may fail to load modules at all when
> > CONFIG_MODVERSIONS=y.
> >
> > Signed-off-by: Adam Borowski <[email protected]>
> > Tested-by: Kalle Valo <[email protected]>
> > Acked-by: Nicholas Piggin <[email protected]>
> > Tested-by: Peter Wu <[email protected]>
> > Tested-by: Oliver Hartkopp <[email protected]>
> > ---
> > Changes: corrected Peter Wu's address, added Tested-by: Oliver.
> > This is an unsplit version (x86/include/ and include/ together).
> >
> > arch/x86/include/asm/asm-prototypes.h | 12 ++++++++++++
> > include/asm-generic/asm-prototypes.h | 7 +++++++
> > 2 files changed, 19 insertions(+)
> > create mode 100644 arch/x86/include/asm/asm-prototypes.h
> > create mode 100644 include/asm-generic/asm-prototypes.h
>
> Michal, I'm quite unhappy about how the offending commit that broke modversions
> for essentially _everyone_ who does more complex modular builds on x86 ended up
> upstream:
>
> commit 4efca4ed05cbdfd13ec3e8cb623fb77d6e4ab187
> Author: Nicholas Piggin <[email protected]>
> AuthorDate: Tue Nov 1 12:46:19 2016 +1100
> Commit: Michal Marek <[email protected]>
> CommitDate: Tue Nov 1 16:20:17 2016 +0100
>
> kbuild: modversions for EXPORT_SYMBOL() for asm
>
> Allow architectures to create asm/asm-prototypes.h file that
> provides C prototypes for exported asm functions, which enables
> proper CRC versions to be generated for them.
What did this break? It's the first I've heard of it. For all architectures
without asm/asm-prototypes.h it should have been a functional noop. Any
breakage is some bug in my patch so that would need to be fixed urgently.
>
> scripts/Makefile.build | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 72 insertions(+), 6 deletions(-)
>
> It was applied 4 hours after it was sent in the -rc3 timeframe, and then it went
> upstream in -rc5:
>
> "Here are some regression fixes for kbuild:
>
> - modversion support for exported asm symbols (Nick Piggin). The
> affected architectures need separate patches adding
> asm-prototypes.h.
>
> ... the fine merge log even says that the commit 'needs separate patches'!
>
> It's still totally broken upstream and it didn't fix any regressions AFAICS (or if
> it did then its changelog was very silent on that fact).
Well it doesn't fix regression by itself, as discussed it needs architecture
patches. I've tried keeping linux-arch on cc for all this modversion breakage
stuff since it became clear it would require arch changes.
The actual x86 bug I suppose you would say is caused by 784d5699eddc5. But I
should probably have included more background in the above initial crc support
patch, e.g, at least reference 22823ab419d. So mea culpa for that.
> Why was such a complex patch applied and why isn't it reverted or fixed upstream?
It's been discussed and reviewed and tested for a long time (mainly on
linux-arch and linux-kbuild) and simply taken a while to find the least nasty
way to get 4.9 working.
The real problem is that this regression was found very late because it seems
very specific to the exact build environment. Simply enabling modversions was
not enough to break it on all configs (you would silently get 0 CRCs) so it
slipped through despite build tests. Then it took a quite a while longer to
settle on how to fix it.
Thanks,
Nick
* Nicholas Piggin <[email protected]> wrote:
> > scripts/Makefile.build | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 72 insertions(+), 6 deletions(-)
> >
> > It was applied 4 hours after it was sent in the -rc3 timeframe, and then it went
> > upstream in -rc5:
> >
> > "Here are some regression fixes for kbuild:
> >
> > - modversion support for exported asm symbols (Nick Piggin). The
> > affected architectures need separate patches adding
> > asm-prototypes.h.
> >
> > ... the fine merge log even says that the commit 'needs separate patches'!
> >
> > It's still totally broken upstream and it didn't fix any regressions AFAICS (or if
> > it did then its changelog was very silent on that fact).
>
> Well it doesn't fix regression by itself, as discussed it needs architecture
> patches. I've tried keeping linux-arch on cc for all this modversion breakage
> stuff since it became clear it would require arch changes.
>
> The actual x86 bug I suppose you would say is caused by 784d5699eddc5. But I
> should probably have included more background in the above initial crc support
> patch, e.g, at least reference 22823ab419d. So mea culpa for that.
Indeed 784d5699eddc5 makes more sense:
784d5699eddc ("x86: move exports to actual definitions")
22823ab419d8 ("EXPORT_SYMBOL() for asm")
... and sorry about coming down on you and Marek!
I've Cc:-ed Al.
I think what happened is that 22823ab419d8 and 784d5699eddc caused the boot
regression (modular builds with modversions enabled not booting), and your fix
half-fixed it - with the remaining fix (that adds the header to x86) fixing the
rest.
Still the fact remains that modversions was broken in -rc1 which delayed testing
done by a number of prominent testers. :-(
Thanks,
Ingo
On Thu, 24 Nov 2016 07:00:50 +0100
Ingo Molnar <[email protected]> wrote:
> * Nicholas Piggin <[email protected]> wrote:
>
> > > scripts/Makefile.build | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> > > 1 file changed, 72 insertions(+), 6 deletions(-)
> > >
> > > It was applied 4 hours after it was sent in the -rc3 timeframe, and then it went
> > > upstream in -rc5:
> > >
> > > "Here are some regression fixes for kbuild:
> > >
> > > - modversion support for exported asm symbols (Nick Piggin). The
> > > affected architectures need separate patches adding
> > > asm-prototypes.h.
> > >
> > > ... the fine merge log even says that the commit 'needs separate patches'!
> > >
> > > It's still totally broken upstream and it didn't fix any regressions AFAICS (or if
> > > it did then its changelog was very silent on that fact).
> >
> > Well it doesn't fix regression by itself, as discussed it needs architecture
> > patches. I've tried keeping linux-arch on cc for all this modversion breakage
> > stuff since it became clear it would require arch changes.
> >
> > The actual x86 bug I suppose you would say is caused by 784d5699eddc5. But I
> > should probably have included more background in the above initial crc support
> > patch, e.g, at least reference 22823ab419d. So mea culpa for that.
>
> Indeed 784d5699eddc5 makes more sense:
>
> 784d5699eddc ("x86: move exports to actual definitions")
> 22823ab419d8 ("EXPORT_SYMBOL() for asm")
>
> ... and sorry about coming down on you and Marek!
>
> I've Cc:-ed Al.
>
> I think what happened is that 22823ab419d8 and 784d5699eddc caused the boot
> regression (modular builds with modversions enabled not booting), and your fix
> half-fixed it - with the remaining fix (that adds the header to x86) fixing the
> rest.
That's about right. My patch *should* be a noop by itself (just provides the
framework for x86 fix to work). So if you notice any new breakage let me
know.
>
> Still the fact remains that modversions was broken in -rc1 which delayed testing
> done by a number of prominent testers. :-(
Yep, not ideal. I have a patch or so which is supposed to make CRC failure
warnings more reliable.
But still, modversions is pretty complicated for what it gives us. It sends
preprocessed C into a C parser that makes CRCs using type definitions of
exported symbols, then turns those CRCs into a linker script which which is
used to link the .o file with. What we get in return is a quite limited and
symbol "versioning" system.
What if we ripped all that out and just attached an explicit version to
each export, and incompatible changes require an increment? Google tells me
Linus is not a neutral bystander on the topic of symbol versioning, so I'm
bracing for a robust response :) (actually I don't much care either way, I'm
happy to put a couple of bandaids on it and keep it going)
Thanks,
Nick
On Thu, Nov 24, 2016 at 06:20:26PM +1100, Nicholas Piggin wrote:
> But still, modversions is pretty complicated for what it gives us. It sends
> preprocessed C into a C parser that makes CRCs using type definitions of
> exported symbols, then turns those CRCs into a linker script which which is
> used to link the .o file with. What we get in return is a quite limited and
> symbol "versioning" system.
>
> What if we ripped all that out and just attached an explicit version to
> each export, and incompatible changes require an increment?
How would that work for structures? Would that be required for every
EXPORT_SYMBOL* somehow?
> Google tells me
> Linus is not a neutral bystander on the topic of symbol versioning, so I'm
> bracing for a robust response :) (actually I don't much care either way, I'm
> happy to put a couple of bandaids on it and keep it going)
There are tools that people are working on to make it more obvious where
API breaks happen by looking at the .o debug data instead of our crazy
current system (which is really better than nothing), perhaps we should
start using them instead?
See here for more details about this:
https://kernel-recipes.org/en/2016/talks/would-an-abi-changes-visualization-tool-be-useful-to-linux-kernel-maintenance/
thanks,
greg k-h
On Thu, 24 Nov 2016 08:36:39 +0100
Greg Kroah-Hartman <[email protected]> wrote:
> On Thu, Nov 24, 2016 at 06:20:26PM +1100, Nicholas Piggin wrote:
> > But still, modversions is pretty complicated for what it gives us. It sends
> > preprocessed C into a C parser that makes CRCs using type definitions of
> > exported symbols, then turns those CRCs into a linker script which which is
> > used to link the .o file with. What we get in return is a quite limited and
> > symbol "versioning" system.
> >
> > What if we ripped all that out and just attached an explicit version to
> > each export, and incompatible changes require an increment?
>
> How would that work for structures? Would that be required for every
> EXPORT_SYMBOL* somehow?
Yeah just have EXPORT_SYMBOL take another parameter which attaches a version
number and use that as the value for the __crc_ symbol versions rather than
a calculated CRC.
Yes it would require some level of care from developers and may be a small
annoyance when changing exports. But making people think a tiny bit more
before chnaging exported ABI shouldn't be the end of the world.
>
> > Google tells me
> > Linus is not a neutral bystander on the topic of symbol versioning, so I'm
> > bracing for a robust response :) (actually I don't much care either way, I'm
> > happy to put a couple of bandaids on it and keep it going)
>
> There are tools that people are working on to make it more obvious where
> API breaks happen by looking at the .o debug data instead of our crazy
> current system (which is really better than nothing), perhaps we should
> start using them instead?
>
> See here for more details about this:
> https://kernel-recipes.org/en/2016/talks/would-an-abi-changes-visualization-tool-be-useful-to-linux-kernel-maintenance/
Hmm. I guess it's basically similar to modversions, so has downsides of not
detecting a semantic change unless it changes the type. But still, if we could
replace our custom code with a tool like this for modversions functionality,
that alone would be a massive improvement. But requiring debug info might be
a bit of a show stopper. I also don't know if that would handle asm functions.
Thanks,
Nick
On 2016-11-24 05:40, Ingo Molnar wrote:
>
> * Adam Borowski <[email protected]> wrote:
>
>> Commit 4efca4ed ("kbuild: modversions for EXPORT_SYMBOL() for asm") adds
>> modversion support for symbols exported from asm files. Architectures
>> must include C-style declarations for those symbols in asm/asm-prototypes.h
>> in order for them to be versioned.
>>
>> Add these declarations for x86, and an architecture-independent file that
>> can be used for common symbols.
>>
>> User impact: kernels may fail to load modules at all when
>> CONFIG_MODVERSIONS=y.
>>
>> Signed-off-by: Adam Borowski <[email protected]>
>> Tested-by: Kalle Valo <[email protected]>
>> Acked-by: Nicholas Piggin <[email protected]>
>> Tested-by: Peter Wu <[email protected]>
>> Tested-by: Oliver Hartkopp <[email protected]>
>> ---
>> Changes: corrected Peter Wu's address, added Tested-by: Oliver.
>> This is an unsplit version (x86/include/ and include/ together).
>>
>> arch/x86/include/asm/asm-prototypes.h | 12 ++++++++++++
>> include/asm-generic/asm-prototypes.h | 7 +++++++
>> 2 files changed, 19 insertions(+)
>> create mode 100644 arch/x86/include/asm/asm-prototypes.h
>> create mode 100644 include/asm-generic/asm-prototypes.h
>
> Michal, I'm quite unhappy about how the offending commit that broke modversions
> for essentially _everyone_ who does more complex modular builds on x86 ended up
> upstream:
>
> commit 4efca4ed05cbdfd13ec3e8cb623fb77d6e4ab187
> Author: Nicholas Piggin <[email protected]>
> AuthorDate: Tue Nov 1 12:46:19 2016 +1100
> Commit: Michal Marek <[email protected]>
> CommitDate: Tue Nov 1 16:20:17 2016 +0100
>
> kbuild: modversions for EXPORT_SYMBOL() for asm
>
> Allow architectures to create asm/asm-prototypes.h file that
> provides C prototypes for exported asm functions, which enables
> proper CRC versions to be generated for them.
>
> scripts/Makefile.build | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 72 insertions(+), 6 deletions(-)
>
> It was applied 4 hours after it was sent in the -rc3 timeframe, and then it went
> upstream in -rc5:
>
> "Here are some regression fixes for kbuild:
>
> - modversion support for exported asm symbols (Nick Piggin). The
> affected architectures need separate patches adding
> asm-prototypes.h.
>
> ... the fine merge log even says that the commit 'needs separate patches'!
It needs separate patches to finally fix the regression that happened in
-rc1.
Michal
On 2016-11-24 08:53, Nicholas Piggin wrote:
> On Thu, 24 Nov 2016 08:36:39 +0100
> Greg Kroah-Hartman <[email protected]> wrote:
>
>> On Thu, Nov 24, 2016 at 06:20:26PM +1100, Nicholas Piggin wrote:
>>> But still, modversions is pretty complicated for what it gives us. It sends
>>> preprocessed C into a C parser that makes CRCs using type definitions of
>>> exported symbols, then turns those CRCs into a linker script which which is
>>> used to link the .o file with. What we get in return is a quite limited and
>>> symbol "versioning" system.
>>>
>>> What if we ripped all that out and just attached an explicit version to
>>> each export, and incompatible changes require an increment?
>>
>> How would that work for structures? Would that be required for every
>> EXPORT_SYMBOL* somehow?
>
> Yeah just have EXPORT_SYMBOL take another parameter which attaches a version
> number and use that as the value for the __crc_ symbol versions rather than
> a calculated CRC.
The problem is that with every kernel release, the structures change in
a way that you would have to bump the version of virtually every export.
At which point, there would be little difference between
CONFIG_MODVERSION on and off (without CONFIG_MODVERSION, we compare the
kernel version strings when loading modules).
Michal
On Thursday, November 24, 2016 6:53:22 PM CET Nicholas Piggin wrote:
> >
> > > Google tells me
> > > Linus is not a neutral bystander on the topic of symbol versioning, so I'm
> > > bracing for a robust response (actually I don't much care either way, I'm
> > > happy to put a couple of bandaids on it and keep it going)
> >
> > There are tools that people are working on to make it more obvious where
> > API breaks happen by looking at the .o debug data instead of our crazy
> > current system (which is really better than nothing), perhaps we should
> > start using them instead?
> >
> > See here for more details about this:
> > https://kernel-recipes.org/en/2016/talks/would-an-abi-changes-visualization-tool-be-useful-to-linux-kernel-maintenance/
>
> Hmm. I guess it's basically similar to modversions, so has downsides of not
> detecting a semantic change unless it changes the type. But still, if we could
> replace our custom code with a tool like this for modversions functionality,
> that alone would be a massive improvement. But requiring debug info might be
> a bit of a show stopper. I also don't know if that would handle asm functions.
It's certainly not an option for v4.9 at this point. There is also no
realistic way we can get a correct asm/asm-prototypes.h for all the
other architectures in place. At the moment, powerpc is the only
one that actually works with modversions.
We can either make CONFIG_MODVERSIONS a per-architecture opt-in
and let only the ones that have the header file select that, or
revert all of Al's original patches that moved the exports.
Arnd
On Thu, Nov 24, 2016 at 06:53:22PM +1100, Nicholas Piggin wrote:
> On Thu, 24 Nov 2016 08:36:39 +0100
> Greg Kroah-Hartman <[email protected]> wrote:
>
> > On Thu, Nov 24, 2016 at 06:20:26PM +1100, Nicholas Piggin wrote:
> > > But still, modversions is pretty complicated for what it gives us. It sends
> > > preprocessed C into a C parser that makes CRCs using type definitions of
> > > exported symbols, then turns those CRCs into a linker script which which is
> > > used to link the .o file with. What we get in return is a quite limited and
> > > symbol "versioning" system.
> > >
> > > What if we ripped all that out and just attached an explicit version to
> > > each export, and incompatible changes require an increment?
> >
> > How would that work for structures? Would that be required for every
> > EXPORT_SYMBOL* somehow?
>
> Yeah just have EXPORT_SYMBOL take another parameter which attaches a version
> number and use that as the value for the __crc_ symbol versions rather than
> a calculated CRC.
>
> Yes it would require some level of care from developers and may be a small
> annoyance when changing exports. But making people think a tiny bit more
> before chnaging exported ABI shouldn't be the end of the world.
That wouldn't work at all for structures that change, as we never
explicitly "mark" them for export anywhere. You need a tool that looks
at either the source code (what we have today), or looks at the
object/debugging code (like the link I pointed at.)
> > > Google tells me
> > > Linus is not a neutral bystander on the topic of symbol versioning, so I'm
> > > bracing for a robust response :) (actually I don't much care either way, I'm
> > > happy to put a couple of bandaids on it and keep it going)
> >
> > There are tools that people are working on to make it more obvious where
> > API breaks happen by looking at the .o debug data instead of our crazy
> > current system (which is really better than nothing), perhaps we should
> > start using them instead?
> >
> > See here for more details about this:
> > https://kernel-recipes.org/en/2016/talks/would-an-abi-changes-visualization-tool-be-useful-to-linux-kernel-maintenance/
>
> Hmm. I guess it's basically similar to modversions, so has downsides of not
> detecting a semantic change unless it changes the type. But still, if we could
> replace our custom code with a tool like this for modversions functionality,
> that alone would be a massive improvement. But requiring debug info might be
> a bit of a show stopper. I also don't know if that would handle asm functions.
I think we can live without asm functions changing their arguments as
that is usually very rare. And maybe debugging info being a requirement
for those that want modversions (i.e. the distros), is ok as they
already generate that as part of their build.
But more importantly, that's a much longer-term solution, fixing what we
have today to at least start working again is much more important before
we start bikeshedding the whole mess :)
thanks,
greg k-h
On Thu, 24 Nov 2016 10:38:04 +0100
Arnd Bergmann <[email protected]> wrote:
> On Thursday, November 24, 2016 6:53:22 PM CET Nicholas Piggin wrote:
> > >
> > > > Google tells me
> > > > Linus is not a neutral bystander on the topic of symbol versioning, so I'm
> > > > bracing for a robust response (actually I don't much care either way, I'm
> > > > happy to put a couple of bandaids on it and keep it going)
> > >
> > > There are tools that people are working on to make it more obvious where
> > > API breaks happen by looking at the .o debug data instead of our crazy
> > > current system (which is really better than nothing), perhaps we should
> > > start using them instead?
> > >
> > > See here for more details about this:
> > > https://kernel-recipes.org/en/2016/talks/would-an-abi-changes-visualization-tool-be-useful-to-linux-kernel-maintenance/
> >
> > Hmm. I guess it's basically similar to modversions, so has downsides of not
> > detecting a semantic change unless it changes the type. But still, if we could
> > replace our custom code with a tool like this for modversions functionality,
> > that alone would be a massive improvement. But requiring debug info might be
> > a bit of a show stopper. I also don't know if that would handle asm functions.
>
> It's certainly not an option for v4.9 at this point. There is also no
Yeah I wasn't suggesting that for 4.9, or 4.10 even.
> realistic way we can get a correct asm/asm-prototypes.h for all the
> other architectures in place. At the moment, powerpc is the only
> one that actually works with modversions.
>
> We can either make CONFIG_MODVERSIONS a per-architecture opt-in
> and let only the ones that have the header file select that, or
> revert all of Al's original patches that moved the exports.
alpha, m68k, s390, sparc, ia64 are affected and have no patch (or none
I've been cc'ed on) for asm-prototypes.h. Doesn't seem infeasible to add
them before 4.9, considering the fairly low risk of patch. Opt-in would
be pointless IMO, might as well just revert the arch part of Al's patch
instead.
Thanks,
Nick
On Thu, 24 Nov 2016 10:32:12 +0100
Michal Marek <[email protected]> wrote:
> On 2016-11-24 08:53, Nicholas Piggin wrote:
> > On Thu, 24 Nov 2016 08:36:39 +0100
> > Greg Kroah-Hartman <[email protected]> wrote:
> >
> >> On Thu, Nov 24, 2016 at 06:20:26PM +1100, Nicholas Piggin wrote:
> >>> But still, modversions is pretty complicated for what it gives us. It sends
> >>> preprocessed C into a C parser that makes CRCs using type definitions of
> >>> exported symbols, then turns those CRCs into a linker script which which is
> >>> used to link the .o file with. What we get in return is a quite limited and
> >>> symbol "versioning" system.
> >>>
> >>> What if we ripped all that out and just attached an explicit version to
> >>> each export, and incompatible changes require an increment?
> >>
> >> How would that work for structures? Would that be required for every
> >> EXPORT_SYMBOL* somehow?
> >
> > Yeah just have EXPORT_SYMBOL take another parameter which attaches a version
> > number and use that as the value for the __crc_ symbol versions rather than
> > a calculated CRC.
>
> The problem is that with every kernel release, the structures change in
> a way that you would have to bump the version of virtually every export.
>
> At which point, there would be little difference between
> CONFIG_MODVERSION on and off (without CONFIG_MODVERSION, we compare the
> kernel version strings when loading modules).
I'm not sure about that. If they are truly incompatible changes and
MODVERSIONS does not pick up a different CRC, then it's even worse if
incompatible modules are missed so often.
Thanks,
Nick
On Thu, 24 Nov 2016 10:56:22 +0100
Greg Kroah-Hartman <[email protected]> wrote:
> On Thu, Nov 24, 2016 at 06:53:22PM +1100, Nicholas Piggin wrote:
> > On Thu, 24 Nov 2016 08:36:39 +0100
> > Greg Kroah-Hartman <[email protected]> wrote:
> >
> > > On Thu, Nov 24, 2016 at 06:20:26PM +1100, Nicholas Piggin wrote:
> > > > But still, modversions is pretty complicated for what it gives us. It sends
> > > > preprocessed C into a C parser that makes CRCs using type definitions of
> > > > exported symbols, then turns those CRCs into a linker script which which is
> > > > used to link the .o file with. What we get in return is a quite limited and
> > > > symbol "versioning" system.
> > > >
> > > > What if we ripped all that out and just attached an explicit version to
> > > > each export, and incompatible changes require an increment?
> > >
> > > How would that work for structures? Would that be required for every
> > > EXPORT_SYMBOL* somehow?
> >
> > Yeah just have EXPORT_SYMBOL take another parameter which attaches a version
> > number and use that as the value for the __crc_ symbol versions rather than
> > a calculated CRC.
> >
> > Yes it would require some level of care from developers and may be a small
> > annoyance when changing exports. But making people think a tiny bit more
> > before chnaging exported ABI shouldn't be the end of the world.
>
> That wouldn't work at all for structures that change, as we never
> explicitly "mark" them for export anywhere.
Well, the module arrives at the objects one way or another via an exported
symbol. Although it can be by following a lot of pointers so yes it's
probably near impossible to do well.
> You need a tool that looks
> at either the source code (what we have today), or looks at the
What we have today only looks at the type of the exported function or
variable I think (or does it? I didn't look that far into the parser).
Does not follow down all possible derivable pointer types.
> object/debugging code (like the link I pointed at.)
>
> > > > Google tells me
> > > > Linus is not a neutral bystander on the topic of symbol versioning, so I'm
> > > > bracing for a robust response :) (actually I don't much care either way, I'm
> > > > happy to put a couple of bandaids on it and keep it going)
> > >
> > > There are tools that people are working on to make it more obvious where
> > > API breaks happen by looking at the .o debug data instead of our crazy
> > > current system (which is really better than nothing), perhaps we should
> > > start using them instead?
> > >
> > > See here for more details about this:
> > > https://kernel-recipes.org/en/2016/talks/would-an-abi-changes-visualization-tool-be-useful-to-linux-kernel-maintenance/
> >
> > Hmm. I guess it's basically similar to modversions, so has downsides of not
> > detecting a semantic change unless it changes the type. But still, if we could
> > replace our custom code with a tool like this for modversions functionality,
> > that alone would be a massive improvement. But requiring debug info might be
> > a bit of a show stopper. I also don't know if that would handle asm functions.
>
> I think we can live without asm functions changing their arguments as
> that is usually very rare. And maybe debugging info being a requirement
> for those that want modversions (i.e. the distros), is ok as they
> already generate that as part of their build.
Maybe. I'd like to know how people really care about it. Linus post from
http://yarchive.net/comp/linux/modversions.html
Seem to be that he just likes it to prevent module loading if the git version
is not available. Fair usage, but could we do better with less effort? Maybe
ship with a source version that can do the same job. If you take care of that
case, then what is left?
>
> But more importantly, that's a much longer-term solution, fixing what we
> have today to at least start working again is much more important before
> we start bikeshedding the whole mess :)
Oh yeah, we're fixing it. I just thought I'd bring it up since I have a few
important ears :)
Thanks,
Nick
Dne 24.11.2016 v 11:03 Nicholas Piggin napsal(a):
> On Thu, 24 Nov 2016 10:32:12 +0100
> Michal Marek <[email protected]> wrote:
>
>> On 2016-11-24 08:53, Nicholas Piggin wrote:
>>> On Thu, 24 Nov 2016 08:36:39 +0100
>>> Greg Kroah-Hartman <[email protected]> wrote:
>>>
>>>> On Thu, Nov 24, 2016 at 06:20:26PM +1100, Nicholas Piggin wrote:
>>>>> But still, modversions is pretty complicated for what it gives us. It sends
>>>>> preprocessed C into a C parser that makes CRCs using type definitions of
>>>>> exported symbols, then turns those CRCs into a linker script which which is
>>>>> used to link the .o file with. What we get in return is a quite limited and
>>>>> symbol "versioning" system.
>>>>>
>>>>> What if we ripped all that out and just attached an explicit version to
>>>>> each export, and incompatible changes require an increment?
>>>>
>>>> How would that work for structures? Would that be required for every
>>>> EXPORT_SYMBOL* somehow?
>>>
>>> Yeah just have EXPORT_SYMBOL take another parameter which attaches a version
>>> number and use that as the value for the __crc_ symbol versions rather than
>>> a calculated CRC.
>>
>> The problem is that with every kernel release, the structures change in
>> a way that you would have to bump the version of virtually every export.
>>
>> At which point, there would be little difference between
>> CONFIG_MODVERSION on and off (without CONFIG_MODVERSION, we compare the
>> kernel version strings when loading modules).
>
>
> I'm not sure about that. If they are truly incompatible changes and
> MODVERSIONS does not pick up a different CRC, then it's even worse if
> incompatible modules are missed so often.
Now I'm confused: Are you suggesting to do this manual symbol versioning
for all exports, or only those defined in asm?
Thanks,
Michal
(Adding Thorsten because this is a serious regression and still not fixed)
Adam Borowski <[email protected]> writes:
> Commit 4efca4ed ("kbuild: modversions for EXPORT_SYMBOL() for asm") adds
> modversion support for symbols exported from asm files. Architectures
> must include C-style declarations for those symbols in asm/asm-prototypes.h
> in order for them to be versioned.
>
> Add these declarations for x86, and an architecture-independent file that
> can be used for common symbols.
>
> User impact: kernels may fail to load modules at all when
> CONFIG_MODVERSIONS=y.
>
> Signed-off-by: Adam Borowski <[email protected]>
> Tested-by: Kalle Valo <[email protected]>
> Acked-by: Nicholas Piggin <[email protected]>
> Tested-by: Peter Wu <[email protected]>
> Tested-by: Oliver Hartkopp <[email protected]>
Calling it "may fail" is an understatement, for me _every_ 4.9-rc build
I have tried on my x86 32 bit test laptop has had broken module loading
and hence unable to boot. PLEASE fix this in Linus tree before he
releases 4.9, either applying this patch (which I currently apply
separately everytime I upgrade the kernel) or reverting the offending
commit which started the whole mess. It's not cool to keep the build
broken for this long, people have wasted lots of time because of this.
--
Kalle Valo
On Thu, Nov 24, 2016 at 09:31:52PM +1100, Nicholas Piggin wrote:
> On Thu, 24 Nov 2016 10:56:22 +0100
> Greg Kroah-Hartman <[email protected]> wrote:
>
> > On Thu, Nov 24, 2016 at 06:53:22PM +1100, Nicholas Piggin wrote:
> > > On Thu, 24 Nov 2016 08:36:39 +0100
> > > Greg Kroah-Hartman <[email protected]> wrote:
> > >
> > > > On Thu, Nov 24, 2016 at 06:20:26PM +1100, Nicholas Piggin wrote:
> > > > > But still, modversions is pretty complicated for what it gives us. It sends
> > > > > preprocessed C into a C parser that makes CRCs using type definitions of
> > > > > exported symbols, then turns those CRCs into a linker script which which is
> > > > > used to link the .o file with. What we get in return is a quite limited and
> > > > > symbol "versioning" system.
> > > > >
> > > > > What if we ripped all that out and just attached an explicit version to
> > > > > each export, and incompatible changes require an increment?
> > > >
> > > > How would that work for structures? Would that be required for every
> > > > EXPORT_SYMBOL* somehow?
> > >
> > > Yeah just have EXPORT_SYMBOL take another parameter which attaches a version
> > > number and use that as the value for the __crc_ symbol versions rather than
> > > a calculated CRC.
> > >
> > > Yes it would require some level of care from developers and may be a small
> > > annoyance when changing exports. But making people think a tiny bit more
> > > before chnaging exported ABI shouldn't be the end of the world.
> >
> > That wouldn't work at all for structures that change, as we never
> > explicitly "mark" them for export anywhere.
>
> Well, the module arrives at the objects one way or another via an exported
> symbol. Although it can be by following a lot of pointers so yes it's
> probably near impossible to do well.
Yes, manual "marking" is never going to be a viable solution.
> > You need a tool that looks
> > at either the source code (what we have today), or looks at the
>
> What we have today only looks at the type of the exported function or
> variable I think (or does it? I didn't look that far into the parser).
It should catch things if you change a structure layout of something
that is an argument in a function (like a pointer to a structure),
otherwise it wouldn't really be that good of a check, and kind of
useless.
> Does not follow down all possible derivable pointer types.
It should be pretty good, as I think the code is based on the old SuSE
scripts that used to do this really well. But it's been a long time
since I looked at it, so I could be wrong.
> > > > > Google tells me
> > > > > Linus is not a neutral bystander on the topic of symbol versioning, so I'm
> > > > > bracing for a robust response :) (actually I don't much care either way, I'm
> > > > > happy to put a couple of bandaids on it and keep it going)
> > > >
> > > > There are tools that people are working on to make it more obvious where
> > > > API breaks happen by looking at the .o debug data instead of our crazy
> > > > current system (which is really better than nothing), perhaps we should
> > > > start using them instead?
> > > >
> > > > See here for more details about this:
> > > > https://kernel-recipes.org/en/2016/talks/would-an-abi-changes-visualization-tool-be-useful-to-linux-kernel-maintenance/
> > >
> > > Hmm. I guess it's basically similar to modversions, so has downsides of not
> > > detecting a semantic change unless it changes the type. But still, if we could
> > > replace our custom code with a tool like this for modversions functionality,
> > > that alone would be a massive improvement. But requiring debug info might be
> > > a bit of a show stopper. I also don't know if that would handle asm functions.
> >
> > I think we can live without asm functions changing their arguments as
> > that is usually very rare. And maybe debugging info being a requirement
> > for those that want modversions (i.e. the distros), is ok as they
> > already generate that as part of their build.
>
> Maybe. I'd like to know how people really care about it. Linus post from
>
> http://yarchive.net/comp/linux/modversions.html
>
> Seem to be that he just likes it to prevent module loading if the git version
> is not available. Fair usage, but could we do better with less effort? Maybe
> ship with a source version that can do the same job. If you take care of that
> case, then what is left?
The goal is to be able to tell when a symbol changed somehow (structure
or function signature) and if it has, then to hopefully prevent loading
a module that doesn't have the same signature. The distros really want
this as they want "external" modules to load properly, even when they
bump their main kernel package version. And it's a good goal to have,
no need to rebuild external packages (that usually come from external
places) if you don't have to, as sometimes you need those modules to
have your machine to work properly (like the fibre channel mess of
out-of-tree drivers...)
So however that type of checking is done, is fine with me, I have no
real desire to mess with this as personally, I never use it for my own
machines (I just use module signing and then throw away the key after
building the kernel).
thanks,
greg k-h
On Thu, 24 Nov 2016 16:24:10 +0100
Greg Kroah-Hartman <[email protected]> wrote:
> On Thu, Nov 24, 2016 at 09:31:52PM +1100, Nicholas Piggin wrote:
> > On Thu, 24 Nov 2016 10:56:22 +0100
> > Greg Kroah-Hartman <[email protected]> wrote:
> >
> > > On Thu, Nov 24, 2016 at 06:53:22PM +1100, Nicholas Piggin wrote:
> > > > On Thu, 24 Nov 2016 08:36:39 +0100
> > > > Greg Kroah-Hartman <[email protected]> wrote:
> > > >
> > > > > On Thu, Nov 24, 2016 at 06:20:26PM +1100, Nicholas Piggin wrote:
> > > > > > But still, modversions is pretty complicated for what it gives us. It sends
> > > > > > preprocessed C into a C parser that makes CRCs using type definitions of
> > > > > > exported symbols, then turns those CRCs into a linker script which which is
> > > > > > used to link the .o file with. What we get in return is a quite limited and
> > > > > > symbol "versioning" system.
> > > > > >
> > > > > > What if we ripped all that out and just attached an explicit version to
> > > > > > each export, and incompatible changes require an increment?
> > > > >
> > > > > How would that work for structures? Would that be required for every
> > > > > EXPORT_SYMBOL* somehow?
> > > >
> > > > Yeah just have EXPORT_SYMBOL take another parameter which attaches a version
> > > > number and use that as the value for the __crc_ symbol versions rather than
> > > > a calculated CRC.
> > > >
> > > > Yes it would require some level of care from developers and may be a small
> > > > annoyance when changing exports. But making people think a tiny bit more
> > > > before chnaging exported ABI shouldn't be the end of the world.
> > >
> > > That wouldn't work at all for structures that change, as we never
> > > explicitly "mark" them for export anywhere.
> >
> > Well, the module arrives at the objects one way or another via an exported
> > symbol. Although it can be by following a lot of pointers so yes it's
> > probably near impossible to do well.
>
> 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.
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?
> > > You need a tool that looks
> > > at either the source code (what we have today), or looks at the
> >
> > What we have today only looks at the type of the exported function or
> > variable I think (or does it? I didn't look that far into the parser).
>
> It should catch things if you change a structure layout of something
> that is an argument in a function (like a pointer to a structure),
> otherwise it wouldn't really be that good of a check, and kind of
> useless.
>
> > Does not follow down all possible derivable pointer types.
>
> It should be pretty good, as I think the code is based on the old SuSE
> scripts that used to do this really well. But it's been a long time
> since I looked at it, so I could be wrong.
Yeah... turns out modversions is in the "kind of useless" camp.
The crc is based on the name of the type and that's it (that's what I
thought, I just now verify it).
> > > > > > Google tells me
> > > > > > Linus is not a neutral bystander on the topic of symbol versioning, so I'm
> > > > > > bracing for a robust response :) (actually I don't much care either way, I'm
> > > > > > happy to put a couple of bandaids on it and keep it going)
> > > > >
> > > > > There are tools that people are working on to make it more obvious where
> > > > > API breaks happen by looking at the .o debug data instead of our crazy
> > > > > current system (which is really better than nothing), perhaps we should
> > > > > start using them instead?
> > > > >
> > > > > See here for more details about this:
> > > > > https://kernel-recipes.org/en/2016/talks/would-an-abi-changes-visualization-tool-be-useful-to-linux-kernel-maintenance/
> > > >
> > > > Hmm. I guess it's basically similar to modversions, so has downsides of not
> > > > detecting a semantic change unless it changes the type. But still, if we could
> > > > replace our custom code with a tool like this for modversions functionality,
> > > > that alone would be a massive improvement. But requiring debug info might be
> > > > a bit of a show stopper. I also don't know if that would handle asm functions.
> > >
> > > I think we can live without asm functions changing their arguments as
> > > that is usually very rare. And maybe debugging info being a requirement
> > > for those that want modversions (i.e. the distros), is ok as they
> > > already generate that as part of their build.
> >
> > Maybe. I'd like to know how people really care about it. Linus post from
> >
> > http://yarchive.net/comp/linux/modversions.html
> >
> > Seem to be that he just likes it to prevent module loading if the git version
> > is not available. Fair usage, but could we do better with less effort? Maybe
> > ship with a source version that can do the same job. If you take care of that
> > case, then what is left?
>
> The goal is to be able to tell when a symbol changed somehow (structure
> or function signature) and if it has, then to hopefully prevent loading
> a module that doesn't have the same signature. The distros really want
> this as they want "external" modules to load properly, even when they
> bump their main kernel package version. And it's a good goal to have,
> no need to rebuild external packages (that usually come from external
> places) if you don't have to, as sometimes you need those modules to
> have your machine to work properly (like the fibre channel mess of
> out-of-tree drivers...)
>
> So however that type of checking is done, is fine with me, I have no
> real desire to mess with this as personally, I never use it for my own
> machines (I just use module signing and then throw away the key after
> building the kernel).
So we have:
1. The distro users. They don't break ABI, they really just want a way to
avoid the kernel version check.
2. Linus or other kernel developer who wants to prevent a kernel
accidentally loading out of date modules when they test some changes
that don't bump kernel version.
3. Advanced end user who does not want to have to recompile their nvidia
blob.
Anything else? So, how to handle them?
1. Distros may just want a way to avoid checking some minor part of the
version string. In rare cases where they do break ABI, they can just
rename the symbol: external modules have a distro/version compat layer
anyway.
2. I wonder if this is still important 8 years later now that everyone
uses git everywhere? :) I also don't think modversions helps this case
reliably because we don't change export types all that often. Can we
ship a git version in the source tree somehow that git can handle
specially?
3. These people today are not well supported with modversions because it
does not tell them about incompatibility of ABI. Semantics could change.
Structure args could change. Objects derived through various pointers
could change. We should not even bother trying IMO. Just let them do
forced loading at their own risk. We shouldn't offer a false sense of
security.
Thanks,
Nick
On Thu, Nov 24, 2016 at 4:40 PM, Nicholas Piggin <[email protected]> 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.
No. Because nobody else will care, so unless it's like a single symbol
or something, it will just be a maintenance nightmare.
> 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.
So I'd personally be ok with just saying "let's disable it for now",
and see if anybody even notices and cares, and then has a good enough
explanation of why. It's entirely possible that most users are "I
enabled it ten years ago, I didn't even realize it was still in my
defconfig".
Linus
On Fri, 25 Nov 2016 10:00:46 -0800
Linus Torvalds <[email protected]> wrote:
> On Thu, Nov 24, 2016 at 4:40 PM, Nicholas Piggin <[email protected]> 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.
>
> No. Because nobody else will care, so unless it's like a single symbol
> or something, it will just be a maintenance nightmare.
Yeah that's true, and as I realized a distro can rename a symbol if they
make incompatible changes which happens very rarely. Avoids having to
carry some whole infrastructure upstream for it.
>
> > 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.
>
> So I'd personally be ok with just saying "let's disable it for now",
> and see if anybody even notices and cares, and then has a good enough
> explanation of why. It's entirely possible that most users are "I
> enabled it ten years ago, I didn't even realize it was still in my
> defconfig".
That sounds good. Should we try to get 4.9 working (which we could
do relatively easily with a few arch reverts), and then disable
modversions for 4.10? (at which point we can un-revert Al's arch
patches)
Thanks,
Nick
Looks like Linus pushed cd3caefb4663e3811d37cc2afad3cce642d60061 a few
days ago effectively disabling modversions. I like this fix much
better, probably need an additional patch to reverse this new one as
well.
You can add me as well if needed.
Acked-by: Robert LeBlanc <[email protected]>
Tested-by: Robert LeBlanc <[email protected]>
----------------
Robert LeBlanc
PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1
On Wed, Nov 23, 2016 at 4:07 PM, Philip Müller <[email protected]> wrote:
> Am 23.11.2016 um 21:53 schrieb Adam Borowski:
>> Last version (rewritten description) is at:
>> https://patchwork.kernel.org/patch/9439501/
>> (needs s/oeter/peter/ for a typo in Peter Wu's address)
>
> Hi Adam,
>
> good to know. I kept track on it. For me it is easy to use patches.
> However it would be also good having it in the final 4.9 release.
>
> You can also add me as ack and tested:
>
> Acked-by: Philip Mueller <[email protected]>
> Tested-by: Philip Mueller <[email protected]>
>
> Thx for the patch again.
>
> Greez, Phil
[I've had to guess at the cc list for this, because we no longer have
mail archives that preserve them.]
On Fri, 2016-11-25 at 10:01 -0800, Linus Torvalds wrote:
> On Thu, Nov 24, 2016 at 4:40 PM, Nicholas Piggin <[email protected]> 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);
> > 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.
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.
Ben.
--
Ben Hutchings
Theory and practice are closer in theory than in practice.
- John Levine, moderator of
comp.compilers
On Tue, 29 Nov 2016 01:15:48 +0000
Ben Hutchings <[email protected]> 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 <[email protected]> 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
On Mon, Nov 28, 2016 at 5:15 PM, Ben Hutchings <[email protected]> wrote:
>>
>> 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:
Honestly, I'd just like to see actual real patches from people who
care about this.
The reason I disabled it entirely was simply that the discussions had
been going on forever, but nobody actually seemed to care enough to
just fix the damn thing. There was all the _noise_ about "look, here's
a patch", but nothing got sent to maintainers and actually actively
pushed as a "this fixes a regression".
At some point I just get fed up and say "this isn't worth the hot air
and endless pointless blathering".
What is the actual exact failure with MODVERSIONS today? IOW, if you
just remove the "broken", is it actually broken, and why? Because it
does work for me, I just got really tired of hearing about it, and
assuming it's just some broken toolchain or other case that I just
don't hit.
So somebody send me a minimal patch that is
(a) tested
(b) explains it
(c) obvious
and I'll happily re-enable modversions.
Linus
Dne 29.11.2016 v 03:31 Nicholas Piggin napsal(a):
> On Tue, 29 Nov 2016 01:15:48 +0000
> Ben Hutchings <[email protected]> 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 <[email protected]> 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:
I guess many distros have similar 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.
The thing is - to maintain an ABI version string, you need some level of
certainty that two given ABIs are really interchangeable. Which means
you need to check whether the symbols _and_ types exposed are unchanged.
Which is a thing that genksyms, the tool behind CONFIG_MODVERSIONS, does
quite well. So yes, you could do a testbuild with CONFIG_MODVERSIONS=y
and a production build with some global ABI string, but what's the point
then.
> 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.
I'd rather change the kconfig to
depends on BROKEN || <archs that have asm/asm-prototypes.h>
and eventuallly remove the dependency again. PPC has the header already,
so it can be added right away. I do not know why the x86 patch has not
been merged yet.
Michal
On Mon, Nov 28, 2016 at 08:08:57PM -0800, Linus Torvalds wrote:
> On Mon, Nov 28, 2016 at 5:15 PM, Ben Hutchings <[email protected]> wrote:
> >>
> >> 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:
>
> Honestly, I'd just like to see actual real patches from people who
> care about this.
>
> The reason I disabled it entirely was simply that the discussions had
> been going on forever, but nobody actually seemed to care enough to
> just fix the damn thing. There was all the _noise_ about "look, here's
> a patch", but nothing got sent to maintainers and actually actively
> pushed as a "this fixes a regression".
Here's some history:
The day of -rc1, multiple people immediately reported the breakage; it was
quickly found out that reverting 784d5699eddc fixes it. A "going forward"
patch has been posted but was insufficient; when the real devs went to bed
the last message was
https://www.mail-archive.com/[email protected]/msg1250370.html
which ends with instructions and "Care to do a patch for x86?".
Then a random person (me) did the legwork, gathered affected symbols, wrote
and tested the x86 patch. It was then tested by multiple people; Arnd
Bergmann wrote the ARM equivalent. Whenever a new lkml thread reporting the
breakage popped up, we pointed people to the patches and everyone was happy.
As for upstreaming, there was a delay because Michal Marek was on vacation.
Michal returned and sent you the pull request, you merged it as 04e36857 on
Nov 18. For some reason the per-arch pieces were excluded; I was instructed
to send my part to x86 maintainers.
I did so; the patch later got a better description by Nick and a bunch of
Tested-by -- but alas, nary a comment or action from x86 guys, despite
pings/resends (last one: https://lkml.org/lkml/2016/11/23/706). I guess I'm
lacking the secret handshake or something -- thus, it looks like it's my
fault, the rest of you can be blamed mostly for letting a
not-a-real-kernel-dev unsupervised.
On Nov 24 finally Ingo responded, the discussion ended with you marking
modversions as BROKEN.
> So somebody send me a minimal patch that is
>
> (a) tested
> (b) explains it
> (c) obvious
>
> and I'll happily re-enable modversions.
Not sure whether you guys want to revert or to go forward. If the latter,
my piece handles x86, Arnd's ARM. Powerpc already has the needed bits in
mainline. I've just tried arm64 -- despite same toolchain versions as
failing x86, with CONFIG_MODVERSIONS=y it loaded a bunch of modules fine so
it appears no arch bits are needed there. My uneducated guess is that most
other architectures should be fine without special handling, too. It'd be
nice if someone with an actual clue could confirm.
Meow!
--
The bill declaring Jesus as the King of Poland fails to specify whether
the addition is at the top or end of the list of kings. What should the
historians do?
* Adam Borowski <[email protected]> wrote:
> Here's some history:
> The day of -rc1, multiple people immediately reported the breakage; it was
> quickly found out that reverting 784d5699eddc fixes it. A "going forward"
> patch has been posted but was insufficient; when the real devs went to bed
> the last message was
> https://www.mail-archive.com/[email protected]/msg1250370.html
> which ends with instructions and "Care to do a patch for x86?".
>
> Then a random person (me) did the legwork, gathered affected symbols, wrote
> and tested the x86 patch. It was then tested by multiple people; Arnd
> Bergmann wrote the ARM equivalent. Whenever a new lkml thread reporting the
> breakage popped up, we pointed people to the patches and everyone was happy.
> As for upstreaming, there was a delay because Michal Marek was on vacation.
>
> Michal returned and sent you the pull request, you merged it as 04e36857 on
> Nov 18. For some reason the per-arch pieces were excluded; I was instructed
> to send my part to x86 maintainers.
>
> I did so; the patch later got a better description by Nick and a bunch of
> Tested-by -- but alas, nary a comment or action from x86 guys, despite
> pings/resends (last one: https://lkml.org/lkml/2016/11/23/706). I guess I'm
> lacking the secret handshake or something -- thus, it looks like it's my
> fault, the rest of you can be blamed mostly for letting a
> not-a-real-kernel-dev unsupervised.
My part of the story is easy to explain: the reason I skipped the 11/23 patch was
because it was tagged 'kbuild' and because the commit that broke it was never
acked by (or was upstreamed via) the x86 maintainers - we never upstreamed any
modversions changes in the past AFAIR - so I assumed it would be handled via
whatever path got the breakage upstream (turns out it was via the VFS tree?),
or via the kbuild tree.
> On Nov 24 finally Ingo responded, the discussion ended with you marking
> modversions as BROKEN.
Yeah, that was when my internal timer ran out: modversions breakage was reported
against -rc1 already and it still wasn't working (a seemingly working kernel build
resulted in an unbootable system) - due to the timeline and confusion you
explained.
I totally agree with marking it BROKEN: it was the simplest, most robust way to
fix it and nobody seemed to be owning the modversions feature.
Thanks,
Ingo
Commit 4efca4ed ("kbuild: modversions for EXPORT_SYMBOL() for asm") adds
modversion support for symbols exported from asm files. Architectures
must include C-style declarations for those symbols in asm/asm-prototypes.h
in order for them to be versioned.
Add these declarations for x86, and an architecture-independent file that
can be used for common symbols.
User impact: kernels may fail to load modules at all when
CONFIG_MODVERSIONS=y.
Signed-off-by: Adam Borowski <[email protected]>
Tested-by: Kalle Valo <[email protected]>
Acked-by: Nicholas Piggin <[email protected]>
Tested-by: Peter Wu <[email protected]>
Tested-by: Oliver Hartkopp <[email protected]>
---
> So somebody send me a minimal patch that is
>
> (a) tested
By many people.
> (b) explains it
The actual logic is in 4efca4ed0. It wants C prototypes defined in
asm/asm-prototypes.h that lists symbols defined in assembly -- genksyms
knows only how to read C code.
> (c) obvious
To be honest I don't quite understand what's the real gain over code that
was removed by 784d5699eddc, this mostly brings the symbols back. But
that's for people wiser than me to explain.
> and I'll happily re-enable modversions.
The powerpc counterpart to this patch is in mainline as 9e5f688, although a
file by that name already existed.
As for arm, it looks like it was handled the other way by 8478132.
diff --git a/arch/x86/include/asm/asm-prototypes.h b/arch/x86/include/asm/asm-prototypes.h
new file mode 100644
index 0000000..ae87224
--- /dev/null
+++ b/arch/x86/include/asm/asm-prototypes.h
@@ -0,0 +1,12 @@
+#include <asm/ftrace.h>
+#include <asm/uaccess.h>
+#include <asm/string.h>
+#include <asm/page.h>
+#include <asm/checksum.h>
+
+#include <asm-generic/asm-prototypes.h>
+
+#include <asm/page.h>
+#include <asm/pgtable.h>
+#include <asm/special_insns.h>
+#include <asm/preempt.h>
diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
new file mode 100644
index 0000000..df13637
--- /dev/null
+++ b/include/asm-generic/asm-prototypes.h
@@ -0,0 +1,7 @@
+#include <linux/bitops.h>
+extern void *__memset(void *, int, __kernel_size_t);
+extern void *__memcpy(void *, const void *, __kernel_size_t);
+extern void *__memmove(void *, const void *, __kernel_size_t);
+extern void *memset(void *, int, __kernel_size_t);
+extern void *memcpy(void *, const void *, __kernel_size_t);
+extern void *memmove(void *, const void *, __kernel_size_t);
--
2.10.2
On Tue, Nov 29, 2016 at 02:29:54PM +0100, Ingo Molnar wrote:
> * Adam Borowski <[email protected]> wrote:
>
> > Here's some history:
> > The day of -rc1, multiple people immediately reported the breakage; it was
> > quickly found out that reverting 784d5699eddc fixes it. A "going forward"
> > patch has been posted but was insufficient; when the real devs went to bed
[...]
> > For some reason the per-arch pieces were excluded; I was instructed
> > to send my part to x86 maintainers.
> >
> > I did so; the patch later got a better description by Nick and a bunch of
> > Tested-by -- but alas, nary a comment or action from x86 guys, despite
> > pings/resends (last one: https://lkml.org/lkml/2016/11/23/706). I guess I'm
> > lacking the secret handshake or something -- thus, it looks like it's my
> > fault, the rest of you can be blamed mostly for letting a
> > not-a-real-kernel-dev unsupervised.
>
> My part of the story is easy to explain: the reason I skipped the 11/23 patch was
> because it was tagged 'kbuild' and because the commit that broke it was never
> acked by (or was upstreamed via) the x86 maintainers - we never upstreamed any
> modversions changes in the past AFAIR - so I assumed it would be handled via
> whatever path got the breakage upstream (turns out it was via the VFS tree?),
> or via the kbuild tree.
The problematic merge was 84d6984 (it brought in 22823ab4^..590abbdd).
Interesting commits have "$ARCH: move exports to definitions" in their
subjects, they indeed did not go through arch trees.
> > On Nov 24 finally Ingo responded, the discussion ended with you marking
> > modversions as BROKEN.
>
> Yeah, that was when my internal timer ran out: modversions breakage was reported
> against -rc1 already and it still wasn't working (a seemingly working kernel build
> resulted in an unbootable system) - due to the timeline and confusion you
> explained.
>
> I totally agree with marking it BROKEN: it was the simplest, most robust way to
> fix it and nobody seemed to be owning the modversions feature.
Per Ben Hutchings' objection, it doesn't look like BROKEN is an option at
least for 4.9 -- even if mainline leaves it as is, distro maintainers would
need to do the work themselves.
Architectures that look like they could be affected:
x86 alpha m68k s390 arm ppc sparc ia64
(this list might be incomplete!).
Of these, ppc and arm are already fixed, x86 is in this thread,
arm64 is not on the list which explains why it works for me. No idea about
the rest.
Meow!
--
The bill declaring Jesus as the King of Poland fails to specify whether
the addition is at the top or end of the list of kings. What should the
historians do?
Dne 29.11.2016 v 16:27 Linus Torvalds napsal(a):
> On Nov 29, 2016 5:51 AM, "Adam Borowski" <[email protected]
> <mailto:[email protected]>> wrote:
>>
>
>> >
>> > (a) tested
>>
>> By many people.
>
> No.
>
> I've tested the build *without* this, and it works fine.
>
>> > (b) explains it
>>
>> The actual logic is in 4efca4ed0. It wants C prototypes defined in
>> asm/asm-prototypes.h that lists symbols defined in assembly -- genksyms
>> knows only how to read C code.
>
> See above. I'm not taking more random patches that "fix" this when it's
> not broken for me. Not without very explicit explanations of why that
> patch is still needed for others.
The original and easily observable bug is that were are not generating
symbol checksums for the asm-exported symbols, so they default to 0.
This can be seen e.g. in the Module.symvers file. This seemed like a
minor issue, because with the functions written in asm, the type
checking is rather weak (this has been the case even before Al's
patches). However, there is another bug that with _some_ toolchains /
architectures, the checksums do not default to 0, but they are simply
missing in the ___kcrctab* sections and the module loader complains. We
can of course research into the details of the second bug, but we
already know that we are not generating the checksums while we should be.
Michal
On Tue, Nov 29, 2016 at 8:03 AM, Michal Marek <[email protected]> wrote:
>
> The original and easily observable bug is that were are not generating
> symbol checksums for the asm-exported symbols, so they default to 0.
> This can be seen e.g. in the Module.symvers file. This seemed like a
> minor issue, because with the functions written in asm, the type
> checking is rather weak (this has been the case even before Al's
> patches). However, there is another bug that with _some_ toolchains /
> architectures, the checksums do not default to 0, but they are simply
> missing in the ___kcrctab* sections and the module loader complains. We
> can of course research into the details of the second bug, but we
> already know that we are not generating the checksums while we should be.
So let's just say that "toolchain is buggy" and make a missing kcrctab
entry mean zero (or mean "matches anything"). And just shut up the
warning.
I do *not* want to add random bandaids for something like a broken
toolchain issue when I'd really rather just delete the feature.
Linus
On Tue, Nov 29, 2016 at 9:05 AM, Adam Borowski <[email protected]> wrote:
>
> Thus, if it's indeed binutils, you'll see the breakage as soon as Fedora
> recovers from the freeze.
So quite frankly, I don't want to make our kernel sources worse due to
broken shit tools getting something wrong that we shouldn't even care
about.
How about this stupid patch? It weakens modversions, but that may be
ok for Debian, and a better alternative than just saying "we don't
support it at all".
Linus
On Tue, Nov 29, 2016 at 9:10 AM, Linus Torvalds
<[email protected]> wrote:
>
> So quite frankly, I don't want to make our kernel sources worse due to
> broken shit tools getting something wrong that we shouldn't even care
> about.
And yes, I'm on binutils 2.26 (with no issues), so it could be that
it's 2.27 that triggers this.
We could make the pr_warn_once() mention "broken binutils?" so that
people know why the warning happens.
Linus
On Tue, Nov 29, 2016 at 07:27:12AM -0800, Linus Torvalds wrote:
> On Nov 29, 2016 5:51 AM, "Adam Borowski" <[email protected]> wrote:
> > >
> > > (a) tested
> >
> > By many people.
>
> No.
>
> I've tested the build *without* this, and it works fine.
Michal mentioned "why", let's try "where".
I have no idea what setup is required to trigger the problem, but here's a
simple sufficient one:
Current Debian unstable, amd64.
git reset --hard v4.9-rc7
git revert cd3caefb
make defconfig
CONFIG_MODVERSIONS=y
(in my case) CONFIG_BTRFS_FS=y (so I can boot)
enable a module for testing, I used CONFIG_EXT4_FS=m
make bindeb-pkg
dpkg -i linux-image_XXXXX.deb
modprobe ext4
[ 63.779490] jbd2: no symbol version for memcpy
[ 63.779492] jbd2: Unknown symbol memcpy (err -22)
[ 63.779550] jbd2: no symbol version for phys_base
[ 63.779551] jbd2: Unknown symbol phys_base (err -22)
[ 63.779561] jbd2: no symbol version for memset
[ 63.779562] jbd2: Unknown symbol memset (err -22)
Not sure which piece of toolchain matters here, someone said binutils.
In that case, Fedora ships 2.26.1-1.fc25, they were frozen so couldn't
update. Debian is at 2.27.51.20161127-1, Gentoo at 2.27, same for Arch,
OpenSUSE; Ubuntu at 2.27.51.20161124-1.
Thus, if it's indeed binutils, you'll see the breakage as soon as Fedora
recovers from the freeze.
Meow!
--
The bill declaring Jesus as the King of Poland fails to specify whether
the addition is at the top or end of the list of kings. What should the
historians do?
On Tue, 2016-11-29 at 08:17 -0800, Linus Torvalds wrote:
> > On Tue, Nov 29, 2016 at 8:03 AM, Michal Marek <[email protected]> wrote:
> >
> > The original and easily observable bug is that were are not generating
> > symbol checksums for the asm-exported symbols, so they default to 0.
> > This can be seen e.g. in the Module.symvers file. This seemed like a
> > minor issue, because with the functions written in asm, the type
> > checking is rather weak (this has been the case even before Al's
> > patches). However, there is another bug that with _some_ toolchains /
> > architectures, the checksums do not default to 0, but they are simply
> > missing in the ___kcrctab* sections and the module loader complains. We
> > can of course research into the details of the second bug, but we
> > already know that we are not generating the checksums while we should be.
>
> So let's just say that "toolchain is buggy" and make a missing kcrctab
> entry mean zero (or mean "matches anything"). And just shut up the
> warning.
>
> I do *not* want to add random bandaids for something like a broken
> toolchain issue when I'd really rather just delete the feature.
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):
diff --git a/init/Kconfig b/init/Kconfig
index c4fbc1e55c25..34407f15e6d3 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1945,7 +1945,6 @@ config MODULE_FORCE_UNLOAD
config MODVERSIONS
bool "Module versioning support"
- depends on BROKEN
help
Usually, you have to use modules compiled with your kernel.
Saying Y here makes it sometimes possible to use modules
diff --git a/kernel/module.c b/kernel/module.c
index f57dd63186e6..78d61ae50bc5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -296,6 +296,12 @@ int unregister_module_notifier(struct notifier_block *nb)
}
EXPORT_SYMBOL(unregister_module_notifier);
+enum {
+ MAGIC_NO_MATCH,
+ MAGIC_MATCH_NEED_CRC,
+ MAGIC_MATCH_EXACT
+};
+
struct load_info {
Elf_Ehdr *hdr;
unsigned long len;
@@ -305,6 +311,7 @@ struct load_info {
struct _ddebug *debug;
unsigned int num_debug;
bool sig_ok;
+ int magic_match;
#ifdef CONFIG_KALLSYMS
unsigned long mod_kallsyms_init_off;
#endif
@@ -1268,13 +1275,14 @@ static unsigned long maybe_relocated(unsigned long crc,
return crc;
}
-static int check_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static int check_version(const struct load_info *info,
const char *symname,
struct module *mod,
const unsigned long *crc,
const struct module *crc_owner)
{
+ Elf_Shdr *sechdrs = info->sechdrs;
+ unsigned int versindex = info->index.vers;
unsigned int i, num_versions;
struct modversion_info *versions;
@@ -1294,6 +1302,10 @@ static int check_version(Elf_Shdr *sechdrs,
if (strcmp(versions[i].name, symname) != 0)
continue;
+ /* Ignore dummy zero CRC */
+ if (versions[i].crc == 0)
+ break;
+
if (versions[i].crc == maybe_relocated(*crc, crc_owner))
return 1;
pr_debug("Found checksum %lX vs module %lX\n",
@@ -1301,6 +1313,9 @@ static int check_version(Elf_Shdr *sechdrs,
goto bad_version;
}
+ if (info->magic_match == MAGIC_MATCH_EXACT)
+ return 1;
+
pr_warn("%s: no symbol version for %s\n", mod->name, symname);
return 0;
@@ -1310,8 +1325,7 @@ static int check_version(Elf_Shdr *sechdrs,
return 0;
}
-static inline int check_modstruct_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static inline int check_modstruct_version(const struct load_info *info,
struct module *mod)
{
const unsigned long *crc;
@@ -1327,24 +1341,24 @@ static inline int check_modstruct_version(Elf_Shdr *sechdrs,
BUG();
}
preempt_enable();
- return check_version(sechdrs, versindex,
+ return check_version(info,
VMLINUX_SYMBOL_STR(module_layout), mod, crc,
NULL);
}
-/* First part is kernel version, which we ignore if module has crcs. */
-static inline int same_magic(const char *amagic, const char *bmagic,
- bool has_crcs)
+/* First part is kernel version, which can be ignored if module has crcs. */
+static inline int compare_magic(const char *amagic, const char *bmagic)
{
- if (has_crcs) {
- amagic += strcspn(amagic, " ");
- bmagic += strcspn(bmagic, " ");
- }
- return strcmp(amagic, bmagic) == 0;
+ if (strcmp(amagic, bmagic) == 0)
+ return MAGIC_MATCH_EXACT;
+
+ amagic += strcspn(amagic, " ");
+ bmagic += strcspn(bmagic, " ");
+ return strcmp(amagic, bmagic) == 0 ? MAGIC_MATCH_NEED_CRC : MAGIC_NO_MATCH;
}
+
#else
-static inline int check_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static inline int check_version(const struct load_info *info,
const char *symname,
struct module *mod,
const unsigned long *crc,
@@ -1353,17 +1367,15 @@ static inline int check_version(Elf_Shdr *sechdrs,
return 1;
}
-static inline int check_modstruct_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static inline int check_modstruct_version(const struct load_info *info,
struct module *mod)
{
return 1;
}
-static inline int same_magic(const char *amagic, const char *bmagic,
- bool has_crcs)
+static inline int compare_magic(const char *amagic, const char *bmagic)
{
- return strcmp(amagic, bmagic) == 0;
+ return strcmp(amagic, bmagic) == 0 ? MAGIC_MATCH_EXACT : MAGIC_NO_MATCH;
}
#endif /* CONFIG_MODVERSIONS */
@@ -1390,8 +1402,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
if (!sym)
goto unlock;
- if (!check_version(info->sechdrs, info->index.vers, name, mod, crc,
- owner)) {
+ if (!check_version(info, name, mod, crc, owner)) {
sym = ERR_PTR(-EINVAL);
goto getname;
}
@@ -2936,7 +2947,7 @@ static struct module *setup_load_info(struct load_info *info, int flags)
info->index.pcpu = find_pcpusec(info);
/* Check module struct version now, before we try to use module. */
- if (!check_modstruct_version(info->sechdrs, info->index.vers, mod))
+ if (!check_modstruct_version(info, mod))
return ERR_PTR(-ENOEXEC);
return mod;
@@ -2952,13 +2963,19 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
/* This is allowed: modprobe --force will invalidate it. */
if (!modmagic) {
+ info->magic_match = MAGIC_NO_MATCH;
err = try_to_force_load(mod, "bad vermagic");
if (err)
return err;
- } else if (!same_magic(modmagic, vermagic, info->index.vers)) {
- pr_err("%s: version magic '%s' should be '%s'\n",
- mod->name, modmagic, vermagic);
- return -ENOEXEC;
+ } else {
+ info->magic_match = compare_magic(modmagic, vermagic);
+ if (info->magic_match == MAGIC_NO_MATCH ||
+ (info->magic_match == MAGIC_MATCH_NEED_CRC &&
+ !info->index.vers)) {
+ pr_err("%s: version magic '%s' should be '%s'\n",
+ mod->name, modmagic, vermagic);
+ return -ENOEXEC;
+ }
}
if (!get_modinfo(info, "intree")) {
--- END ---
Ben.
--
Ben Hutchings
Theory and practice are closer in theory than in practice.
????????????????????????????????- John Levine, moderator of comp.compilers
On Tue, Nov 29, 2016 at 11:57 AM, Ben Hutchings <[email protected]> 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.
Linus
Dne 29.11.2016 v 18:10 Linus Torvalds napsal(a):
> How about this stupid patch? It weakens modversions, but that may be
> ok for Debian, and a better alternative than just saying "we don't
> support it at all".
[...]
> - pr_warn("%s: no symbol version for %s\n", mod->name, symname);
> - return 0;
> + /* Broken toolchain. Warn once, then let it go.. */
> + pr_warn_once("%s: no symbol version for %s\n", mod->name, symname);
> + return 1;
Fine with me, if it goes with the revert of the "depends on BROKEN."
Michal
On Tue, 29 Nov 2016 12:35:57 -0800
Linus Torvalds <[email protected]> wrote:
> On Tue, Nov 29, 2016 at 11:57 AM, Ben Hutchings <[email protected]> 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
building external modules.
-NOTE: "modules_prepare" will not build Module.symvers even if
-CONFIG_MODVERSIONS is set; therefore, a full kernel build needs to be
-executed to make module versioning work.
-
--- 2.1 Command Syntax
The command to build an external module is:
diff --git a/Makefile b/Makefile
index 694111b..2066419 100644
--- a/Makefile
+++ b/Makefile
@@ -316,13 +316,6 @@ KBUILD_MODULES :=
KBUILD_BUILTIN := 1
# If we have only "make modules", don't compile built-in objects.
-# When we're building modules with modversions, we need to consider
-# the built-in objects during the descend as well, in order to
-# make sure the checksums are up to date before we record them.
-
-ifeq ($(MAKECMDGOALS),modules)
- KBUILD_BUILTIN := $(if $(CONFIG_MODVERSIONS),1)
-endif
# If we have "make <whatever> modules", compile modules
# in addition to whatever we do anyway.
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index 06ff7fd..253f6ca 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -33,10 +33,6 @@ u64 module_emit_plt_entry(struct module *mod, const Elf64_Rela *rela,
Elf64_Sym *sym);
#ifdef CONFIG_RANDOMIZE_BASE
-#ifdef CONFIG_MODVERSIONS
-#define ARCH_RELOCATES_KCRCTAB
-#define reloc_start (kimage_vaddr - KIMAGE_VADDR)
-#endif
extern u64 module_alloc_base;
#else
#define module_alloc_base ((u64)_etext - MODULES_VSIZE)
diff --git a/arch/avr32/boot/images/Makefile b/arch/avr32/boot/images/Makefile
index 2a3b539..0878bef 100644
--- a/arch/avr32/boot/images/Makefile
+++ b/arch/avr32/boot/images/Makefile
@@ -37,8 +37,6 @@ OBJCOPYFLAGS_vmlinux.elf := --change-section-lma .text-0x80000000 \
--change-section-lma __param-0x80000000 \
--change-section-lma __ksymtab-0x80000000 \
--change-section-lma __ksymtab_gpl-0x80000000 \
- --change-section-lma __kcrctab-0x80000000 \
- --change-section-lma __kcrctab_gpl-0x80000000 \
--change-section-lma __ksymtab_strings-0x80000000 \
--set-start 0xa0000000
$(obj)/vmlinux.elf: vmlinux FORCE
diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index cd4ffd8..94a7f7a 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -94,9 +94,5 @@ struct exception_table_entry;
void sort_ex_table(struct exception_table_entry *start,
struct exception_table_entry *finish);
-#if defined(CONFIG_MODVERSIONS) && defined(CONFIG_PPC64)
-#define ARCH_RELOCATES_KCRCTAB
-#define reloc_start PHYSICAL_START
-#endif
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_MODULE_H */
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 183368e..3e59856 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -277,30 +277,11 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
return relocs * sizeof(struct ppc64_stub_entry);
}
-/* Still needed for ELFv2, for .TOC. */
-static void dedotify_versions(struct modversion_info *vers,
- unsigned long size)
-{
- struct modversion_info *end;
-
- for (end = (void *)vers + size; vers < end; vers++)
- if (vers->name[0] == '.') {
- memmove(vers->name, vers->name+1, strlen(vers->name));
-#ifdef ARCH_RELOCATES_KCRCTAB
- /* The TOC symbol has no CRC computed. To avoid CRC
- * check failing, we must force it to the expected
- * value (see CRC check in module.c).
- */
- if (!strcmp(vers->name, "TOC."))
- vers->crc = -(unsigned long)reloc_start;
-#endif
- }
-}
-
/*
* Undefined symbols which refer to .funcname, hack to funcname. Make .TOC.
* seem to be defined (value set later).
*/
+/* XXX: remove for ELFv2? */
static void dedotify(Elf64_Sym *syms, unsigned int numsyms, char *strtab)
{
unsigned int i;
@@ -349,9 +330,6 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
me->arch.stubs_section = i;
else if (strcmp(secstrings + sechdrs[i].sh_name, ".toc") == 0)
me->arch.toc_section = i;
- else if (strcmp(secstrings+sechdrs[i].sh_name,"__versions")==0)
- dedotify_versions((void *)hdr + sechdrs[i].sh_offset,
- sechdrs[i].sh_size);
/* We don't handle .init for the moment: rename to _init */
while ((p = strstr(secstrings + sechdrs[i].sh_name, ".init")))
diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c
index 0c2fae8..4c15dc0 100644
--- a/arch/x86/tools/relocs.c
+++ b/arch/x86/tools/relocs.c
@@ -59,7 +59,6 @@ static const char * const sym_regex_kernel[S_NSYMTYPES] = {
"__(start|end)_pci_.*|"
"__(start|end)_builtin_fw|"
"__(start|stop)___ksymtab(|_gpl|_unused|_unused_gpl|_gpl_future)|"
- "__(start|stop)___kcrctab(|_gpl|_unused|_unused_gpl|_gpl_future)|"
"__(start|stop)___param|"
"__(start|stop)___modver|"
"__(start|stop)___bug_table|"
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 5673fff..89c3ece 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -30,11 +30,6 @@
* TODO: Figure out how to use SMB alerts. This will require a new
* interface into the I2C driver, I believe.
*/
-
-#if defined(MODVERSIONS)
-#include <linux/modversions.h>
-#endif
-
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/sched.h>
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 5e23e2d..0d9d84e 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -60,7 +60,7 @@ CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
extra-$(CONFIG_EFI_ARMSTUB) := $(lib-y)
lib-$(CONFIG_EFI_ARMSTUB) := $(patsubst %.o,%.stub.o,$(lib-y))
-STUBCOPY_FLAGS-y := -R .debug* -R *ksymtab* -R *kcrctab*
+STUBCOPY_FLAGS-y := -R .debug* -R *ksymtab*
STUBCOPY_FLAGS-$(CONFIG_ARM64) += --prefix-alloc-sections=.init \
--prefix-symbols=__efistub_
STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
@@ -80,5 +80,5 @@ quiet_cmd_stubcopy = STUBCPY $@
# explicitly by the decompressor linker script.
#
STUBCOPY_FLAGS-$(CONFIG_ARM) += --rename-section .data=.data.efistub \
- -R ___ksymtab+sort -R ___kcrctab+sort
+ -R ___ksymtab+sort
STUBCOPY_RELOC-$(CONFIG_ARM) := R_ARM_ABS
diff --git a/drivers/message/fusion/mptlan.h b/drivers/message/fusion/mptlan.h
index 69e9d54..f976a5b 100644
--- a/drivers/message/fusion/mptlan.h
+++ b/drivers/message/fusion/mptlan.h
@@ -51,10 +51,7 @@
#define LINUX_MPTLAN_H_INCLUDED
/*****************************************************************************/
-#if !defined(__GENKSYMS__)
#include <linux/module.h>
-#endif
-
#include <linux/netdevice.h>
#include <linux/errno.h>
// #include <linux/etherdevice.h>
diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
index 63554e9..cb0c6cf 100644
--- a/include/asm-generic/export.h
+++ b/include/asm-generic/export.h
@@ -48,14 +48,6 @@ KSYM(__kstrtab_\name):
.asciz "\name"
#endif
.previous
-#ifdef CONFIG_MODVERSIONS
- .section ___kcrctab\sec+\name,"a"
- .balign KCRC_ALIGN
-KSYM(__kcrctab_\name):
- __put KSYM(__crc_\name)
- .weak KSYM(__crc_\name)
- .previous
-#endif
#endif
.endm
#undef __put
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 6d5d35f..8b52b57 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -360,41 +360,6 @@
VMLINUX_SYMBOL(__stop___ksymtab_gpl_future) = .; \
} \
\
- /* Kernel symbol table: Normal symbols */ \
- __kcrctab : AT(ADDR(__kcrctab) - LOAD_OFFSET) { \
- VMLINUX_SYMBOL(__start___kcrctab) = .; \
- KEEP(*(SORT(___kcrctab+*))) \
- VMLINUX_SYMBOL(__stop___kcrctab) = .; \
- } \
- \
- /* Kernel symbol table: GPL-only symbols */ \
- __kcrctab_gpl : AT(ADDR(__kcrctab_gpl) - LOAD_OFFSET) { \
- VMLINUX_SYMBOL(__start___kcrctab_gpl) = .; \
- KEEP(*(SORT(___kcrctab_gpl+*))) \
- VMLINUX_SYMBOL(__stop___kcrctab_gpl) = .; \
- } \
- \
- /* Kernel symbol table: Normal unused symbols */ \
- __kcrctab_unused : AT(ADDR(__kcrctab_unused) - LOAD_OFFSET) { \
- VMLINUX_SYMBOL(__start___kcrctab_unused) = .; \
- KEEP(*(SORT(___kcrctab_unused+*))) \
- VMLINUX_SYMBOL(__stop___kcrctab_unused) = .; \
- } \
- \
- /* Kernel symbol table: GPL-only unused symbols */ \
- __kcrctab_unused_gpl : AT(ADDR(__kcrctab_unused_gpl) - LOAD_OFFSET) { \
- VMLINUX_SYMBOL(__start___kcrctab_unused_gpl) = .; \
- KEEP(*(SORT(___kcrctab_unused_gpl+*))) \
- VMLINUX_SYMBOL(__stop___kcrctab_unused_gpl) = .; \
- } \
- \
- /* Kernel symbol table: GPL-future-only symbols */ \
- __kcrctab_gpl_future : AT(ADDR(__kcrctab_gpl_future) - LOAD_OFFSET) { \
- VMLINUX_SYMBOL(__start___kcrctab_gpl_future) = .; \
- KEEP(*(SORT(___kcrctab_gpl_future+*))) \
- VMLINUX_SYMBOL(__stop___kcrctab_gpl_future) = .; \
- } \
- \
/* Kernel symbol table: strings */ \
__ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) { \
KEEP(*(__ksymtab_strings)) \
diff --git a/include/linux/export.h b/include/linux/export.h
index 2a0f61f..ba359cf 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -39,24 +39,11 @@ extern struct module __this_module;
#ifdef CONFIG_MODULES
-#if defined(__KERNEL__) && !defined(__GENKSYMS__)
-#ifdef CONFIG_MODVERSIONS
-/* Mark the CRC weak since genksyms apparently decides not to
- * generate a checksums for some symbols */
-#define __CRC_SYMBOL(sym, sec) \
- extern __visible void *__crc_##sym __attribute__((weak)); \
- static const unsigned long __kcrctab_##sym \
- __used \
- __attribute__((section("___kcrctab" sec "+" #sym), used)) \
- = (unsigned long) &__crc_##sym;
-#else
-#define __CRC_SYMBOL(sym, sec)
-#endif
+#if defined(__KERNEL__)
/* For every exported symbol, place a struct in the __ksymtab section */
#define ___EXPORT_SYMBOL(sym, sec) \
extern typeof(sym) sym; \
- __CRC_SYMBOL(sym, sec) \
static const char __kstrtab_##sym[] \
__attribute__((section("__ksymtab_strings"), aligned(1))) \
= VMLINUX_SYMBOL_STR(sym); \
@@ -110,7 +97,7 @@ extern struct module __this_module;
#define EXPORT_UNUSED_SYMBOL_GPL(sym)
#endif
-#endif /* __GENKSYMS__ */
+#endif /* __KERNEL__ */
#else /* !CONFIG_MODULES... */
diff --git a/include/linux/module.h b/include/linux/module.h
index 0c3207d..ee121f2 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -32,11 +32,6 @@
#define MODULE_NAME_LEN MAX_PARAM_PREFIX_LEN
-struct modversion_info {
- unsigned long crc;
- char name[MODULE_NAME_LEN];
-};
-
struct module;
struct exception_table_entry;
@@ -346,7 +341,6 @@ struct module {
/* Exported symbols */
const struct kernel_symbol *syms;
- const unsigned long *crcs;
unsigned int num_syms;
/* Kernel parameters. */
@@ -359,18 +353,15 @@ struct module {
/* GPL-only exported symbols. */
unsigned int num_gpl_syms;
const struct kernel_symbol *gpl_syms;
- const unsigned long *gpl_crcs;
#ifdef CONFIG_UNUSED_SYMBOLS
/* unused exported symbols. */
const struct kernel_symbol *unused_syms;
- const unsigned long *unused_crcs;
unsigned int num_unused_syms;
/* GPL-only, unused exported symbols. */
unsigned int num_unused_gpl_syms;
const struct kernel_symbol *unused_gpl_syms;
- const unsigned long *unused_gpl_crcs;
#endif
#ifdef CONFIG_MODULE_SIG
@@ -382,7 +373,6 @@ struct module {
/* symbols that will be GPL-only in the near future. */
const struct kernel_symbol *gpl_future_syms;
- const unsigned long *gpl_future_crcs;
unsigned int num_gpl_future_syms;
/* Exception table */
@@ -523,7 +513,6 @@ struct module *find_module(const char *name);
struct symsearch {
const struct kernel_symbol *start, *stop;
- const unsigned long *crcs;
enum {
NOT_GPL_ONLY,
GPL_ONLY,
@@ -539,7 +528,6 @@ struct symsearch {
*/
const struct kernel_symbol *find_symbol(const char *name,
struct module **owner,
- const unsigned long **crc,
bool gplok,
bool warn);
diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h
index 6f8fbcf..fafeea0 100644
--- a/include/linux/vermagic.h
+++ b/include/linux/vermagic.h
@@ -16,18 +16,16 @@
#else
#define MODULE_VERMAGIC_MODULE_UNLOAD ""
#endif
-#ifdef CONFIG_MODVERSIONS
-#define MODULE_VERMAGIC_MODVERSIONS "modversions "
-#else
-#define MODULE_VERMAGIC_MODVERSIONS ""
-#endif
#ifndef MODULE_ARCH_VERMAGIC
#define MODULE_ARCH_VERMAGIC ""
#endif
+#ifdef CONFIG_MODULE_ABI_EXPLICIT
+#define VERMAGIC_STRING CONFIG_MODULE_ABI_EXPLICIT_STRING
+#else
#define VERMAGIC_STRING \
UTS_RELEASE " " \
MODULE_VERMAGIC_SMP MODULE_VERMAGIC_PREEMPT \
- MODULE_VERMAGIC_MODULE_UNLOAD MODULE_VERMAGIC_MODVERSIONS \
+ MODULE_VERMAGIC_MODULE_UNLOAD \
MODULE_ARCH_VERMAGIC
-
+#endif
diff --git a/init/Kconfig b/init/Kconfig
index 34407f1..7a1b6ac 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1943,15 +1943,25 @@ config MODULE_FORCE_UNLOAD
rmmod). This is mainly for kernel developers and desperate users.
If unsure, say N.
-config MODVERSIONS
- bool "Module versioning support"
- help
- Usually, you have to use modules compiled with your kernel.
- Saying Y here makes it sometimes possible to use modules
- compiled for different kernels, by adding enough information
- to the modules to (hopefully) spot any changes which would
- make them incompatible with the kernel you are running. If
- unsure, say N.
+config MODULE_ABI_EXPLICIT
+ bool "Provide explicit module ABI version string" if EXPERT
+ default n
+ help
+ This option is mostly for distro maintainers.
+
+ When this option is NOT set, the kernel/module ABI version string
+ is based on the kernel version (among other things), which means
+ modules are not compatible when there is any change to kernel
+ version. This is what most people want.
+
+ Enabling this option allows you to provide an explicit module
+ ABI version string. This allows modules to be usable between kernel
+ version bumps.
+
+ If unusure, say N.
+
+config MODULE_ABI_EXPLICIT_STRING
+ string "Explicit module ABI version string" if MODULE_ABI_EXPLICIT
config MODULE_SRCVERSION_ALL
bool "Source checksum for all modules"
diff --git a/kernel/module.c b/kernel/module.c
index 0e54d5b..ff68854 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -309,7 +309,7 @@ struct load_info {
unsigned long mod_kallsyms_init_off;
#endif
struct {
- unsigned int sym, str, mod, vers, info, pcpu;
+ unsigned int sym, str, mod, info, pcpu;
} index;
};
@@ -386,22 +386,11 @@ extern const struct kernel_symbol __start___ksymtab_gpl[];
extern const struct kernel_symbol __stop___ksymtab_gpl[];
extern const struct kernel_symbol __start___ksymtab_gpl_future[];
extern const struct kernel_symbol __stop___ksymtab_gpl_future[];
-extern const unsigned long __start___kcrctab[];
-extern const unsigned long __start___kcrctab_gpl[];
-extern const unsigned long __start___kcrctab_gpl_future[];
#ifdef CONFIG_UNUSED_SYMBOLS
extern const struct kernel_symbol __start___ksymtab_unused[];
extern const struct kernel_symbol __stop___ksymtab_unused[];
extern const struct kernel_symbol __start___ksymtab_unused_gpl[];
extern const struct kernel_symbol __stop___ksymtab_unused_gpl[];
-extern const unsigned long __start___kcrctab_unused[];
-extern const unsigned long __start___kcrctab_unused_gpl[];
-#endif
-
-#ifndef CONFIG_MODVERSIONS
-#define symversion(base, idx) NULL
-#else
-#define symversion(base, idx) ((base != NULL) ? ((base) + (idx)) : NULL)
#endif
static bool each_symbol_in_section(const struct symsearch *arr,
@@ -430,20 +419,16 @@ bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
{
struct module *mod;
static const struct symsearch arr[] = {
- { __start___ksymtab, __stop___ksymtab, __start___kcrctab,
+ { __start___ksymtab, __stop___ksymtab,
NOT_GPL_ONLY, false },
{ __start___ksymtab_gpl, __stop___ksymtab_gpl,
- __start___kcrctab_gpl,
GPL_ONLY, false },
{ __start___ksymtab_gpl_future, __stop___ksymtab_gpl_future,
- __start___kcrctab_gpl_future,
WILL_BE_GPL_ONLY, false },
#ifdef CONFIG_UNUSED_SYMBOLS
{ __start___ksymtab_unused, __stop___ksymtab_unused,
- __start___kcrctab_unused,
NOT_GPL_ONLY, true },
{ __start___ksymtab_unused_gpl, __stop___ksymtab_unused_gpl,
- __start___kcrctab_unused_gpl,
GPL_ONLY, true },
#endif
};
@@ -455,23 +440,19 @@ bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
list_for_each_entry_rcu(mod, &modules, list) {
struct symsearch arr[] = {
- { mod->syms, mod->syms + mod->num_syms, mod->crcs,
+ { mod->syms, mod->syms + mod->num_syms,
NOT_GPL_ONLY, false },
{ mod->gpl_syms, mod->gpl_syms + mod->num_gpl_syms,
- mod->gpl_crcs,
GPL_ONLY, false },
{ mod->gpl_future_syms,
mod->gpl_future_syms + mod->num_gpl_future_syms,
- mod->gpl_future_crcs,
WILL_BE_GPL_ONLY, false },
#ifdef CONFIG_UNUSED_SYMBOLS
{ mod->unused_syms,
mod->unused_syms + mod->num_unused_syms,
- mod->unused_crcs,
NOT_GPL_ONLY, true },
{ mod->unused_gpl_syms,
mod->unused_gpl_syms + mod->num_unused_gpl_syms,
- mod->unused_gpl_crcs,
GPL_ONLY, true },
#endif
};
@@ -494,7 +475,6 @@ struct find_symbol_arg {
/* Output */
struct module *owner;
- const unsigned long *crc;
const struct kernel_symbol *sym;
};
@@ -527,7 +507,6 @@ static bool check_symbol(const struct symsearch *syms,
#endif
fsa->owner = owner;
- fsa->crc = symversion(syms->crcs, symnum);
fsa->sym = &syms->start[symnum];
return true;
}
@@ -556,11 +535,12 @@ static bool find_symbol_in_section(const struct symsearch *syms,
return false;
}
-/* Find a symbol and return it, along with, (optional) crc and
- * (optional) module which owns it. Needs preempt disabled or module_mutex. */
+/*
+ * Find a symbol and return it, along with, and (optional) module which owns
+ * it. Needs preempt disabled or module_mutex.
+ */
const struct kernel_symbol *find_symbol(const char *name,
struct module **owner,
- const unsigned long **crc,
bool gplok,
bool warn)
{
@@ -573,8 +553,6 @@ const struct kernel_symbol *find_symbol(const char *name,
if (each_symbol_section(find_symbol_in_section, &fsa)) {
if (owner)
*owner = fsa.owner;
- if (crc)
- *crc = fsa.crc;
return fsa.sym;
}
@@ -1031,7 +1009,7 @@ void __symbol_put(const char *symbol)
struct module *owner;
preempt_disable();
- if (!find_symbol(symbol, &owner, NULL, true, false))
+ if (!find_symbol(symbol, &owner, true, false))
BUG();
module_put(owner);
preempt_enable();
@@ -1256,118 +1234,6 @@ static int try_to_force_load(struct module *mod, const char *reason)
#endif
}
-#ifdef CONFIG_MODVERSIONS
-/* If the arch applies (non-zero) relocations to kernel kcrctab, unapply it. */
-static unsigned long maybe_relocated(unsigned long crc,
- const struct module *crc_owner)
-{
-#ifdef ARCH_RELOCATES_KCRCTAB
- if (crc_owner == NULL)
- return crc - (unsigned long)reloc_start;
-#endif
- return crc;
-}
-
-static int check_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
- const char *symname,
- struct module *mod,
- const unsigned long *crc,
- const struct module *crc_owner)
-{
- unsigned int i, num_versions;
- struct modversion_info *versions;
-
- /* Exporting module didn't supply crcs? OK, we're already tainted. */
- if (!crc)
- return 1;
-
- /* No versions at all? modprobe --force does this. */
- if (versindex == 0)
- return try_to_force_load(mod, symname) == 0;
-
- versions = (void *) sechdrs[versindex].sh_addr;
- num_versions = sechdrs[versindex].sh_size
- / sizeof(struct modversion_info);
-
- for (i = 0; i < num_versions; i++) {
- if (strcmp(versions[i].name, symname) != 0)
- continue;
-
- if (versions[i].crc == maybe_relocated(*crc, crc_owner))
- return 1;
- pr_debug("Found checksum %lX vs module %lX\n",
- maybe_relocated(*crc, crc_owner), versions[i].crc);
- goto bad_version;
- }
-
- /* Broken toolchain. Warn once, then let it go.. */
- pr_warn_once("%s: no symbol version for %s\n", mod->name, symname);
- return 1;
-
-bad_version:
- pr_warn("%s: disagrees about version of symbol %s\n",
- mod->name, symname);
- return 0;
-}
-
-static inline int check_modstruct_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
- struct module *mod)
-{
- const unsigned long *crc;
-
- /*
- * Since this should be found in kernel (which can't be removed), no
- * locking is necessary -- use preempt_disable() to placate lockdep.
- */
- preempt_disable();
- if (!find_symbol(VMLINUX_SYMBOL_STR(module_layout), NULL,
- &crc, true, false)) {
- preempt_enable();
- BUG();
- }
- preempt_enable();
- return check_version(sechdrs, versindex,
- VMLINUX_SYMBOL_STR(module_layout), mod, crc,
- NULL);
-}
-
-/* First part is kernel version, which we ignore if module has crcs. */
-static inline int same_magic(const char *amagic, const char *bmagic,
- bool has_crcs)
-{
- if (has_crcs) {
- amagic += strcspn(amagic, " ");
- bmagic += strcspn(bmagic, " ");
- }
- return strcmp(amagic, bmagic) == 0;
-}
-#else
-static inline int check_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
- const char *symname,
- struct module *mod,
- const unsigned long *crc,
- const struct module *crc_owner)
-{
- return 1;
-}
-
-static inline int check_modstruct_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
- struct module *mod)
-{
- return 1;
-}
-
-static inline int same_magic(const char *amagic, const char *bmagic,
- bool has_crcs)
-{
- return strcmp(amagic, bmagic) == 0;
-}
-#endif /* CONFIG_MODVERSIONS */
-
/* Resolve a symbol for this module. I.e. if we find one, record usage. */
static const struct kernel_symbol *resolve_symbol(struct module *mod,
const struct load_info *info,
@@ -1376,7 +1242,6 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
{
struct module *owner;
const struct kernel_symbol *sym;
- const unsigned long *crc;
int err;
/*
@@ -1386,24 +1251,15 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
*/
sched_annotate_sleep();
mutex_lock(&module_mutex);
- sym = find_symbol(name, &owner, &crc,
- !(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true);
+ sym = find_symbol(name, &owner,
+ !(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true);
if (!sym)
goto unlock;
- if (!check_version(info->sechdrs, info->index.vers, name, mod, crc,
- owner)) {
- sym = ERR_PTR(-EINVAL);
- goto getname;
- }
-
err = ref_module(mod, owner);
- if (err) {
+ if (err)
sym = ERR_PTR(err);
- goto getname;
- }
-getname:
/* We must make copy under the lock if we failed to get ref. */
strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
unlock:
@@ -2149,7 +2005,7 @@ void *__symbol_get(const char *symbol)
const struct kernel_symbol *sym;
preempt_disable();
- sym = find_symbol(symbol, &owner, NULL, true, true);
+ sym = find_symbol(symbol, &owner, true, true);
if (sym && strong_try_module_get(owner))
sym = NULL;
preempt_enable();
@@ -2184,7 +2040,7 @@ static int verify_export_symbols(struct module *mod)
for (i = 0; i < ARRAY_SIZE(arr); i++) {
for (s = arr[i].sym; s < arr[i].sym + arr[i].num; s++) {
- if (find_symbol(s->name, &owner, NULL, true, false)) {
+ if (find_symbol(s->name, &owner, true, false)) {
pr_err("%s: exports duplicate symbol %s"
" (owned by %s)\n",
mod->name, s->name, module_name(owner));
@@ -2876,14 +2732,9 @@ static int rewrite_section_headers(struct load_info *info, int flags)
#endif
}
- /* Track but don't keep modinfo and version sections. */
- if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
- info->index.vers = 0; /* Pretend no __versions section! */
- else
- info->index.vers = find_sec(info, "__versions");
info->index.info = find_sec(info, ".modinfo");
info->sechdrs[info->index.info].sh_flags &= ~(unsigned long)SHF_ALLOC;
- info->sechdrs[info->index.vers].sh_flags &= ~(unsigned long)SHF_ALLOC;
+
return 0;
}
@@ -2936,10 +2787,6 @@ static struct module *setup_load_info(struct load_info *info, int flags)
info->index.pcpu = find_pcpusec(info);
- /* Check module struct version now, before we try to use module. */
- if (!check_modstruct_version(info->sechdrs, info->index.vers, mod))
- return ERR_PTR(-ENOEXEC);
-
return mod;
}
@@ -2956,7 +2803,7 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
err = try_to_force_load(mod, "bad vermagic");
if (err)
return err;
- } else if (!same_magic(modmagic, vermagic, info->index.vers)) {
+ } else if (strcmp(modmagic, vermagic)) {
pr_err("%s: version magic '%s' should be '%s'\n",
mod->name, modmagic, vermagic);
return -ENOEXEC;
@@ -2991,26 +2838,21 @@ static int find_module_sections(struct module *mod, struct load_info *info)
sizeof(*mod->kp), &mod->num_kp);
mod->syms = section_objs(info, "__ksymtab",
sizeof(*mod->syms), &mod->num_syms);
- mod->crcs = section_addr(info, "__kcrctab");
mod->gpl_syms = section_objs(info, "__ksymtab_gpl",
sizeof(*mod->gpl_syms),
&mod->num_gpl_syms);
- mod->gpl_crcs = section_addr(info, "__kcrctab_gpl");
mod->gpl_future_syms = section_objs(info,
"__ksymtab_gpl_future",
sizeof(*mod->gpl_future_syms),
&mod->num_gpl_future_syms);
- mod->gpl_future_crcs = section_addr(info, "__kcrctab_gpl_future");
#ifdef CONFIG_UNUSED_SYMBOLS
mod->unused_syms = section_objs(info, "__ksymtab_unused",
sizeof(*mod->unused_syms),
&mod->num_unused_syms);
- mod->unused_crcs = section_addr(info, "__kcrctab_unused");
mod->unused_gpl_syms = section_objs(info, "__ksymtab_unused_gpl",
sizeof(*mod->unused_gpl_syms),
&mod->num_unused_gpl_syms);
- mod->unused_gpl_crcs = section_addr(info, "__kcrctab_unused_gpl");
#endif
#ifdef CONFIG_CONSTRUCTORS
mod->ctors = section_objs(info, ".ctors",
@@ -3159,19 +3001,6 @@ static int check_module_license_and_versions(struct module *mod)
if (!prev_taint && test_taint(TAINT_PROPRIETARY_MODULE))
pr_warn("%s: module license taints kernel.\n", mod->name);
-#ifdef CONFIG_MODVERSIONS
- if ((mod->num_syms && !mod->crcs)
- || (mod->num_gpl_syms && !mod->gpl_crcs)
- || (mod->num_gpl_future_syms && !mod->gpl_future_crcs)
-#ifdef CONFIG_UNUSED_SYMBOLS
- || (mod->num_unused_syms && !mod->unused_crcs)
- || (mod->num_unused_gpl_syms && !mod->unused_gpl_crcs)
-#endif
- ) {
- return try_to_force_load(mod,
- "no versions for exported symbols");
- }
-#endif
return 0;
}
@@ -4273,15 +4102,3 @@ void print_modules(void)
pr_cont("\n");
}
-#ifdef CONFIG_MODVERSIONS
-/* Generate the signature for all relevant module structures here.
- * If these change, we don't want to try to parse the module. */
-void module_layout(struct module *mod,
- struct modversion_info *ver,
- struct kernel_param *kp,
- struct kernel_symbol *ks,
- struct tracepoint * const *tp)
-{
-}
-EXPORT_SYMBOL(module_layout);
-#endif
diff --git a/scripts/Makefile b/scripts/Makefile
index 1d80897..675a266 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -40,7 +40,6 @@ build_docproc: $(obj)/docproc
build_check-lc_ctype: $(obj)/check-lc_ctype
@:
-subdir-$(CONFIG_MODVERSIONS) += genksyms
subdir-y += mod
subdir-$(CONFIG_SECURITY_SELINUX) += selinux
subdir-$(CONFIG_DTC) += dtc
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 7675d11..5aa0fea 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -159,60 +159,12 @@ cmd_cpp_i_c = $(CPP) $(c_flags) -o $@ $<
$(obj)/%.i: $(src)/%.c FORCE
$(call if_changed_dep,cpp_i_c)
-# These mirror gensymtypes_S and co below, keep them in synch.
-cmd_gensymtypes_c = \
- $(CPP) -D__GENKSYMS__ $(c_flags) $< | \
- $(GENKSYMS) $(if $(1), -T $(2)) \
- $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX)) \
- $(if $(KBUILD_PRESERVE),-p) \
- -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
-
-quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@
-cmd_cc_symtypes_c = \
- set -e; \
- $(call cmd_gensymtypes_c,true,$@) >/dev/null; \
- test -s $@ || rm -f $@
-
-$(obj)/%.symtypes : $(src)/%.c FORCE
- $(call cmd,cc_symtypes_c)
-
# C (.c) files
# The C file is compiled and updated dependency information is generated.
# (See cmd_cc_o_c + relevant part of rule_cc_o_c)
-
quiet_cmd_cc_o_c = CC $(quiet_modtag) $@
-
-ifndef CONFIG_MODVERSIONS
cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $<
-else
-# When module versioning is enabled the following steps are executed:
-# o compile a .tmp_<file>.o from <file>.c
-# o if .tmp_<file>.o doesn't contain a __ksymtab version, i.e. does
-# not export symbols, we just rename .tmp_<file>.o to <file>.o and
-# are done.
-# o otherwise, we calculate symbol versions using the good old
-# genksyms on the preprocessed source and postprocess them in a way
-# that they are usable as a linker script
-# o generate <file>.o from .tmp_<file>.o using the linker to
-# replace the unresolved symbols __crc_exported_symbol with
-# the actual value of the checksum generated by genksyms
-
-cmd_cc_o_c = $(CC) $(c_flags) -c -o $(@D)/.tmp_$(@F) $<
-
-cmd_modversions_c = \
- if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then \
- $(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
- > $(@D)/.tmp_$(@F:.o=.ver); \
- \
- $(LD) $(LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) \
- -T $(@D)/.tmp_$(@F:.o=.ver); \
- rm -f $(@D)/.tmp_$(@F) $(@D)/.tmp_$(@F:.o=.ver); \
- else \
- mv -f $(@D)/.tmp_$(@F) $@; \
- fi;
-endif
-
ifdef CONFIG_FTRACE_MCOUNT_RECORD
ifdef BUILD_C_RECORDMCOUNT
ifeq ("$(origin RECORDMCOUNT_WARN)", "command line")
@@ -270,14 +222,12 @@ endif # CONFIG_STACK_VALIDATION
define rule_cc_o_c
$(call echo-cmd,checksrc) $(cmd_checksrc) \
$(call cmd_and_fixdep,cc_o_c) \
- $(cmd_modversions_c) \
$(cmd_objtool) \
$(call echo-cmd,record_mcount) $(cmd_record_mcount)
endef
define rule_as_o_S
$(call cmd_and_fixdep,as_o_S) \
- $(cmd_modversions_S) \
$(cmd_objtool)
endef
@@ -317,39 +267,6 @@ modkern_aflags := $(KBUILD_AFLAGS_KERNEL) $(AFLAGS_KERNEL)
$(real-objs-m) : modkern_aflags := $(KBUILD_AFLAGS_MODULE) $(AFLAGS_MODULE)
$(real-objs-m:.o=.s): modkern_aflags := $(KBUILD_AFLAGS_MODULE) $(AFLAGS_MODULE)
-# .S file exports must have their C prototypes defined in asm/asm-prototypes.h
-# or a file that it includes, in order to get versioned symbols. We build a
-# dummy C file that includes asm-prototypes and the EXPORT_SYMBOL lines from
-# the .S file (with trailing ';'), and run genksyms on that, to extract vers.
-#
-# This is convoluted. The .S file must first be preprocessed to run guards and
-# expand names, then the resulting exports must be constructed into plain
-# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed
-# to make the genksyms input.
-#
-# These mirror gensymtypes_c and co above, keep them in synch.
-cmd_gensymtypes_S = \
- (echo "\#include <linux/kernel.h>" ; \
- echo "\#include <asm/asm-prototypes.h>" ; \
- $(CPP) $(a_flags) $< | \
- grep "\<___EXPORT_SYMBOL\>" | \
- sed 's/.*___EXPORT_SYMBOL[[:space:]]*\([a-zA-Z0-9_]*\)[[:space:]]*,.*/EXPORT_SYMBOL(\1);/' ) | \
- $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | \
- $(GENKSYMS) $(if $(1), -T $(2)) \
- $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX)) \
- $(if $(KBUILD_PRESERVE),-p) \
- -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
-
-quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
-cmd_cc_symtypes_S = \
- set -e; \
- $(call cmd_gensymtypes_S,true,$@) >/dev/null; \
- test -s $@ || rm -f $@
-
-$(obj)/%.symtypes : $(src)/%.S FORCE
- $(call cmd,cc_symtypes_S)
-
-
quiet_cmd_cpp_s_S = CPP $(quiet_modtag) $@
cmd_cpp_s_S = $(CPP) $(a_flags) -o $@ $<
@@ -357,38 +274,8 @@ $(obj)/%.s: $(src)/%.S FORCE
$(call if_changed_dep,cpp_s_S)
quiet_cmd_as_o_S = AS $(quiet_modtag) $@
-
-ifndef CONFIG_MODVERSIONS
cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
-else
-
-ASM_PROTOTYPES := $(wildcard $(srctree)/arch/$(SRCARCH)/include/asm/asm-prototypes.h)
-
-ifeq ($(ASM_PROTOTYPES),)
-cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
-
-else
-
-# versioning matches the C process described above, with difference that
-# we parse asm-prototypes.h C header to get function definitions.
-
-cmd_as_o_S = $(CC) $(a_flags) -c -o $(@D)/.tmp_$(@F) $<
-
-cmd_modversions_S = \
- if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then \
- $(call cmd_gensymtypes_S,$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
- > $(@D)/.tmp_$(@F:.o=.ver); \
- \
- $(LD) $(LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) \
- -T $(@D)/.tmp_$(@F:.o=.ver); \
- rm -f $(@D)/.tmp_$(@F) $(@D)/.tmp_$(@F:.o=.ver); \
- else \
- mv -f $(@D)/.tmp_$(@F) $@; \
- fi;
-endif
-endif
-
$(obj)/%.o: $(src)/%.S $(objtool_obj) FORCE
$(call if_changed_rule,as_o_S)
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 0a07f90..68018a6 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -89,8 +89,7 @@ multi-objs-m := $(addprefix $(obj)/,$(multi-objs-m))
subdir-ym := $(addprefix $(obj)/,$(subdir-ym))
obj-dirs := $(addprefix $(obj)/,$(obj-dirs))
-# These flags are needed for modversions and compiling, so we define them here
-# already
+# These flags are needed for compiling, so we define them here already
# $(modname_flags) #defines KBUILD_MODNAME as the name of the module it will
# end up in (or would, if it gets compiled in)
# Note: Files that end up in two or more modules are compiled without the
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 16923ba..eb3d3c6 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -1,5 +1,5 @@
# ===========================================================================
-# Module versions
+# Module final link
# ===========================================================================
#
# Stage one of module building created the following:
diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
index 8dc1918..944c0bf 100755
--- a/scripts/adjust_autoksyms.sh
+++ b/scripts/adjust_autoksyms.sh
@@ -67,11 +67,6 @@ while read sym; do
echo "#define __KSYM_${sym} 1"
done >> "$new_ksyms_file"
-# Special case for modversions (see modpost.c)
-if [ -n "$CONFIG_MODVERSIONS" ]; then
- echo "#define __KSYM_module_layout 1" >> "$new_ksyms_file"
-fi
-
# Extract changes between old and new list and touch corresponding
# dependency files.
changed=$(
diff --git a/scripts/export_report.pl b/scripts/export_report.pl
index 8f79b70..255ef81 100755
--- a/scripts/export_report.pl
+++ b/scripts/export_report.pl
@@ -102,8 +102,6 @@ close($module_symvers);
#
# collect the usage count of each symbol.
#
-my $modversion_warnings = 0;
-
foreach my $thismod (@allcfiles) {
my $module;
@@ -112,30 +110,15 @@ foreach my $thismod (@allcfiles) {
next;
}
- my $state=0;
while ( <$module> ) {
chomp;
- if ($state == 0) {
- $state = 1 if ($_ =~ /static const struct modversion_info/);
- next;
- }
- if ($state == 1) {
- $state = 2 if ($_ =~ /__attribute__\(\(section\("__versions"\)\)\)/);
+ if ( $_ !~ /0x[0-9a-f]+,/ ) {
next;
}
- if ($state == 2) {
- if ( $_ !~ /0x[0-9a-f]+,/ ) {
- next;
- }
- my $sym = (split /([,"])/,)[4];
- my ($module, $value, $symbol, $gpl) = @{$SYMBOL{$sym}};
- $SYMBOL{ $sym } = [ $module, $value+1, $symbol, $gpl];
- push(@{$MODULE{$thismod}} , $sym);
- }
- }
- if ($state != 2) {
- warn "WARNING:$thismod is not built with CONFIG_MODVERSIONS enabled\n";
- $modversion_warnings++;
+ my $sym = (split /([,"])/,)[4];
+ my ($module, $value, $symbol, $gpl) = @{$SYMBOL{$sym}};
+ $SYMBOL{ $sym } = [ $module, $value+1, $symbol, $gpl];
+ push(@{$MODULE{$thismod}} , $sym);
}
close($module);
}
@@ -169,9 +152,6 @@ printf("SECTION 2:\n\tThis section reports export-symbol-usage of in-kernel
modules. Each module lists the modules, and the symbols from that module that
it uses. Each listed symbol reports the number of modules using it\n");
-print "\nNOTE: Got $modversion_warnings CONFIG_MODVERSIONS warnings\n\n"
- if $modversion_warnings;
-
print "~"x80 , "\n";
for my $thismod (sort keys %MODULE) {
my $list = $MODULE{$thismod};
diff --git a/scripts/mksysmap b/scripts/mksysmap
index a35acc0..7226ade 100755
--- a/scripts/mksysmap
+++ b/scripts/mksysmap
@@ -37,8 +37,6 @@
# readprofile starts reading symbols when _stext is found, and
# continue until it finds a symbol which is not either of 'T', 't',
-# 'W' or 'w'. __crc_ are 'A' and placed in the middle
-# so we just ignore them to let readprofile continue to work.
-# (At least sparc64 has __crc_ in the middle).
+# 'W' or 'w'.
-$NM -n $1 | grep -v '\( [aNUw] \)\|\(__crc_\)\|\( \$[adt]\)\|\( .L\)' > $2
+$NM -n $1 | grep -v '\( [aNUw] \)\|\( \$[adt]\)\|\( .L\)' > $2
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index bd83497..c929794 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -23,8 +23,6 @@
#include "../../include/linux/license.h"
#include "../../include/linux/export.h"
-/* Are we using CONFIG_MODVERSIONS? */
-static int modversions = 0;
/* Warn about undefined symbols? (do so if we have vmlinux) */
static int have_vmlinux = 0;
/* Is CONFIG_MODULE_SRCVERSION_ALL set? */
@@ -159,13 +157,11 @@ static struct module *new_module(const char *modname)
struct symbol {
struct symbol *next;
struct module *module;
- unsigned int crc;
- int crc_valid;
unsigned int weak:1;
unsigned int vmlinux:1; /* 1 if symbol is defined in vmlinux */
unsigned int kernel:1; /* 1 if symbol is from kernel
* (only for external modules) **/
- unsigned int preloaded:1; /* 1 if symbol from Module.symvers, or crc */
+ unsigned int preloaded:1; /* 1 if symbol from Module.symvers */
enum export export; /* Type of export */
char name[0];
};
@@ -328,20 +324,6 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
return s;
}
-static void sym_update_crc(const char *name, struct module *mod,
- unsigned int crc, enum export export)
-{
- struct symbol *s = find_symbol(name);
-
- if (!s) {
- s = new_symbol(name, mod, export);
- /* Don't complain when we find it later. */
- s->preloaded = 1;
- }
- s->crc = crc;
- s->crc_valid = 1;
-}
-
void *grab_file(const char *filename, unsigned long *size)
{
struct stat st;
@@ -601,13 +583,11 @@ static int ignore_undef_symbol(struct elf_info *info, const char *symname)
return 0;
}
-#define CRC_PFX VMLINUX_SYMBOL_STR(__crc_)
#define KSYMTAB_PFX VMLINUX_SYMBOL_STR(__ksymtab_)
static void handle_modversions(struct module *mod, struct elf_info *info,
Elf_Sym *sym, const char *symname)
{
- unsigned int crc;
enum export export;
if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
@@ -616,13 +596,6 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
else
export = export_from_sec(info, get_secindex(info, sym));
- /* CRC'd symbol */
- if (strncmp(symname, CRC_PFX, strlen(CRC_PFX)) == 0) {
- crc = (unsigned int) sym->st_value;
- sym_update_crc(symname + strlen(CRC_PFX), mod, crc,
- export);
- }
-
switch (sym->st_shndx) {
case SHN_COMMON:
if (!strncmp(symname, "__gnu_lto_", sizeof("__gnu_lto_")-1)) {
@@ -1979,13 +1952,6 @@ static void read_symbols(char *modname)
sizeof(mod->srcversion)-1);
parse_elf_finish(&info);
-
- /* Our trick to get versioning for module struct etc. - it's
- * never passed as an argument to an exported function, so
- * the automatic versioning doesn't pick it up, but it's really
- * important anyhow */
- if (modversions)
- mod->unres = alloc_symbol("module_layout", 0, mod->unres);
}
static void read_symbols_from_files(const char *filename)
@@ -2137,70 +2103,6 @@ static void add_staging_flag(struct buffer *b, const char *name)
buf_printf(b, "\nMODULE_INFO(staging, \"Y\");\n");
}
-/* In kernel, this size is defined in linux/module.h;
- * here we use Elf_Addr instead of long for covering cross-compile
- */
-#define MODULE_NAME_LEN (64 - sizeof(Elf_Addr))
-
-/**
- * Record CRCs for unresolved symbols
- **/
-static int add_versions(struct buffer *b, struct module *mod)
-{
- struct symbol *s, *exp;
- int err = 0;
-
- for (s = mod->unres; s; s = s->next) {
- exp = find_symbol(s->name);
- if (!exp || exp->module == mod) {
- if (have_vmlinux && !s->weak) {
- if (warn_unresolved) {
- warn("\"%s\" [%s.ko] undefined!\n",
- s->name, mod->name);
- } else {
- merror("\"%s\" [%s.ko] undefined!\n",
- s->name, mod->name);
- err = 1;
- }
- }
- continue;
- }
- s->module = exp->module;
- s->crc_valid = exp->crc_valid;
- s->crc = exp->crc;
- }
-
- if (!modversions)
- return err;
-
- buf_printf(b, "\n");
- buf_printf(b, "static const struct modversion_info ____versions[]\n");
- buf_printf(b, "__used\n");
- buf_printf(b, "__attribute__((section(\"__versions\"))) = {\n");
-
- for (s = mod->unres; s; s = s->next) {
- if (!s->module)
- continue;
- if (!s->crc_valid) {
- warn("\"%s\" [%s.ko] has no CRC!\n",
- s->name, mod->name);
- continue;
- }
- if (strlen(s->name) >= MODULE_NAME_LEN) {
- merror("too long symbol \"%s\" [%s.ko]\n",
- s->name, mod->name);
- err = 1;
- break;
- }
- buf_printf(b, "\t{ %#8x, __VMLINUX_SYMBOL_STR(%s) },\n",
- s->crc, s->name);
- }
-
- buf_printf(b, "};\n");
-
- return err;
-}
-
static void add_depends(struct buffer *b, struct module *mod,
struct module *modules)
{
@@ -2290,7 +2192,7 @@ static void write_if_changed(struct buffer *b, const char *fname)
}
/* parse Module.symvers file. line format:
- * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something]
+ * symbol<tab>module[[<tab>export]<tab>something]
**/
static void read_dump(const char *fname, unsigned int kernel)
{
@@ -2303,14 +2205,11 @@ static void read_dump(const char *fname, unsigned int kernel)
return;
while ((line = get_next_line(&pos, file, size))) {
- char *symname, *modname, *d, *export, *end;
- unsigned int crc;
+ char *symname, *modname, *export, *end;
struct module *mod;
struct symbol *s;
- if (!(symname = strchr(line, '\t')))
- goto fail;
- *symname++ = '\0';
+ symname = line;
if (!(modname = strchr(symname, '\t')))
goto fail;
*modname++ = '\0';
@@ -2318,8 +2217,7 @@ static void read_dump(const char *fname, unsigned int kernel)
*export++ = '\0';
if (export && ((end = strchr(export, '\t')) != NULL))
*end = '\0';
- crc = strtoul(line, &d, 16);
- if (*symname == '\0' || *modname == '\0' || *d != '\0')
+ if (*symname == '\0' || *modname == '\0')
goto fail;
mod = find_module(modname);
if (!mod) {
@@ -2331,7 +2229,6 @@ static void read_dump(const char *fname, unsigned int kernel)
s = sym_add_exported(symname, mod, export_no(export));
s->kernel = kernel;
s->preloaded = 1;
- sym_update_crc(symname, mod, crc, export_no(export));
}
release_file(file, size);
return;
@@ -2363,9 +2260,8 @@ static void write_dump(const char *fname)
symbol = symbolhash[n];
while (symbol) {
if (dump_sym(symbol))
- buf_printf(&buf, "0x%08x\t%s\t%s\t%s\n",
- symbol->crc, symbol->name,
- symbol->module->name,
+ buf_printf(&buf, "%s\t%s\t%s\n",
+ symbol->name, symbol->module->name,
export_str(symbol->export));
symbol = symbol->next;
}
@@ -2406,9 +2302,6 @@ int main(int argc, char **argv)
extsym_iter->file = optarg;
extsym_start = extsym_iter;
break;
- case 'm':
- modversions = 1;
- break;
case 'n':
ignore_missing_files = 1;
break;
@@ -2474,7 +2367,6 @@ int main(int argc, char **argv)
add_header(&buf, mod);
add_intree_flag(&buf, !external_module);
add_staging_flag(&buf, mod->name);
- err |= add_versions(&buf, mod);
add_depends(&buf, mod, modules);
add_moddevtable(&buf, mod);
add_srcversion(&buf, mod);
diff --git a/scripts/module-common.lds b/scripts/module-common.lds
index 73a2c7d..da76f519a 100644
--- a/scripts/module-common.lds
+++ b/scripts/module-common.lds
@@ -11,11 +11,6 @@ SECTIONS {
__ksymtab_unused 0 : { *(SORT(___ksymtab_unused+*)) }
__ksymtab_unused_gpl 0 : { *(SORT(___ksymtab_unused_gpl+*)) }
__ksymtab_gpl_future 0 : { *(SORT(___ksymtab_gpl_future+*)) }
- __kcrctab 0 : { *(SORT(___kcrctab+*)) }
- __kcrctab_gpl 0 : { *(SORT(___kcrctab_gpl+*)) }
- __kcrctab_unused 0 : { *(SORT(___kcrctab_unused+*)) }
- __kcrctab_unused_gpl 0 : { *(SORT(___kcrctab_unused_gpl+*)) }
- __kcrctab_gpl_future 0 : { *(SORT(___kcrctab_gpl_future+*)) }
. = ALIGN(8);
.init_array 0 : { *(SORT(.init_array.*)) *(.init_array) }
diff --git a/scripts/namespace.pl b/scripts/namespace.pl
index 9f3c9d4..e76f858 100755
--- a/scripts/namespace.pl
+++ b/scripts/namespace.pl
@@ -306,14 +306,12 @@ sub do_nm
$name !~ /^__parm_/ &&
$name !~ /^__kstrtab/ &&
$name !~ /^__ksymtab/ &&
- $name !~ /^__kcrctab_/ &&
$name !~ /^__exitcall_/ &&
$name !~ /^__initcall_/ &&
$name !~ /^__kdb_initcall_/ &&
$name !~ /^__kdb_exitcall_/ &&
$name !~ /^__module_/ &&
$name !~ /^__mod_/ &&
- $name !~ /^__crc_/ &&
$name ne '__this_module' &&
$name ne 'kernel_version') {
if (!exists($def{$name})) {
--
2.10.2
On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <[email protected]> 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..
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
On Wed, 2016-11-30 at 10:40 -0800, Linus Torvalds wrote:
> > On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <[email protected]> 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.
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'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.
If this was just about non-free modules I wouldn't care. There are
also many freely licenced out-of-tree modules that for various reasons
don't get submitted or accepted upstream; also backports of new or
updated drivers.
Ben.
--
Ben Hutchings
Never attribute to conspiracy what can adequately be explained by
stupidity.
On Wed, 30 Nov 2016 21:33:01 +0000
Ben Hutchings <[email protected]> wrote:
> On Wed, 2016-11-30 at 10:40 -0800, Linus Torvalds wrote:
> > > On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <[email protected]> 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
On Thu, 2016-12-01 at 12:55 +1100, Nicholas Piggin wrote:
> On Wed, 30 Nov 2016 21:33:01 +0000
> > Ben Hutchings <[email protected]> wrote:
>
> > On Wed, 2016-11-30 at 10:40 -0800, Linus Torvalds wrote:
> > > > On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <[email protected]> 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.
> 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), a structure change can change all exports from foo-core. That
ABI is of no use to out-of-tree drivers so we don't care about keeping
it stable, but we do care about preventing an accidental mismatch.
> > 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?
Since the kernel does the symbol lookup and version matching, I'm not
sure what userland can do about it.
Ben.
--
Ben Hutchings
A free society is one where it is safe to be unpopular. - Adlai
Stevenson
On Thu, 01 Dec 2016 02:35:54 +0000
Ben Hutchings <[email protected]> wrote:
> On Thu, 2016-12-01 at 12:55 +1100, Nicholas Piggin wrote:
> > On Wed, 30 Nov 2016 21:33:01 +0000
> > > Ben Hutchings <[email protected]> wrote:
> >
> > > On Wed, 2016-11-30 at 10:40 -0800, Linus Torvalds wrote:
> > > > > On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <[email protected]> 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), a structure change can change all exports from foo-core. That
> ABI is of no use to out-of-tree drivers so we don't care about keeping
> it stable, but we do care about preventing an accidental mismatch.
I still don't think modversions helps there. And how much burden is it
to change the export function names occasionally? I thought it was *very*
rare that an ABI change was required in distros.
>
> > > 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?
>
> Since the kernel does the symbol lookup and version matching, I'm not
> sure what userland can do about it.
I just didn't realize what you wanted the blacklist for. It sounded like
you wanted to be able to just wholesale prevent a module's symbols from
being exported.
Thanks,
Nick
On Wed, Nov 30, 2016 at 10:40:02AM -0800, Linus Torvalds wrote:
> On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <[email protected]> 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 ([email protected]) (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
On Wed, 30 Nov 2016 23:13:25 -0500
Don Zickus <[email protected]> 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 <[email protected]> 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
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
Regards,
-Stanislav
On Thu, 1 Dec 2016 11:48:09 +0100
Stanislav Kozina <[email protected]> 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
On 12/01/2016 12:09 PM, Nicholas Piggin wrote:
> On Thu, 1 Dec 2016 11:48:09 +0100
> Stanislav Kozina <[email protected]> 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. And
the problem with modversions is that it pulls in just everything which
gets (accidentally?) #included in the source file.
The actual ABI maintenance is a different problem, but there are many
possible approaches, the _v2 suffix being one of them.
Regards,
-Stanislav
On Thu, 1 Dec 2016 12:33:02 +0100
Stanislav Kozina <[email protected]> wrote:
> On 12/01/2016 12:09 PM, Nicholas Piggin wrote:
> > On Thu, 1 Dec 2016 11:48:09 +0100
> > Stanislav Kozina <[email protected]> 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
On Tuesday, November 29, 2016 9:14:46 AM CET Linus Torvalds wrote:
> On Tue, Nov 29, 2016 at 9:10 AM, Linus Torvalds
> <[email protected]> wrote:
> >
> > So quite frankly, I don't want to make our kernel sources worse due to
> > broken shit tools getting something wrong that we shouldn't even care
> > about.
>
> And yes, I'm on binutils 2.26 (with no issues), so it could be that
> it's 2.27 that triggers this.
>
> We could make the pr_warn_once() mention "broken binutils?" so that
> people know why the warning happens.
I've tried to get to the bottom of this, but couldn't find anything
related to the toolchain version. I've tried binutils 2.23, 2.24, 2.26
and 2.27, and also gcc-7.0, gcc-5.4.1 and gcc-4.9.3, but with today's
linux-next, I always get
WARNING: EXPORT symbol "mcount" [arch/x86/entry/built-in.ko] version generation failed, symbol will not be versioned.
WARNING: EXPORT symbol "mcount" [arch/x86/built-in.ko] version generation failed, symbol will not be versioned.
WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, symbol will not be versioned.
WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, symbol will not be versioned.
WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol will not be versioned.
WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, symbol will not be versioned.
WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, symbol will not be versioned.
WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol will not be versioned.
Out of 12 randconfig builds that had CONFIG_MODVERSIONS enabled, all 12
had this problem, though not always with all the symbols.
Arnd
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
On Thu, Dec 01, 2016 at 10:20:39AM -0500, Don Zickus wrote:
>
> - provide the memory allocation (instead of having the driver staticly
> allocate)
> - provide functions to retrieve various internal data (instead of having the
> driver do direct referencing to deep internal elements)
> - cut down on some static inlines (and use accessory functions instead),
> etc.
>
> Those types of changes allow the OOT driver to be more ignorant of kernel
> changes and struct modifications.
All that is counter to what we really want to have: a well integrated
kernel that moves forward together so that we can see and improve the
whole situation. No need to make things worse just to help leeches.
Get your damn drivers upstream ASAP and let's stop this discussion..
Nicholas Piggin <[email protected]> a écrit:
[...]
> On Thu, 1 Dec 2016 11:48:09 +0100
> Stanislav Kozina <[email protected]> wrote:
>
>> On 12/01/2016 05:13 AM, Don Zickus wrote:
>>
>> ...
>>
>> > I think GregKH pointed to one such tool, libabigail? We are working on
>> > others too.
>>
>> I should mention one of the others here:
>> https://github.com/skozina/kabi-dw
[...]
> Now this seems much better for distro ABI checking.
Right, Incidentally, Fedora does distro-wide ABI verfication for
userspace libraries updates in a given stable distribution. You can
look at an example of output of the libabigail-based tool that compares
a new version of an ELF library to it's latest stable version:
https://taskotron.fedoraproject.org/artifacts/all/a55cbac8-ab64-11e6-94e0-525400120b80/task_output/curl-7.51.0-2.fc25.log
The results of those ABI comparison can browsed at https://taskotron.fedoraproject.org/resultsdb/results?testcase_name=dist.abicheck.
Of course, you can run the comparison yourself by using a
libabigail-based tool like
https://sourceware.org/libabigail/manual/abipkgdiff.html which takes
.deb and .rpm packages.
We are currently working on making libabigail-based tools understand
Linux kernel specifities so that we can run that kind of analysis on
Kernel updates too. Ideally, when this is done, you should be able to
just use abipkgdiff on two Linux Kernel packages and get the same kind
of output.
> The next question is, do they need any kernel support for rare cases
> where they do have to break the ABI of an export? Simple rename of the
> function with a _v2 postfix might be enough. We could retain some per
> symbol versioning in the kernel if needed, but how much would it
> actually help?
As a reviewer of the ABI change report emitted by the tool, if what you
want is to say "I reviewed that ABI change and it's OK, so please do not
show it to me next time", then you can feed the tool with a "suppression
specification". It's a text file in the INI syntax in which you can
specify the kind of change you want the tool to suppress from its
output: https://sourceware.org/libabigail/manual/libabigail-concepts.html.
So I don't think you need to do anything to the source code of the
Kernel in the cases where you need to change the ABI. Just tell the
tool about that change.
Cheers,
--
Dodji
On Thu, Dec 01, 2016 at 07:26:09AM -0800, Christoph Hellwig wrote:
> On Thu, Dec 01, 2016 at 10:20:39AM -0500, Don Zickus wrote:
> >
> > - provide the memory allocation (instead of having the driver staticly
> > allocate)
> > - provide functions to retrieve various internal data (instead of having the
> > driver do direct referencing to deep internal elements)
> > - cut down on some static inlines (and use accessory functions instead),
> > etc.
> >
> > Those types of changes allow the OOT driver to be more ignorant of kernel
> > changes and struct modifications.
>
> All that is counter to what we really want to have: a well integrated
> kernel that moves forward together so that we can see and improve the
> whole situation. No need to make things worse just to help leeches.
> Get your damn drivers upstream ASAP and let's stop this discussion..
I understand and won't disagree with you. :-)
Unfortunately, there are various drivers that will never go upstream
- paid storage drivers that provide bells and whistles on top of inbox
driver
- old drivers/fs that application has been relying on for a long time but
company doesn't have resources to migrate to current technology.
We have been trying over the years to do what we can to move customers in
the right direction. It is just a slow process, sadly.
Cheers,
Don
On Thu, Dec 01, 2016 at 10:40:59AM -0500, Don Zickus wrote:
> Unfortunately, there are various drivers that will never go upstream
>
> - paid storage drivers that provide bells and whistles on top of inbox
> driver
That's because the developer doesn't want them upstream, that's their
fault, nothing we can do about them.
> - old drivers/fs that application has been relying on for a long time but
> company doesn't have resources to migrate to current technology.
That's what drivers/staging/ is for, I'll take anything that builds (and
sometimes stuff that doesn't build) as long as people are actually using
it. So send the stuff that is in this category on to me and that will
reduce your burden a _lot_.
thanks,
greg k-h
On 2016-12-01 04:39, Nicholas Piggin wrote:
> On Thu, 01 Dec 2016 02:35:54 +0000
> Ben Hutchings <[email protected]> wrote:
>> As I understand it, genksyms incorporates the definitions of a
>> function's parameter and return types - not just their names - and all
>> the types they refer to, recursively. So a structure size change
>> should change the version of all functions where the function and its
>> caller pass that structure between them, however indirectly. It finds
>> such indirect ABI breakage for me fairly regularly, though of course I
>> don't know that it finds everything.
>
> It is only the type name.
>
> Not only that but even if you did extend it further to structure type
> arrangement then you still have to deal with other structures followed
> via pointers. Or (rarer but not unheard of):
>
> - changes to structures without changes of the types of their members
> - changes to arguments without changes of their type
This is already covered by genksyms. Try make V=1 with
CONFIG_MODVERSIONS=y and add the -D option to one of the genksyms
command. I wanted to paste the expanded signature for
register_filesystem() as an example, but vger would probably drop the
mail for being too big :).
> - changes to semantics of functions
> - data structures derived in ways other than exported symbols, e.g.,
> fixed register for `current` on some archs
Right, this is something that genksyms has no idea about.
Michal
On 2016-12-01 05:13, Don Zickus wrote:
> Sorry for chiming in late, but yes RHEL is a big user of MODVERSIONS for our
> kabi protection work. Despite our best intentions we still have lots of
> partners and customers that provide value-add out-of-tree drivers to their
> customers. These module builders requested we have a mechanism to allow
> rolling modules forward for each of our minor RHEL updates without breaking
> their drivers.
FWIW, in SLES we use CONFIG_MODVERSION for pretty much the same reasons
you listed. We also enable it in openSUSE, but there it's not as crucial.
Michal
On 2016-12-01 14:58, Arnd Bergmann wrote:
> On Tuesday, November 29, 2016 9:14:46 AM CET Linus Torvalds wrote:
>> On Tue, Nov 29, 2016 at 9:10 AM, Linus Torvalds
>> <[email protected]> wrote:
>>>
>>> So quite frankly, I don't want to make our kernel sources worse due to
>>> broken shit tools getting something wrong that we shouldn't even care
>>> about.
>>
>> And yes, I'm on binutils 2.26 (with no issues), so it could be that
>> it's 2.27 that triggers this.
>>
>> We could make the pr_warn_once() mention "broken binutils?" so that
>> people know why the warning happens.
>
> I've tried to get to the bottom of this, but couldn't find anything
> related to the toolchain version. I've tried binutils 2.23, 2.24, 2.26
> and 2.27, and also gcc-7.0, gcc-5.4.1 and gcc-4.9.3, but with today's
> linux-next, I always get
>
> WARNING: EXPORT symbol "mcount" [arch/x86/entry/built-in.ko] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [arch/x86/built-in.ko] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol will not be versioned.
>
> Out of 12 randconfig builds that had CONFIG_MODVERSIONS enabled, all 12
> had this problem, though not always with all the symbols.
That we are not generating modversions for the asm exports is a known
problem, independent of the toolchain. The problem with some toolchains
(presumably, because that's the next thing to blame if the source and
the .config is identical) is that the CRCs do not appear as 0 in the
___kcrctab/___kcrctab_gpl section.
Michal
On Thu, Dec 1, 2016 at 5:58 AM, Arnd Bergmann <[email protected]> wrote:
>
> WARNING: EXPORT symbol "mcount" [arch/x86/entry/built-in.ko] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [arch/x86/built-in.ko] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, symbol will not be versioned.
> WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol will not be versioned.
>
> Out of 12 randconfig builds that had CONFIG_MODVERSIONS enabled, all 12
> had this problem, though not always with all the symbols.
Well, the good news is that pretty fundamentally, if it's just the asm
symbls, those really don't have ABI's that change (or if they change,
it's such a fundamental change that everything else will likely have
changed too and we don't need to worry about one asm symbol crc - the
change will be caught by other symbols).
So I think the whole "we don't really care" approach should work fine.
The "let's make every symbol always be versioned" may just be too much
pain for no real gain.
Linus
On Thu, Dec 01, 2016 at 05:06:11PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Dec 01, 2016 at 10:40:59AM -0500, Don Zickus wrote:
> > Unfortunately, there are various drivers that will never go upstream
> >
> > - paid storage drivers that provide bells and whistles on top of inbox
> > driver
>
> That's because the developer doesn't want them upstream, that's their
> fault, nothing we can do about them.
>
> > - old drivers/fs that application has been relying on for a long time but
> > company doesn't have resources to migrate to current technology.
>
> That's what drivers/staging/ is for, I'll take anything that builds (and
> sometimes stuff that doesn't build) as long as people are actually using
> it. So send the stuff that is in this category on to me and that will
> reduce your burden a _lot_.
Hi Greg,
I will forward this offer to the right folks and see who we can get to bite.
:-) Thanks!
Cheers,
Don
On Thursday, December 1, 2016 10:26:07 AM CET Linus Torvalds wrote:
> On Thu, Dec 1, 2016 at 5:58 AM, Arnd Bergmann <[email protected]> wrote:
> >
> > WARNING: EXPORT symbol "mcount" [arch/x86/entry/built-in.ko] version generation failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "mcount" [arch/x86/built-in.ko] version generation failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "cmpxchg8b_emu" [vmlinux] version generation failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "empty_zero_page" [vmlinux] version generation failed, symbol will not be versioned.
> > WARNING: EXPORT symbol "mcount" [vmlinux] version generation failed, symbol will not be versioned.
> >
> > Out of 12 randconfig builds that had CONFIG_MODVERSIONS enabled, all 12
> > had this problem, though not always with all the symbols.
>
> Well, the good news is that pretty fundamentally, if it's just the asm
> symbls, those really don't have ABI's that change (or if they change,
> it's such a fundamental change that everything else will likely have
> changed too and we don't need to worry about one asm symbol crc - the
> change will be caught by other symbols).
Yes, it's always been just the assembly symbols that broke, these were
the ones that Al's original patch changed and that ended up with
no version information.
> So I think the whole "we don't really care" approach should work fine.
> The "let's make every symbol always be versioned" may just be too much
> pain for no real gain.
I have managed to bisect the link failure to a specific binutils
commit by Alan Modra now:
d983c8c ("Strip undefined symbols from .symtab")
went into binutils-2_26 and was reverted in
a82e3ef ("Revert "Strip undefined symbols from .symtab"")
after the release branch for 2.26 was started, so that version
ended up being fine. However, the 2.27 version never saw the revert
and causes loadable kernel modules to become unusable when they refer
to a weak symbol in vmlinux. This works with 2.26 and lower:
$ nm obj-arm/vmlinux | grep __gnu_mcount_nc
w __crc___gnu_mcount_nc
c031ed58 T __gnu_mcount_nc
c0ef4590 r __kcrctab___gnu_mcount_nc
c0efb4f5 r __kstrtab___gnu_mcount_nc
c0ee68bc R __ksymtab___gnu_mcount_nc
and this is breaks with 2.27 and higher:
$ make modules 2>&1 | tail -n 1
WARNING: "__gnu_mcount_nc" [drivers/ata/ahci_platform.ko] has no CRC!
$ nm obj-arm/vmlinux | grep __gnu_mcount_nc
c031ed58 T __gnu_mcount_nc
c0ef4590 r __kcrctab___gnu_mcount_nc
c0efb4f5 r __kstrtab___gnu_mcount_nc
c0ee68bc R __ksymtab___gnu_mcount_nc
Applying the patch below on top of today's binutils master branch
makes it work again. The real patch will also have to fix the testsuite.
Arnd
---
commit 9a48071533f311ff1f567361874fab141bd77230
Author: Arnd Bergmann <[email protected]>
Date: Fri Dec 2 11:31:34 2016 +0100
Revert "Strip undefined symbols from .symtab"
This reverts commit d983c8c5503d680c6d4955ceb610a9beebc64460.
Signed-off-by: Arnd Bergmann <[email protected]>
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 5f87f87..57c4d42 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -9354,9 +9354,8 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
a regular file, or that we have been told to strip. However, if
h->indx is set to -2, the symbol is used by a reloc and we must
output it. */
- strip = FALSE;
if (h->indx == -2)
- ;
+ strip = FALSE;
else if ((h->def_dynamic
|| h->ref_dynamic
|| h->root.type == bfd_link_hash_new)
@@ -9382,13 +9381,14 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
&& h->root.u.undef.abfd != NULL
&& (h->root.u.undef.abfd->flags & BFD_PLUGIN) != 0)
strip = TRUE;
+ else
+ strip = FALSE;
type = h->type;
/* If we're stripping it, and it's not a dynamic symbol, there's
- nothing else to do. However, if it is a forced local symbol or
- an ifunc symbol we need to give the backend finish_dynamic_symbol
- function a chance to make it dynamic. */
+ nothing else to do unless it is a forced local symbol or a
+ STT_GNU_IFUNC symbol. */
if (strip
&& h->dynindx == -1
&& type != STT_GNU_IFUNC
@@ -9691,18 +9691,9 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
}
}
- /* If the symbol is undefined, and we didn't output it to .dynsym,
- strip it from .symtab too. Obviously we can't do this for
- relocatable output or when needed for --emit-relocs. */
- else if (input_sec == bfd_und_section_ptr
- && h->indx != -2
- && !bfd_link_relocatable (flinfo->info))
- return TRUE;
- /* Also strip others that we couldn't earlier due to dynamic symbol
- processing. */
- if (strip)
- return TRUE;
- if ((input_sec->flags & SEC_EXCLUDE) != 0)
+ /* If we're stripping it, then it was just a dynamic symbol, and
+ there's nothing else to do. */
+ if (strip || (input_sec->flags & SEC_EXCLUDE) != 0)
return TRUE;
/* Output a FILE symbol so that following locals are not associated
@@ -9952,9 +9943,8 @@ elf_link_input_bfd (struct elf_final_link_info *flinfo, bfd *input_bfd)
*ppsection = isec;
- /* Don't output the first, undefined, symbol. In fact, don't
- output any undefined local symbol. */
- if (isec == bfd_und_section_ptr)
+ /* Don't output the first, undefined, symbol. */
+ if (ppsection == flinfo->sections)
continue;
if (ELF_ST_TYPE (isym->st_info) == STT_SECTION)
With binutils-2.16 and before, a weak missing symbol was kept during the
final link, and a missing CRC for an export would lead to that CRC
being treated as zero implicitly. With binutils-2.17, the crc
symbol gets dropped, and any module trying to use it will fail to
load.
This sets the weak CRC symbol to zero explicitly, making it defined
in vmlinux, which in turn lets us load the modules referring to
that CRC.
The comment above the __CRC_SYMBOL macro suggests that this was
always the intention, although it also seems that all symbols
defined in C have a correct CRC these days, and only the exports
that are now done in assembly need this.
Signed-off-by: Arnd Bergmann <[email protected]>
---
Not sure if this is the correct way of doing it, but this seems trivial
enough and lets me build the kernel with missing CRCs with any binutils
version.
diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
index 63554e9..59a3b2f 100644
--- a/include/asm-generic/export.h
+++ b/include/asm-generic/export.h
@@ -54,6 +54,7 @@ KSYM(__kstrtab_\name):
KSYM(__kcrctab_\name):
__put KSYM(__crc_\name)
.weak KSYM(__crc_\name)
+ .set KSYM(__crc_\name), 0
.previous
#endif
#endif
On Fri, Dec 2, 2016 at 1:40 PM, Arnd Bergmann <[email protected]> wrote:
> With binutils-2.16 and before, a weak missing symbol was kept during the
2.26?
> final link, and a missing CRC for an export would lead to that CRC
> being treated as zero implicitly. With binutils-2.17, the crc
2.27?
> symbol gets dropped, and any module trying to use it will fail to
> load.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 01.12.2016 17:12, Michal Marek wrote:
> On 2016-12-01 04:39, Nicholas Piggin wrote:
>> On Thu, 01 Dec 2016 02:35:54 +0000
>> Ben Hutchings <[email protected]> wrote:
>>> As I understand it, genksyms incorporates the definitions of a
>>> function's parameter and return types - not just their names - and all
>>> the types they refer to, recursively. So a structure size change
>>> should change the version of all functions where the function and its
>>> caller pass that structure between them, however indirectly. It finds
>>> such indirect ABI breakage for me fairly regularly, though of course I
>>> don't know that it finds everything.
>>
>> It is only the type name.
>>
>> Not only that but even if you did extend it further to structure type
>> arrangement then you still have to deal with other structures followed
>> via pointers. Or (rarer but not unheard of):
>>
>> - changes to structures without changes of the types of their members
>> - changes to arguments without changes of their type
>
> This is already covered by genksyms. Try make V=1 with
> CONFIG_MODVERSIONS=y and add the -D option to one of the genksyms
> command. I wanted to paste the expanded signature for
> register_filesystem() as an example, but vger would probably drop the
> mail for being too big :).
It is easier to just use e.g. `make net/core/dev.symtypes` and look at
the generated file.
Bye,
Hannes
On Friday, December 2, 2016 1:59:15 PM CET Geert Uytterhoeven wrote:
> On Fri, Dec 2, 2016 at 1:40 PM, Arnd Bergmann <[email protected]> wrote:
> > With binutils-2.16 and before, a weak missing symbol was kept during the
>
> 2.26?
>
> > final link, and a missing CRC for an export would lead to that CRC
> > being treated as zero implicitly. With binutils-2.17, the crc
>
> 2.27?'
Yes, serious version number deficiency on my end. Also 4.9 instead of 3.9
in the subject...
Arnd
On Fri, Dec 02, 2016 at 01:40:27PM +0100, Arnd Bergmann wrote:
> With binutils-2.16 and before, a weak missing symbol was kept during the
> final link, and a missing CRC for an export would lead to that CRC
> being treated as zero implicitly. With binutils-2.17, the crc
> symbol gets dropped, and any module trying to use it will fail to
> load.
>
> This sets the weak CRC symbol to zero explicitly, making it defined
> in vmlinux, which in turn lets us load the modules referring to
> that CRC.
>
> The comment above the __CRC_SYMBOL macro suggests that this was
> always the intention, although it also seems that all symbols
> defined in C have a correct CRC these days, and only the exports
> that are now done in assembly need this.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
Looks good, works for me, and, unlike faaae2a5, doesn't produce a nasty
user-scaring warning in normal operation.
> diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
> index 63554e9..59a3b2f 100644
> --- a/include/asm-generic/export.h
> +++ b/include/asm-generic/export.h
> @@ -54,6 +54,7 @@ KSYM(__kstrtab_\name):
> KSYM(__kcrctab_\name):
> __put KSYM(__crc_\name)
> .weak KSYM(__crc_\name)
> + .set KSYM(__crc_\name), 0
> .previous
> #endif
> #endif
--
The bill declaring Jesus as the King of Poland fails to specify whether
the addition is at the top or end of the list of kings. What should the
historians do?
On Fri, Dec 2, 2016 at 2:55 AM, Arnd Bergmann <[email protected]> wrote:
>
> Yes, it's always been just the assembly symbols that broke, these were
> the ones that Al's original patch changed and that ended up with
> no version information.
Ok, and the reason is because even if we have a weak symbol from C, it
would always have a value.
Good to know what the heck the problem was.
> I have managed to bisect the link failure to a specific binutils
> commit by Alan Modra now:
Thanks. And I committed your "set to zero" patch, so we can hopefully
leave this all behind us.
I marked it for stable (not because older kernels need it, but because
it's the right thing to do and if we ever backport anything that
causes this we don't want to forget this).
And I'll keep the workaround in kernel/module.c just because it's
really late in the rc series, and I'd rather have both belt and
suspenders for this all right now.
Linus
On Fri, 2016-12-02 at 13:40 +0100, Arnd Bergmann wrote:
> With binutils-2.16 and before, a weak missing symbol was kept during the
> final link, and a missing CRC for an export would lead to that CRC
> being treated as zero implicitly. With binutils-2.17, the crc
> symbol gets dropped, and any module trying to use it will fail to
> load.
>
> This sets the weak CRC symbol to zero explicitly, making it defined
> in vmlinux, which in turn lets us load the modules referring to
> that CRC.
>
> The comment above the __CRC_SYMBOL macro suggests that this was
> always the intention, although it also seems that all symbols
> defined in C have a correct CRC these days, and only the exports
> that are now done in assembly need this.
>
> > Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> Not sure if this is the correct way of doing it, but this seems trivial
> enough and lets me build the kernel with missing CRCs with any binutils
> version.
I tried this along with Adam's patch on x86_64, with Debian's binutils
2.27.51.20161127. The result was that the kernel's __kcrctab held 0
for several symbols, even though there was type information in asm-
prototypes.h and Module.symvers and the modules had a non-zero CRC for
those symbols. With just Adam's patch, the kernel and modules agreed.
Ben.
> diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
> index 63554e9..59a3b2f 100644
> --- a/include/asm-generic/export.h
> +++ b/include/asm-generic/export.h
> @@ -54,6 +54,7 @@ KSYM(__kstrtab_\name):
> KSYM(__kcrctab_\name):
> > __put KSYM(__crc_\name)
> > .weak KSYM(__crc_\name)
> > + .set KSYM(__crc_\name), 0
> > .previous
> #endif
> #endif
>
--
Ben Hutchings
Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
On Saturday, December 3, 2016 4:36:37 AM CET Ben Hutchings wrote:
> On Fri, 2016-12-02 at 13:40 +0100, Arnd Bergmann wrote:
> > With binutils-2.16 and before, a weak missing symbol was kept during the
> > final link, and a missing CRC for an export would lead to that CRC
> > being treated as zero implicitly. With binutils-2.17, the crc
> > symbol gets dropped, and any module trying to use it will fail to
> > load.
> >
> > This sets the weak CRC symbol to zero explicitly, making it defined
> > in vmlinux, which in turn lets us load the modules referring to
> > that CRC.
> >
> > The comment above the __CRC_SYMBOL macro suggests that this was
> > always the intention, although it also seems that all symbols
> > defined in C have a correct CRC these days, and only the exports
> > that are now done in assembly need this.
> >
> > > Signed-off-by: Arnd Bergmann <[email protected]>
> > ---
> > Not sure if this is the correct way of doing it, but this seems trivial
> > enough and lets me build the kernel with missing CRCs with any binutils
> > version.
>
> I tried this along with Adam's patch on x86_64, with Debian's binutils
> 2.27.51.20161127. The result was that the kernel's __kcrctab held 0
> for several symbols, even though there was type information in asm-
> prototypes.h and Module.symvers and the modules had a non-zero CRC for
> those symbols. With just Adam's patch, the kernel and modules agreed.
Can you be more specific? Which symbols are those? I would have expected
modpost to generate Module.symvers from the vmlinux file, so I wonder
where that difference comes from.
Arnd
On Fri, Dec 02, 2016 at 11:55:58AM +0100, Arnd Bergmann wrote:
> I have managed to bisect the link failure to a specific binutils
> commit by Alan Modra now:
>
> d983c8c ("Strip undefined symbols from .symtab")
>
> went into binutils-2_26 and was reverted in
>
> a82e3ef ("Revert "Strip undefined symbols from .symtab"")
>
> after the release branch for 2.26 was started, so that version
> ended up being fine. However, the 2.27 version never saw the revert
> and causes loadable kernel modules to become unusable when they refer
> to a weak symbol in vmlinux. This works with 2.26 and lower:
See https://sourceware.org/ml/binutils/2016-01/msg00118.html thread
for discussion on why the patch was reverted for 2.26. At the time I
believed that only the ppc64 kernel was affected, by a weak undefined
"__crc_TOC." disappearing. Am I correct in thinking that remained
true up until Linus' merge commit 84d69848c9 2016-10-14?
As far as reverting the binutils commit goes, I'm quite willing to do
that if necessary. You can see how important I think the fix was by
viewing https://sourceware.org/bugzilla/show_bug.cgi?id=4317 and
noticing that the bug was reported in 2007 and didn't see any action
for 8 years..
--
Alan Modra
Australia Development Lab, IBM
On Sat, Dec 3, 2016 at 11:44 PM, Alan Modra <[email protected]> wrote:
>
> As far as reverting the binutils commit goes, I'm quite willing to do
> that if necessary
I think we have the proper fix in the kernel now, witht he "mark weak
asm symbols with value 0". Or if not "proper", then at least
acceptable. So I think the ship has sailed on the binutils change, and
there's no point in reverting it any more.
Linus
On Thu, 1 Dec 2016 17:12:41 +0100
Michal Marek <[email protected]> wrote:
> On 2016-12-01 04:39, Nicholas Piggin wrote:
> > On Thu, 01 Dec 2016 02:35:54 +0000
> > Ben Hutchings <[email protected]> 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
On Thu, 1 Dec 2016 10:20:39 -0500
Don Zickus <[email protected]> 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
>> The question is how to provide a similar guarantee if a different way?
> As a tool to aid distro reviewers, modversions has some value, but the
> debug info parsing tools that have been mentioned in this thread seem
> superior (not that I've tested them).
On the other hand the big advantage of modversions is that it also
verifies the checksum during runtime (module loading). In other words, I
believe that any other solution should still generate some form of
checksum/watermark which can be easily checked for compatibility on
module load.
It should not be hard to add to the DWARF based tools though. We'd just
parse DWARF data instead of the C code.
Regards,
-Stanislav
On Fri, 9 Dec 2016 08:55:51 +0100
Stanislav Kozina <[email protected]> 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
>>>> The question is how to provide a similar guarantee if a different way?
>>> As a tool to aid distro reviewers, modversions has some value, but the
>>> debug info parsing tools that have been mentioned in this thread seem
>>> superior (not that I've tested them).
>> On the other hand the big advantage of modversions is that it also
>> verifies the checksum during runtime (module loading). In other words, I
>> believe that any other solution should still generate some form of
>> checksum/watermark which can be easily checked for compatibility on
>> module load.
>> It should not be hard to add to the DWARF based tools though. We'd just
>> parse DWARF data instead of the C code.
> A runtime check is still done, with per-module vermagic which distros
> can change when they bump the ABI version. Is it really necessary to
> have more than that (i.e., per-symbol versioning)?
From my point of view, it is. We need to allow changing ABI for some
modules while maintaining it for others.
In fact I think that there should be version not only for every exported
symbol (in the EXPORT_SYMBOL() sense), but also for every public type
(in the sense of eg. structure defined in the public header file).
Thanks,
-Stanislav
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.
On Fri, 9 Dec 2016 15:36:04 +0100
Stanislav Kozina <[email protected]> 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
On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote:
> On Fri, 9 Dec 2016 15:36:04 +0100
> Stanislav Kozina <[email protected]> wrote:
>
> > >>>> The question is how to provide a similar guarantee if a different way?
> > >>> As a tool to aid distro reviewers, modversions has some value, but the
> > >>> debug info parsing tools that have been mentioned in this thread seem
> > >>> superior (not that I've tested them).
> > >> On the other hand the big advantage of modversions is that it also
> > >> verifies the checksum during runtime (module loading). In other words, I
> > >> believe that any other solution should still generate some form of
> > >> checksum/watermark which can be easily checked for compatibility on
> > >> module load.
> > >> It should not be hard to add to the DWARF based tools though. We'd just
> > >> parse DWARF data instead of the C code.
> > > A runtime check is still done, with per-module vermagic which distros
> > > can change when they bump the ABI version. Is it really necessary to
> > > have more than that (i.e., per-symbol versioning)?
> >
> > From my point of view, it is. We need to allow changing ABI for some
> > modules while maintaining it for others.
> > In fact I think that there should be version not only for every exported
> > symbol (in the EXPORT_SYMBOL() sense), but also for every public type
> > (in the sense of eg. structure defined in the public header file).
>
> Well the distro can just append _v2, _v3 to the name of the function
> or type if it has to break compat for some reason. Would that be enough?
There are other ways that distros can work around when upstream "breaks"
the ABI, sometimes they can rename functions, and others they can
"preload" structures with padding in anticipation for when/if fields get
added to them. But that's all up to the distros, no need for us to
worry about that at all :)
thanks,
greg k-h
On Fri, 09 Dec 2016 15:21:33 +0000
Ian Campbell <[email protected]> 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
On Fri, Dec 09, 2016 at 01:50:41PM +1000, Nicholas Piggin wrote:
> >
> > We have plenty of customers with 10 year old drivers, where the expertise
> > has long left the company. The engineers still around, recompile and make
> > tweaks to get things working on the latest RHEL. Verify it passes testing
> > and release it. Then they hope to not touch it again for a few years until
> > the next RHEL comes along.
> >
> > Scary, huh? :-)
>
> Oh yeah my aim here is not to make distro or out of tree module vendors
> life harder, actually the opposite. If it turns out modversions really is
> the best approach, I'm not in a position to complain about its complexity
> because we have Suse and Redhat people maintaining the build and module
> systems :) I just want to see if we can do things better.
Hi Nick,
I think we are in pretty good agreement here. We can do better than
modversions. On the flip side, I would hate to see modversions ripped out
until we have an alternate path forward as it does get us by for now. :-)
Cheers,
Don
Hello,
Nicholas Piggin <[email protected]> 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?
[1]: https://sourceware.org/libabigail/manual/abidiff.html
Okay, the abidiff I used in this message is one that is not yet
released. It's source code is in the dodji/kabidiff branch of the
Git repository at https://sourceware.org/git/?p=libabigail.git;a=summary
Cheers,
--
Dodji
On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote:
> Hello,
>
> Nicholas Piggin <[email protected]> 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
On Sat, 10 Dec 2016 13:41:03 +0100
Greg Kroah-Hartman <[email protected]> wrote:
> On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote:
> > Hello,
> >
> > Nicholas Piggin <[email protected]> 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
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.
>>>> A runtime check is still done, with per-module vermagic which distros
>>>> can change when they bump the ABI version. Is it really necessary to
>>>> have more than that (i.e., per-symbol versioning)?
>>> From my point of view, it is. We need to allow changing ABI for some
>>> modules while maintaining it for others.
>>> In fact I think that there should be version not only for every exported
>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type
>>> (in the sense of eg. structure defined in the public header file).
>> Well the distro can just append _v2, _v3 to the name of the function
>> or type if it has to break compat for some reason. Would that be enough?
> There are other ways that distros can work around when upstream "breaks"
> the ABI, sometimes they can rename functions, and others they can
> "preload" structures with padding in anticipation for when/if fields get
> added to them. But that's all up to the distros, no need for us to
> worry about that at all :)
Currently, the ABI version (checksum) is stored outside of the actual
code in the __ksymtab section. That means that the distributions can
still apply upstream patches cleanly and only update the version
checksum if these break ABI.
With the _v2, _v3 suffixes (or similar solutions) we'd be effectively
storing the ABI versions directly in the code and that would cause
conflicts when pulling further patches from upstream.
My view is that it would be than easier to maintain out-of-tree
modversions (or similar tool) rather than to solve all these conflicts.
Warm Regards,
-Stanislav
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
On Mon, 12 Dec 2016 10:48:47 +0100
Stanislav Kozina <[email protected]> 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
Dne 9.12.2016 v 23:46 Dodji Seketeli napsal(a):
> Hello,
>
> Nicholas Piggin <[email protected]> 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
Michal Marek <[email protected]> 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
On 2016-12-14 09:58, Dodji Seketeli wrote:
> Michal Marek <[email protected]> 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
Michal Marek <[email protected]> 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
On 2016-12-14 10:36, Dodji Seketeli wrote:
> Michal Marek <[email protected]> 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
Dodji Seketeli <[email protected]> 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
Michal Marek <[email protected]> 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
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
On 2016-12-14 11:02, Dodji Seketeli wrote:
> Michal Marek <[email protected]> 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
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 <[email protected]> wrote:
>>
>>>>>>> The question is how to provide a similar guarantee if a different way?
>>>>>> As a tool to aid distro reviewers, modversions has some value, but the
>>>>>> debug info parsing tools that have been mentioned in this thread seem
>>>>>> superior (not that I've tested them).
>>>>> On the other hand the big advantage of modversions is that it also
>>>>> verifies the checksum during runtime (module loading). In other words, I
>>>>> believe that any other solution should still generate some form of
>>>>> checksum/watermark which can be easily checked for compatibility on
>>>>> module load.
>>>>> It should not be hard to add to the DWARF based tools though. We'd just
>>>>> parse DWARF data instead of the C code.
>>>> A runtime check is still done, with per-module vermagic which distros
>>>> can change when they bump the ABI version. Is it really necessary to
>>>> have more than that (i.e., per-symbol versioning)?
>>>
>>> From my point of view, it is. We need to allow changing ABI for some
>>> modules while maintaining it for others.
>>> In fact I think that there should be version not only for every exported
>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type
>>> (in the sense of eg. structure defined in the public header file).
>>
>> Well the distro can just append _v2, _v3 to the name of the function
>> or type if it has to break compat for some reason. Would that be enough?
>
> There are other ways that distros can work around when upstream "breaks"
> the ABI, sometimes they can rename functions, and others they can
> "preload" structures with padding in anticipation for when/if fields get
> added to them. But that's all up to the distros, no need for us to
> worry about that at all :)
The _v2 and _v3 functions are probably the ones that also get used by
future backports in the distro kernel itself and are probably the reason
for the ABI change in the first place. Thus going down this route will
basically require distros to touch every future backport patch and will
in general generate a big mess internally.
I think it is important to keep versioning information outside of the
source code. Some kind of modversions will still be required, but
distros should be able to decide if they put in some kind of checksum or
a string, what suites them most.
Thanks,
Hannes (who is still impressed by the genksyms tools)
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 <[email protected]> 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
On Wed, 14 Dec 2016 15:04:36 +0100
Hannes Frederic Sowa <[email protected]> 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 <[email protected]> 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
On 15.12.2016 03:06, Nicholas Piggin wrote:
> On Wed, 14 Dec 2016 15:04:36 +0100
> Hannes Frederic Sowa <[email protected]> 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 <[email protected]> wrote:
>>>>
>>>>>>>>> The question is how to provide a similar guarantee if a different way?
>>>>>>>> As a tool to aid distro reviewers, modversions has some value, but the
>>>>>>>> debug info parsing tools that have been mentioned in this thread seem
>>>>>>>> superior (not that I've tested them).
>>>>>>> On the other hand the big advantage of modversions is that it also
>>>>>>> verifies the checksum during runtime (module loading). In other words, I
>>>>>>> believe that any other solution should still generate some form of
>>>>>>> checksum/watermark which can be easily checked for compatibility on
>>>>>>> module load.
>>>>>>> It should not be hard to add to the DWARF based tools though. We'd just
>>>>>>> parse DWARF data instead of the C code.
>>>>>> A runtime check is still done, with per-module vermagic which distros
>>>>>> can change when they bump the ABI version. Is it really necessary to
>>>>>> have more than that (i.e., per-symbol versioning)?
>>>>>
>>>>> From my point of view, it is. We need to allow changing ABI for some
>>>>> modules while maintaining it for others.
>>>>> In fact I think that there should be version not only for every exported
>>>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type
>>>>> (in the sense of eg. structure defined in the public header file).
>>>>
>>>> Well the distro can just append _v2, _v3 to the name of the function
>>>> or type if it has to break compat for some reason. Would that be enough?
>>>
>>> There are other ways that distros can work around when upstream "breaks"
>>> the ABI, sometimes they can rename functions, and others they can
>>> "preload" structures with padding in anticipation for when/if fields get
>>> added to them. But that's all up to the distros, no need for us to
>>> worry about that at all :)
>>
>> The _v2 and _v3 functions are probably the ones that also get used by
>> future backports in the distro kernel itself and are probably the reason
>> for the ABI change in the first place. Thus going down this route will
>> basically require distros to touch every future backport patch and will
>> in general generate a big mess internally.
>
> What kind of big mess? You have to check the logic of each backport even
> if it does apply cleanly, so the added overhead of the name change should
> be relatively tiny, no?
Basically single patches are backported in huge series. Reviewing each
single patch also definitely makes sense, a review of the series as a
whole is much more worthwhile because it focuses more on logic.
The patches themselves are checked by individual robots or humans
against merge conflict introduced mistakes which ring alarm bells for
people to look more closely during review.
Merge conflicts introduced mistakes definitely can happen because
developers/backporters lose the focus from the actual logic but deal
with shifting lines around or just fixing up postfixes to function names.
We still try to align the kernel as much as possible with upstream,
because most developers can't really hold the differences between
upstream and the internal functions in their heads (is this function RMW
safe in this version but not that kernel version...).
Anyway, I don't think we will at any time have multiple versions of a
function exported to 3rd party kernel modules. The headaches are just
too big. Basically we would have to version structs and not functions
(this is our bigger problem), thus exporting new versions of functions
don't really help at all. Having multiple versions of structs really
scares me. ;)
We already pad structs to allow for additional struct members to be
added, which helps a lot.
If versioning of function symbols would be an issue we probably would
have switched to ELF function versioning (like glibc does it) long time ago.
>> I think it is important to keep versioning information outside of the
>> source code. Some kind of modversions will still be required, but
>> distros should be able to decide if they put in some kind of checksum or
>> a string, what suites them most.
>
> The module crc symbols are just an integer that requires a match, so it
> could easily be populated by a list that the distro keeps, rather than
> by genksyms. Most of the complexity is on the build side, so that would
> still be an improvement for the kernel. So we *could* do this if the
> distros need it.
Like Don also already said, genksyms already did a pretty good job so
far. We are right now working with Dodji to come up with a way to
replace genksyms, in case people really want to have very specific
control about what causes the symbol version to be changed.
Also I wonder what Ben's opinion on this is.. As I understood that he
wants to maintain a super-long-term stable kernel with kabi guarantees.
Note, what we want is to weaken the check for kabi, by excluding parts
of the struct from genksyms with libabigail. For Red Hat genksyms is too
strict in the checks.
Bye,
Hannes
On Thu, 15 Dec 2016 12:19:02 +0100
Hannes Frederic Sowa <[email protected]> wrote:
> On 15.12.2016 03:06, Nicholas Piggin wrote:
> > On Wed, 14 Dec 2016 15:04:36 +0100
> > Hannes Frederic Sowa <[email protected]> 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 <[email protected]> wrote:
> >>>>
> >>>>>>>>> The question is how to provide a similar guarantee if a different way?
> >>>>>>>> As a tool to aid distro reviewers, modversions has some value, but the
> >>>>>>>> debug info parsing tools that have been mentioned in this thread seem
> >>>>>>>> superior (not that I've tested them).
> >>>>>>> On the other hand the big advantage of modversions is that it also
> >>>>>>> verifies the checksum during runtime (module loading). In other words, I
> >>>>>>> believe that any other solution should still generate some form of
> >>>>>>> checksum/watermark which can be easily checked for compatibility on
> >>>>>>> module load.
> >>>>>>> It should not be hard to add to the DWARF based tools though. We'd just
> >>>>>>> parse DWARF data instead of the C code.
> >>>>>> A runtime check is still done, with per-module vermagic which distros
> >>>>>> can change when they bump the ABI version. Is it really necessary to
> >>>>>> have more than that (i.e., per-symbol versioning)?
> >>>>>
> >>>>> From my point of view, it is. We need to allow changing ABI for some
> >>>>> modules while maintaining it for others.
> >>>>> In fact I think that there should be version not only for every exported
> >>>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type
> >>>>> (in the sense of eg. structure defined in the public header file).
> >>>>
> >>>> Well the distro can just append _v2, _v3 to the name of the function
> >>>> or type if it has to break compat for some reason. Would that be enough?
> >>>
> >>> There are other ways that distros can work around when upstream "breaks"
> >>> the ABI, sometimes they can rename functions, and others they can
> >>> "preload" structures with padding in anticipation for when/if fields get
> >>> added to them. But that's all up to the distros, no need for us to
> >>> worry about that at all :)
> >>
> >> The _v2 and _v3 functions are probably the ones that also get used by
> >> future backports in the distro kernel itself and are probably the reason
> >> for the ABI change in the first place. Thus going down this route will
> >> basically require distros to touch every future backport patch and will
> >> in general generate a big mess internally.
> >
> > What kind of big mess? You have to check the logic of each backport even
> > if it does apply cleanly, so the added overhead of the name change should
> > be relatively tiny, no?
>
> Basically single patches are backported in huge series. Reviewing each
> single patch also definitely makes sense, a review of the series as a
> whole is much more worthwhile because it focuses more on logic.
>
> The patches themselves are checked by individual robots or humans
> against merge conflict introduced mistakes which ring alarm bells for
> people to look more closely during review.
>
> Merge conflicts introduced mistakes definitely can happen because
> developers/backporters lose the focus from the actual logic but deal
> with shifting lines around or just fixing up postfixes to function names.
>
> We still try to align the kernel as much as possible with upstream,
> because most developers can't really hold the differences between
> upstream and the internal functions in their heads (is this function RMW
> safe in this version but not that kernel version...).
I agree with all this, but in the case of a function rename, you can
automate it all with scripts if that's what you want.
When you have your list of exported symbols with non-zero version number,
then you can script that __abivXXX into the changeset applying process,
or alternatively apply the rename after your patches are applied, or
use the c preprocessor to define names to something else.
>
> Anyway, I don't think we will at any time have multiple versions of a
> function exported to 3rd party kernel modules. The headaches are just
> too big. Basically we would have to version structs and not functions
> (this is our bigger problem), thus exporting new versions of functions
> don't really help at all. Having multiple versions of structs really
> scares me. ;)
>
> We already pad structs to allow for additional struct members to be
> added, which helps a lot.
>
> If versioning of function symbols would be an issue we probably would
> have switched to ELF function versioning (like glibc does it) long time ago.
>
> >> I think it is important to keep versioning information outside of the
> >> source code. Some kind of modversions will still be required, but
> >> distros should be able to decide if they put in some kind of checksum or
> >> a string, what suites them most.
> >
> > The module crc symbols are just an integer that requires a match, so it
> > could easily be populated by a list that the distro keeps, rather than
> > by genksyms. Most of the complexity is on the build side, so that would
> > still be an improvement for the kernel. So we *could* do this if the
> > distros need it.
>
> Like Don also already said, genksyms already did a pretty good job so
> far. We are right now working with Dodji to come up with a way to
> replace genksyms, in case people really want to have very specific
> control about what causes the symbol version to be changed.
Yeah it's great work, so is Stanislav's checker. I wouldn't mind having
a kernel-centric checker tool merged in the kernel if it is small,
maintained, and does a sufficient job for distros.
> Also I wonder what Ben's opinion on this is.. As I understood that he
> wants to maintain a super-long-term stable kernel with kabi guarantees.
>
> Note, what we want is to weaken the check for kabi, by excluding parts
> of the struct from genksyms with libabigail. For Red Hat genksyms is too
> strict in the checks.
Sure, that makes sense.
So if I understand where we are, moving the ABI compatibility checking
to one of these tools looks possible. What to do when we have an ABI change
is not settled, but feeding version numbers explicitly into modversions
is an option that would be close to what distros do today.
Thanks,
Nick
On 15.12.2016 13:03, Nicholas Piggin wrote:
> On Thu, 15 Dec 2016 12:19:02 +0100
> Hannes Frederic Sowa <[email protected]> wrote:
>
>> On 15.12.2016 03:06, Nicholas Piggin wrote:
>>> On Wed, 14 Dec 2016 15:04:36 +0100
>>> Hannes Frederic Sowa <[email protected]> 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 <[email protected]> wrote:
>>>>>>
>>>>>>>>>>> The question is how to provide a similar guarantee if a different way?
>>>>>>>>>> As a tool to aid distro reviewers, modversions has some value, but the
>>>>>>>>>> debug info parsing tools that have been mentioned in this thread seem
>>>>>>>>>> superior (not that I've tested them).
>>>>>>>>> On the other hand the big advantage of modversions is that it also
>>>>>>>>> verifies the checksum during runtime (module loading). In other words, I
>>>>>>>>> believe that any other solution should still generate some form of
>>>>>>>>> checksum/watermark which can be easily checked for compatibility on
>>>>>>>>> module load.
>>>>>>>>> It should not be hard to add to the DWARF based tools though. We'd just
>>>>>>>>> parse DWARF data instead of the C code.
>>>>>>>> A runtime check is still done, with per-module vermagic which distros
>>>>>>>> can change when they bump the ABI version. Is it really necessary to
>>>>>>>> have more than that (i.e., per-symbol versioning)?
>>>>>>>
>>>>>>> From my point of view, it is. We need to allow changing ABI for some
>>>>>>> modules while maintaining it for others.
>>>>>>> In fact I think that there should be version not only for every exported
>>>>>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type
>>>>>>> (in the sense of eg. structure defined in the public header file).
>>>>>>
>>>>>> Well the distro can just append _v2, _v3 to the name of the function
>>>>>> or type if it has to break compat for some reason. Would that be enough?
>>>>>
>>>>> There are other ways that distros can work around when upstream "breaks"
>>>>> the ABI, sometimes they can rename functions, and others they can
>>>>> "preload" structures with padding in anticipation for when/if fields get
>>>>> added to them. But that's all up to the distros, no need for us to
>>>>> worry about that at all :)
>>>>
>>>> The _v2 and _v3 functions are probably the ones that also get used by
>>>> future backports in the distro kernel itself and are probably the reason
>>>> for the ABI change in the first place. Thus going down this route will
>>>> basically require distros to touch every future backport patch and will
>>>> in general generate a big mess internally.
>>>
>>> What kind of big mess? You have to check the logic of each backport even
>>> if it does apply cleanly, so the added overhead of the name change should
>>> be relatively tiny, no?
>>
>> Basically single patches are backported in huge series. Reviewing each
>> single patch also definitely makes sense, a review of the series as a
>> whole is much more worthwhile because it focuses more on logic.
>>
>> The patches themselves are checked by individual robots or humans
>> against merge conflict introduced mistakes which ring alarm bells for
>> people to look more closely during review.
>>
>> Merge conflicts introduced mistakes definitely can happen because
>> developers/backporters lose the focus from the actual logic but deal
>> with shifting lines around or just fixing up postfixes to function names.
>>
>> We still try to align the kernel as much as possible with upstream,
>> because most developers can't really hold the differences between
>> upstream and the internal functions in their heads (is this function RMW
>> safe in this version but not that kernel version...).
>
> I agree with all this, but in the case of a function rename, you can
> automate it all with scripts if that's what you want.
>
> When you have your list of exported symbols with non-zero version number,
> then you can script that __abivXXX into the changeset applying process,
> or alternatively apply the rename after your patches are applied, or
> use the c preprocessor to define names to something else.
Yes, probably one could come up with coccinelle patches to do this,
preprocessing/string matching could have false positives. But as I wrote
above, we need one stable ABI and not multiple for our particular
kernels, so it seems like a lot of overhead to rename particular
functions internally all the time to make them inaccessible for external
modules.
>> Anyway, I don't think we will at any time have multiple versions of a
>> function exported to 3rd party kernel modules. The headaches are just
>> too big. Basically we would have to version structs and not functions
>> (this is our bigger problem), thus exporting new versions of functions
>> don't really help at all. Having multiple versions of structs really
>> scares me. ;)
>>
>> We already pad structs to allow for additional struct members to be
>> added, which helps a lot.
>>
>> If versioning of function symbols would be an issue we probably would
>> have switched to ELF function versioning (like glibc does it) long time ago.
>>
>>>> I think it is important to keep versioning information outside of the
>>>> source code. Some kind of modversions will still be required, but
>>>> distros should be able to decide if they put in some kind of checksum or
>>>> a string, what suites them most.
>>>
>>> The module crc symbols are just an integer that requires a match, so it
>>> could easily be populated by a list that the distro keeps, rather than
>>> by genksyms. Most of the complexity is on the build side, so that would
>>> still be an improvement for the kernel. So we *could* do this if the
>>> distros need it.
>>
>> Like Don also already said, genksyms already did a pretty good job so
>> far. We are right now working with Dodji to come up with a way to
>> replace genksyms, in case people really want to have very specific
>> control about what causes the symbol version to be changed.
>
> Yeah it's great work, so is Stanislav's checker. I wouldn't mind having
> a kernel-centric checker tool merged in the kernel if it is small,
> maintained, and does a sufficient job for distros.
Yes, I think this needs more experimentation and thought right now
before we can make a decision.
>> Also I wonder what Ben's opinion on this is.. As I understood that he
>> wants to maintain a super-long-term stable kernel with kabi guarantees.
>>
>> Note, what we want is to weaken the check for kabi, by excluding parts
>> of the struct from genksyms with libabigail. For Red Hat genksyms is too
>> strict in the checks.
>
> Sure, that makes sense.
>
> So if I understand where we are, moving the ABI compatibility checking
> to one of these tools looks possible. What to do when we have an ABI change
> is not settled, but feeding version numbers explicitly into modversions
> is an option that would be close to what distros do today.
Agreed!
Thanks also,
Hannes
> Yeah it's great work, so is Stanislav's checker. I wouldn't mind having
> a kernel-centric checker tool merged in the kernel if it is small,
> maintained, and does a sufficient job for distros.
I'd be very happy to see the resulting tool in the kernel tree, as it
needs to be kept in sync with any significant changes done to the kernel
layout in the future.
It's not important if the result is based off libabigail or kabi-dw
code, I'm sure both can be adjusted to serve the needs. kabi-dw tends to
be smaller and still rather proof-of-concept, libabigail is definitely
more mature. The only real difference is C vs. C++ code.
> So if I understand where we are, moving the ABI compatibility checking
> to one of these tools looks possible. What to do when we have an ABI change
> is not settled, but feeding version numbers explicitly into modversions
> is an option that would be close to what distros do today.
Sounds great!
Thank you!
-Stanislav
On Thu, 15 Dec 2016 14:15:31 +0100
Hannes Frederic Sowa <[email protected]> wrote:
> On 15.12.2016 13:03, Nicholas Piggin wrote:
> > On Thu, 15 Dec 2016 12:19:02 +0100
> > Hannes Frederic Sowa <[email protected]> wrote:
> >
> >> On 15.12.2016 03:06, Nicholas Piggin wrote:
> >>> On Wed, 14 Dec 2016 15:04:36 +0100
> >>> Hannes Frederic Sowa <[email protected]> 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 <[email protected]> wrote:
> >>>>>>
> >>>>>>>>>>> The question is how to provide a similar guarantee if a different way?
> >>>>>>>>>> As a tool to aid distro reviewers, modversions has some value, but the
> >>>>>>>>>> debug info parsing tools that have been mentioned in this thread seem
> >>>>>>>>>> superior (not that I've tested them).
> >>>>>>>>> On the other hand the big advantage of modversions is that it also
> >>>>>>>>> verifies the checksum during runtime (module loading). In other words, I
> >>>>>>>>> believe that any other solution should still generate some form of
> >>>>>>>>> checksum/watermark which can be easily checked for compatibility on
> >>>>>>>>> module load.
> >>>>>>>>> It should not be hard to add to the DWARF based tools though. We'd just
> >>>>>>>>> parse DWARF data instead of the C code.
> >>>>>>>> A runtime check is still done, with per-module vermagic which distros
> >>>>>>>> can change when they bump the ABI version. Is it really necessary to
> >>>>>>>> have more than that (i.e., per-symbol versioning)?
> >>>>>>>
> >>>>>>> From my point of view, it is. We need to allow changing ABI for some
> >>>>>>> modules while maintaining it for others.
> >>>>>>> In fact I think that there should be version not only for every exported
> >>>>>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type
> >>>>>>> (in the sense of eg. structure defined in the public header file).
> >>>>>>
> >>>>>> Well the distro can just append _v2, _v3 to the name of the function
> >>>>>> or type if it has to break compat for some reason. Would that be enough?
> >>>>>
> >>>>> There are other ways that distros can work around when upstream "breaks"
> >>>>> the ABI, sometimes they can rename functions, and others they can
> >>>>> "preload" structures with padding in anticipation for when/if fields get
> >>>>> added to them. But that's all up to the distros, no need for us to
> >>>>> worry about that at all :)
> >>>>
> >>>> The _v2 and _v3 functions are probably the ones that also get used by
> >>>> future backports in the distro kernel itself and are probably the reason
> >>>> for the ABI change in the first place. Thus going down this route will
> >>>> basically require distros to touch every future backport patch and will
> >>>> in general generate a big mess internally.
> >>>
> >>> What kind of big mess? You have to check the logic of each backport even
> >>> if it does apply cleanly, so the added overhead of the name change should
> >>> be relatively tiny, no?
> >>
> >> Basically single patches are backported in huge series. Reviewing each
> >> single patch also definitely makes sense, a review of the series as a
> >> whole is much more worthwhile because it focuses more on logic.
> >>
> >> The patches themselves are checked by individual robots or humans
> >> against merge conflict introduced mistakes which ring alarm bells for
> >> people to look more closely during review.
> >>
> >> Merge conflicts introduced mistakes definitely can happen because
> >> developers/backporters lose the focus from the actual logic but deal
> >> with shifting lines around or just fixing up postfixes to function names.
> >>
> >> We still try to align the kernel as much as possible with upstream,
> >> because most developers can't really hold the differences between
> >> upstream and the internal functions in their heads (is this function RMW
> >> safe in this version but not that kernel version...).
> >
> > I agree with all this, but in the case of a function rename, you can
> > automate it all with scripts if that's what you want.
> >
> > When you have your list of exported symbols with non-zero version number,
> > then you can script that __abivXXX into the changeset applying process,
> > or alternatively apply the rename after your patches are applied, or
> > use the c preprocessor to define names to something else.
>
> Yes, probably one could come up with coccinelle patches to do this,
> preprocessing/string matching could have false positives. But as I wrote
> above, we need one stable ABI and not multiple for our particular
> kernels, so it seems like a lot of overhead to rename particular
> functions internally all the time to make them inaccessible for external
> modules.
I can't be sure until it's implemented in a workflow that distros are
happy with of course, but I just don't see why it would be a lot of
overhead. Particularly if you just scripted everything.
How frequently do symbols become incompatible within an ostensibly ABI-
stable release?
> >> Like Don also already said, genksyms already did a pretty good job so
> >> far. We are right now working with Dodji to come up with a way to
> >> replace genksyms, in case people really want to have very specific
> >> control about what causes the symbol version to be changed.
> >
> > Yeah it's great work, so is Stanislav's checker. I wouldn't mind having
> > a kernel-centric checker tool merged in the kernel if it is small,
> > maintained, and does a sufficient job for distros.
>
> Yes, I think this needs more experimentation and thought right now
> before we can make a decision.
Sure, I wanted to mention it in case people had a concern about out
of tree tools. It will depend on what distros end up settling with.
> >> Also I wonder what Ben's opinion on this is.. As I understood that he
> >> wants to maintain a super-long-term stable kernel with kabi guarantees.
> >>
> >> Note, what we want is to weaken the check for kabi, by excluding parts
> >> of the struct from genksyms with libabigail. For Red Hat genksyms is too
> >> strict in the checks.
> >
> > Sure, that makes sense.
> >
> > So if I understand where we are, moving the ABI compatibility checking
> > to one of these tools looks possible. What to do when we have an ABI change
> > is not settled, but feeding version numbers explicitly into modversions
> > is an option that would be close to what distros do today.
>
> Agreed!
Thanks,
Nick
On 15.12.2016 15:15, Nicholas Piggin wrote:
> On Thu, 15 Dec 2016 14:15:31 +0100
> Hannes Frederic Sowa <[email protected]> wrote:
>
>> On 15.12.2016 13:03, Nicholas Piggin wrote:
>>> On Thu, 15 Dec 2016 12:19:02 +0100
>>> Hannes Frederic Sowa <[email protected]> wrote:
>>>
>>>> On 15.12.2016 03:06, Nicholas Piggin wrote:
>>>>> On Wed, 14 Dec 2016 15:04:36 +0100
>>>>> Hannes Frederic Sowa <[email protected]> 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 <[email protected]> wrote:
>>>>>>>>
>>>>>>>>>>>>> The question is how to provide a similar guarantee if a different way?
>>>>>>>>>>>> As a tool to aid distro reviewers, modversions has some value, but the
>>>>>>>>>>>> debug info parsing tools that have been mentioned in this thread seem
>>>>>>>>>>>> superior (not that I've tested them).
>>>>>>>>>>> On the other hand the big advantage of modversions is that it also
>>>>>>>>>>> verifies the checksum during runtime (module loading). In other words, I
>>>>>>>>>>> believe that any other solution should still generate some form of
>>>>>>>>>>> checksum/watermark which can be easily checked for compatibility on
>>>>>>>>>>> module load.
>>>>>>>>>>> It should not be hard to add to the DWARF based tools though. We'd just
>>>>>>>>>>> parse DWARF data instead of the C code.
>>>>>>>>>> A runtime check is still done, with per-module vermagic which distros
>>>>>>>>>> can change when they bump the ABI version. Is it really necessary to
>>>>>>>>>> have more than that (i.e., per-symbol versioning)?
>>>>>>>>>
>>>>>>>>> From my point of view, it is. We need to allow changing ABI for some
>>>>>>>>> modules while maintaining it for others.
>>>>>>>>> In fact I think that there should be version not only for every exported
>>>>>>>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type
>>>>>>>>> (in the sense of eg. structure defined in the public header file).
>>>>>>>>
>>>>>>>> Well the distro can just append _v2, _v3 to the name of the function
>>>>>>>> or type if it has to break compat for some reason. Would that be enough?
>>>>>>>
>>>>>>> There are other ways that distros can work around when upstream "breaks"
>>>>>>> the ABI, sometimes they can rename functions, and others they can
>>>>>>> "preload" structures with padding in anticipation for when/if fields get
>>>>>>> added to them. But that's all up to the distros, no need for us to
>>>>>>> worry about that at all :)
>>>>>>
>>>>>> The _v2 and _v3 functions are probably the ones that also get used by
>>>>>> future backports in the distro kernel itself and are probably the reason
>>>>>> for the ABI change in the first place. Thus going down this route will
>>>>>> basically require distros to touch every future backport patch and will
>>>>>> in general generate a big mess internally.
>>>>>
>>>>> What kind of big mess? You have to check the logic of each backport even
>>>>> if it does apply cleanly, so the added overhead of the name change should
>>>>> be relatively tiny, no?
>>>>
>>>> Basically single patches are backported in huge series. Reviewing each
>>>> single patch also definitely makes sense, a review of the series as a
>>>> whole is much more worthwhile because it focuses more on logic.
>>>>
>>>> The patches themselves are checked by individual robots or humans
>>>> against merge conflict introduced mistakes which ring alarm bells for
>>>> people to look more closely during review.
>>>>
>>>> Merge conflicts introduced mistakes definitely can happen because
>>>> developers/backporters lose the focus from the actual logic but deal
>>>> with shifting lines around or just fixing up postfixes to function names.
>>>>
>>>> We still try to align the kernel as much as possible with upstream,
>>>> because most developers can't really hold the differences between
>>>> upstream and the internal functions in their heads (is this function RMW
>>>> safe in this version but not that kernel version...).
>>>
>>> I agree with all this, but in the case of a function rename, you can
>>> automate it all with scripts if that's what you want.
>>>
>>> When you have your list of exported symbols with non-zero version number,
>>> then you can script that __abivXXX into the changeset applying process,
>>> or alternatively apply the rename after your patches are applied, or
>>> use the c preprocessor to define names to something else.
>>
>> Yes, probably one could come up with coccinelle patches to do this,
>> preprocessing/string matching could have false positives. But as I wrote
>> above, we need one stable ABI and not multiple for our particular
>> kernels, so it seems like a lot of overhead to rename particular
>> functions internally all the time to make them inaccessible for external
>> modules.
>
> I can't be sure until it's implemented in a workflow that distros are
> happy with of course, but I just don't see why it would be a lot of
> overhead. Particularly if you just scripted everything.
Yes, me neither, of course. I just would favor to get away with the
scripting if possible. ;)
> How frequently do symbols become incompatible within an ostensibly ABI-
> stable release?
Given how genksyms works right now, I would say quite frequently. I
would even go that far to say that every larger backport I had to deal
with in networking had those problems because of some very important top
level structs that make everything reachable (struct net_device or
struct sock), so each change to those structs basically cause a kabi change.
Even if we replace padding in a structure we added before a release,
this is actually considered a kabi breakage, we must annotate this
appropriately because of genksyms.
E.g. I also thought about reordering the structs. As already discussed
in this thread, genksyms takes ordering of its definition vs.
declarations into account, e.g.:
struct foo;
struct bar {
struct foo *f;
};
<< private >>
struct foo {
};
wouldn't actually include struct foo as part of the symvers, but the
other way around it would.
Instead of burden upstream with this kind of reordering, a suppression
engine would be favored by me which we can drive by review and
discussions. Thus upstream doesn't have any burden on that, but it is
solely the burden of the distributions, where the burden belongs. ;)
>>>> Like Don also already said, genksyms already did a pretty good job so
>>>> far. We are right now working with Dodji to come up with a way to
>>>> replace genksyms, in case people really want to have very specific
>>>> control about what causes the symbol version to be changed.
>>>
>>> Yeah it's great work, so is Stanislav's checker. I wouldn't mind having
>>> a kernel-centric checker tool merged in the kernel if it is small,
>>> maintained, and does a sufficient job for distros.
>>
>> Yes, I think this needs more experimentation and thought right now
>> before we can make a decision.
>
> Sure, I wanted to mention it in case people had a concern about out
> of tree tools. It will depend on what distros end up settling with.
The possibilities I see right now:
* we leave genksyms where it is for the time being and add a KCONFIG
knob to be able to call a replacement tool with well formed command line
syntax and stdout semantics.
* we import one of the alternatives instead of genksyms if it fits to
the kernel. I am still a bit concerned about differences in dwarf vs.
source code parsing. In the past I had twice bugs in dwarf generation,
but this probably can be dealt with, but just needs a bit more
experience in my opinion.
* we remove genksyms and just use one versioning vector for upstream but
still allow some kind of modversions per symbol being loaded from a
third party program like kabi-dw or libabigail.
Personally, because I see genksyms working quite well right now, I would
stick to it until we (the distributions) came to a conclusion.
Bye,
Hannes