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

Groups > linux.kernel > #1530479 > unrolled thread

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

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

Page 1 of 4  [1] 2 3 4  Next page →

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

On Thu, Nov 24, 2016 at 4:40 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Yes, manual "marking" is never going to be a viable solution.
>
> I guess it really depends on how exactly you want to use it. For distros
> that do stable ABI but rarely may have to break something for security
> reasons, it should work and give exact control.

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

[toc] | [next] | [standalone]

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

On Fri, 25 Nov 2016 10:00:46 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Nov 24, 2016 at 4:40 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
> >>
> >> Yes, manual "marking" is never going to be a viable solution.  
> >
> > I guess it really depends on how exactly you want to use it. For distros
> > that do stable ABI but rarely may have to break something for security
> > reasons, it should work and give exact control.  
> 
> 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

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

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

[Multipart message — attachments visible in raw view] — view raw

[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 <npiggin@gmail.com> wrote:
> > > 
> > > Yes, manual "marking" is never going to be a viable solution.
> > 
> > I guess it really depends on how exactly you want to use it. For distros
> > that do stable ABI but rarely may have to break something for security
> > reasons, it should work and give exact control.

This is roughly how Debian handles the kernel module ABI during a
stable release.

> No. Because nobody else will care, so unless it's like a single symbol
> or something, it will just be a maintenance nightmare.

I agree with this.  We can explicitly "version" individual symbols
anyway by doing something like:

-int foo(void);
+#define foo foo_2
+int foo_2(int);

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

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

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

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

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

You got it about right.

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

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

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

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

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

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

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

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

Thanks,
Nick

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

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

Dne 29.11.2016 v 03:31 Nicholas Piggin napsal(a):
> On Tue, 29 Nov 2016 01:15:48 +0000
> Ben Hutchings <ben@decadent.org.uk> wrote:
> 
>> [I've had to guess at the cc list for this, because we no longer have
>> mail archives that preserve them.]
> 
> You got it about right.
> 
>> On Fri, 2016-11-25 at 10:01 -0800, Linus Torvalds wrote:
>>> On Thu, Nov 24, 2016 at 4:40 PM, Nicholas Piggin <npiggin@gmail.com> wrote:  
>>>>>
>>>>> Yes, manual "marking" is never going to be a viable solution.  
>>>>
>>>> I guess it really depends on how exactly you want to use it. For distros
>>>> that do stable ABI but rarely may have to break something for security
>>>> reasons, it should work and give exact control.  
>>
>> This is roughly how Debian handles the kernel module ABI during a
>> stable release.
>>
>>> No. Because nobody else will care, so unless it's like a single symbol
>>> or something, it will just be a maintenance nightmare.  
>>
>> I agree with this.  We can explicitly "version" individual symbols
>> anyway by doing something like:
>>
>> -int foo(void);
>> +#define foo foo_2
>> +int foo_2(int);
> 
> Yeah... Benefit being it's very simple and everybody can see exactly
> what it does and knows how it will work.
> 
>>
>>>> What else do people *actually* use it for? Preventing mismatched modules
>>>> when .git version is not attached and release version of the kernel has
>>>> not been bumped. Is that it?  
>>>
>>> It used to be very useful for avoiding loading stale modules and then
>>> wasting days on debugging something that wasn't the case when you had
>>> forgotten to do "make modules_install". Change some subtle internal
>>> ABI issue (add/remove a parameter, whatever) and it would really help.
>>>
>>> These days, for me, LOCALVERSION_AUTO and module signing are what I
>>> personally tend to use.
>>>
>>> The modversions stuff may just be too painful to bother with. Very few
>>> people probably use it, and the ones that do likely don't have any
>>> overriding reason why.  
>> [...]
>>
>> Debian has some strong reasons:

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

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

#1531888

On Mon, Nov 28, 2016 at 5:15 PM, Ben Hutchings <ben@decadent.org.uk> 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

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

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

On Mon, Nov 28, 2016 at 08:08:57PM -0800, Linus Torvalds wrote:
> On Mon, Nov 28, 2016 at 5:15 PM, Ben Hutchings <ben@decadent.org.uk> 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/linux-kernel@vger.kernel.org/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?

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

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

* Adam Borowski <kilobyte@angband.pl> 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/linux-kernel@vger.kernel.org/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

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

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

On Tue, Nov 29, 2016 at 02:29:54PM +0100, Ingo Molnar wrote:
> * Adam Borowski <kilobyte@angband.pl> 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?

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

#1532310

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

> 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

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

#1532442

[Multipart message — attachments visible in raw view] — view raw

On Nov 29, 2016 5:51 AM, "Adam Borowski" <kilobyte@angband.pl> 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.

I suspect one of the other patches already fixed is for x86.

     Linus

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

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

Dne 29.11.2016 v 16:27 Linus Torvalds napsal(a):
> On Nov 29, 2016 5:51 AM, "Adam Borowski" <kilobyte@angband.pl
> <mailto:kilobyte@angband.pl>> 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

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

#1532489

On Tue, Nov 29, 2016 at 8:03 AM, Michal Marek <mmarek@suse.com> 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

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

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

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2016-11-29 at 08:17 -0800, Linus Torvalds wrote:
> > On Tue, Nov 29, 2016 at 8:03 AM, Michal Marek <mmarek@suse.com> 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

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

#1532760

On Tue, Nov 29, 2016 at 11:57 AM, Ben Hutchings <ben@decadent.org.uk> 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

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

#1533511

On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Here's an initial rough hack at removing modversions. It gives an idea
> of the complexity we're carrying for this feature (keeping in mind most
> of the lines removed are generated parser).

