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

Groups > linux.kernel > #1528743 > unrolled thread

Re: BUG: 4.9-rc6 Still "no symbol version" on boot

Back to article view | Back to linux.kernel

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

Contents

  Re: BUG: 4.9-rc6 Still "no symbol version" on boot Philip Müller <philm@manjaro.org> - 2016-11-23 21:10 +0100
    Re: BUG: 4.9-rc6 Still "no symbol version" on boot Robert LeBlanc <robert@leblancnet.us> - 2016-11-23 21:20 +0100
      Re: BUG: 4.9-rc6 Still "no symbol version" on boot Philip Müller <philm@manjaro.org> - 2016-11-23 21:30 +0100
    Re: BUG: 4.9-rc6 Still "no symbol version" on boot Adam Borowski <kilobyte@angband.pl> - 2016-11-23 22:00 +0100
      Re: BUG: 4.9-rc6 Still "no symbol version" on boot Robert LeBlanc <robert@leblancnet.us> - 2016-11-23 22:10 +0100
      [PATCH] x86/kbuild: enable modversions for symbols exported from asm Adam Borowski <kilobyte@angband.pl> - 2016-11-23 23:10 +0100
        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Philip Müller <philm@manjaro.org> - 2016-11-24 00:20 +0100
        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Ingo Molnar <mingo@kernel.org> - 2016-11-24 06:10 +0100
          Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-11-24 06:30 +0100
            Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Ingo Molnar <mingo@kernel.org> - 2016-11-24 07:10 +0100
              Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-11-24 08:30 +0100
                Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-24 08:40 +0100
                  Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-11-24 09:00 +0100
                    Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-11-24 10:40 +0100
                      Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-11-24 11:10 +0100
                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-11-24 12:00 +0100
                    Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Arnd Bergmann <arnd@arndb.de> - 2016-11-24 10:50 +0100
                      Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-11-24 11:10 +0100
                    Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-24 11:00 +0100
                      Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-11-24 11:40 +0100
                        Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-24 16:30 +0100
                          Re: [PATCH] x86/kbuild: enable modversions for symbols exported  from asm Nicholas Piggin <npiggin@gmail.com> - 2016-11-25 01:50 +0100
          Re: [PATCH] x86/kbuild: enable modversions for symbols exported from  asm Michal Marek <mmarek@suse.com> - 2016-11-24 10:30 +0100
        Regression: Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm Kalle Valo <kvalo@codeaurora.org> - 2016-11-24 12:50 +0100
      Re: BUG: 4.9-rc6 Still "no symbol version" on boot Philip Müller <philm@manjaro.org> - 2016-11-24 00:10 +0100

Page 1 of 2  [1] 2  Next page →

#1528743 — Re: BUG: 4.9-rc6 Still "no symbol version" on boot

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 <philm@manjaro.org> 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
> 

[toc] | [next] | [standalone]

#1528757

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 <philm@manjaro.org> 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 <philm@manjaro.org> 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
>>
>

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

#1528762

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

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

#1528772

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?

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

#1528776

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 <kilobyte@angband.pl> 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?

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

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

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 <kilobyte@angband.pl>
Tested-by: Kalle Valo <kvalo@codeaurora.org>
Acked-by: Nicholas Piggin <npiggin@gmail.com>
Tested-by: Peter Wu <peter@lekensteyn.nl>
Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
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

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

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

Thx Adam for the cleaned up patch ...

Acked-by: Philip Mueller <philm@manjaro.org>
Tested-by: Philip Mueller <philm@manjaro.org>

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

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

* Adam Borowski <kilobyte@angband.pl> 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 <kilobyte@angband.pl>
> Tested-by: Kalle Valo <kvalo@codeaurora.org>
> Acked-by: Nicholas Piggin <npiggin@gmail.com>
> Tested-by: Peter Wu <peter@lekensteyn.nl>
> Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> 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 <npiggin@gmail.com>
  AuthorDate: Tue Nov 1 12:46:19 2016 +1100
  Commit:     Michal Marek <mmarek@suse.com>
  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

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

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

On Thu, 24 Nov 2016 05:40:28 +0100
Ingo Molnar <mingo@kernel.org> wrote:

> * Adam Borowski <kilobyte@angband.pl> 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 <kilobyte@angband.pl>
> > Tested-by: Kalle Valo <kvalo@codeaurora.org>
> > Acked-by: Nicholas Piggin <npiggin@gmail.com>
> > Tested-by: Peter Wu <peter@lekensteyn.nl>
> > Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
> > ---
> > 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 <npiggin@gmail.com>
>   AuthorDate: Tue Nov 1 12:46:19 2016 +1100
>   Commit:     Michal Marek <mmarek@suse.com>
>   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

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

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

* Nicholas Piggin <npiggin@gmail.com> 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

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

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

On Thu, 24 Nov 2016 07:00:50 +0100
Ingo Molnar <mingo@kernel.org> wrote:

> * Nicholas Piggin <npiggin@gmail.com> 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

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

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

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

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

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

On Thu, 24 Nov 2016 08:36:39 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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

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

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

On 2016-11-24 08:53, Nicholas Piggin wrote:
> On Thu, 24 Nov 2016 08:36:39 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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

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

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

On Thu, 24 Nov 2016 10:32:12 +0100
Michal Marek <mmarek@suse.com> wrote:

> On 2016-11-24 08:53, Nicholas Piggin wrote:
> > On Thu, 24 Nov 2016 08:36:39 +0100
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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

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

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

Dne 24.11.2016 v 11:03 Nicholas Piggin napsal(a):
> On Thu, 24 Nov 2016 10:32:12 +0100
> Michal Marek <mmarek@suse.com> wrote:
> 
>> On 2016-11-24 08:53, Nicholas Piggin wrote:
>>> On Thu, 24 Nov 2016 08:36:39 +0100
>>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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

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

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

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

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

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

On Thu, 24 Nov 2016 10:38:04 +0100
Arnd Bergmann <arnd@arndb.de> 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

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

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

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 <gregkh@linuxfoundation.org> 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

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

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

On Thu, 24 Nov 2016 10:56:22 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web