You definitely don't have to try to convince me. We've had many issues
with modversions over the years. This was just the "last drop" as far
as I'm concerned, we've had random odd crc generation failures due to
some build races too.

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

So at least Fedora doesn't even enable CONFIG_MODVERSIONS as-is. I'm
_hoping_ it's just Debian that wants this, and we'd need to get some
input from the Debian people whether that "control vermagic" is
sufficient? I suspect it isn't, but I can't come up with any simple
alternate model either..

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

            Linus

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

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

[Multipart message — attachments visible in raw view] — view raw

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

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.

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

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

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

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

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

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

[Multipart message — attachments visible in raw view] — view raw

On Thu, 2016-12-01 at 12:55 +1100, Nicholas Piggin wrote:
> On Wed, 30 Nov 2016 21:33:01 +0000
> > Ben Hutchings <ben@decadent.org.uk> wrote:
> 
> > On Wed, 2016-11-30 at 10:40 -0800, Linus Torvalds wrote:
> > > > On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
> > > > 
> > > > Here's an initial rough hack at removing modversions. It gives an idea
> > > > of the complexity we're carrying for this feature (keeping in mind most
> > > > of the lines removed are generated parser).  
> > > 
> > > You definitely don't have to try to convince me. We've had many issues
> > > with modversions over the years. This was just the "last drop" as far
> > > as I'm concerned, we've had random odd crc generation failures due to
> > > some build races too.
> > >   
> > > > In its place I just added a simple config option to override vermagic
> > > > so distros can manage it entirely themselves.  
> > > 
> > > So at least Fedora doesn't even enable CONFIG_MODVERSIONS as-is. I'm
> > > _hoping_ it's just Debian that wants this,  
> > 
> > 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

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

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

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

> On Thu, 2016-12-01 at 12:55 +1100, Nicholas Piggin wrote:
> > On Wed, 30 Nov 2016 21:33:01 +0000  
> > > Ben Hutchings <ben@decadent.org.uk> wrote:  
> >   
> > > On Wed, 2016-11-30 at 10:40 -0800, Linus Torvalds wrote:  
> > > > > On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
> > > > > 
> > > > > Here's an initial rough hack at removing modversions. It gives an idea
> > > > > of the complexity we're carrying for this feature (keeping in mind most
> > > > > of the lines removed are generated parser).    
> > > > 
> > > > You definitely don't have to try to convince me. We've had many issues
> > > > with modversions over the years. This was just the "last drop" as far
> > > > as I'm concerned, we've had random odd crc generation failures due to
> > > > some build races too.
> > > >     
> > > > > In its place I just added a simple config option to override vermagic
> > > > > so distros can manage it entirely themselves.    
> > > > 
> > > > So at least Fedora doesn't even enable CONFIG_MODVERSIONS as-is. I'm
> > > > _hoping_ it's just Debian that wants this,    
> > > 
> > > 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

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

Page 1 of 4  [1] 2 3 4  Next page →

Back to top | Article view | linux.kernel

csiph-web