| Message ID | 20161201051852.28dc335f@roar.ozlabs.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 30, 2016 at 10:40:02AM -0800, Linus Torvalds wrote: > On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <npiggin@gmail.com> wrote: > > > > Here's an initial rough hack at removing modversions. It gives an idea > > of the complexity we're carrying for this feature (keeping in mind most > > of the lines removed are generated parser). > > You definitely don't have to try to convince me. We've had many issues > with modversions over the years. This was just the "last drop" as far > as I'm concerned, we've had random odd crc generation failures due to > some build races too. > > > In its place I just added a simple config option to override vermagic > > so distros can manage it entirely themselves. > > So at least Fedora doesn't even enable CONFIG_MODVERSIONS as-is. I'm > _hoping_ it's just Debian that wants this, and we'd need to get some > input from the Debian people whether that "control vermagic" is > sufficient? I suspect it isn't, but I can't come up with any simple > alternate model either.. Oddly, I just posted a patch to enable this for Fedora and then someone pointed me at this thread. :-/ Sorry for chiming in late, but yes RHEL is a big user of MODVERSIONS for our kabi protection work. Despite our best intentions we still have lots of partners and customers that provide value-add out-of-tree drivers to their customers. These module builders requested we have a mechanism to allow rolling modules forward for each of our minor RHEL updates without breaking their drivers. They requested this to save time and money on rebuilding and retesting. It also helps deal with situations where RHEL puts out a security fix or new minor release and the provider of OOT driver has not released the appropriate update. Customers like the ability to roll their special drivers forward quickly to their schedule. Now we don't protect every symbol, just a select few that our meets our customers needs (and developers willing to support it). Anyway, MODVERSIONS is our way of protecting our kabi for the last 10 years. It isn't perfect and we have fixed the genksyms tool over the years, but so far it mostly works fine. I am not sure what 'control vermagic' is, but it sounds like a string check, which won't protect against the boatload of backports we do to structs, enums, and functions. Currently we are exploring various ways to get smarter here. The genksyms tool has its limitations and handling kabi hacks in RHEL is getting tiresome. I think GregKH pointed to one such tool, libabigail? We are working on others too. Circling back to enabling MODVERSIONS in Fedora, that was to start the process of syncing Fedora with RHEL stuff in preparation for smarter tools. If you take away MODVERSIONS, that would put a damper in our work, but easily carried privately (much like MODSIGNING for 8 years until it went upstream :-) ). We would prefer to work with various folks to figure out a better solution to solve our/others needs. Anyone interested in working with Red Hat should contact Stanislav Kozina (skozina@redhat.com) (cc'd above) and cc myself. Cheers, Don > > I'm also somewhat surprised that it's Debian that has this problem, > considering how Debian is usually the distro that is _least_ receptive > to various non-free binaries. > > Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 30 Nov 2016 23:13:25 -0500 Don Zickus <dzickus@redhat.com> wrote: > On Wed, Nov 30, 2016 at 10:40:02AM -0800, Linus Torvalds wrote: > > On Wed, Nov 30, 2016 at 10:18 AM, Nicholas Piggin <npiggin@gmail.com> wrote: > > > > > > Here's an initial rough hack at removing modversions. It gives an idea > > > of the complexity we're carrying for this feature (keeping in mind most > > > of the lines removed are generated parser). > > > > You definitely don't have to try to convince me. We've had many issues > > with modversions over the years. This was just the "last drop" as far > > as I'm concerned, we've had random odd crc generation failures due to > > some build races too. > > > > > In its place I just added a simple config option to override vermagic > > > so distros can manage it entirely themselves. > > > > So at least Fedora doesn't even enable CONFIG_MODVERSIONS as-is. I'm > > _hoping_ it's just Debian that wants this, and we'd need to get some > > input from the Debian people whether that "control vermagic" is > > sufficient? I suspect it isn't, but I can't come up with any simple > > alternate model either.. > > Oddly, I just posted a patch to enable this for Fedora and then someone > pointed me at this thread. :-/ > > Sorry for chiming in late, but yes RHEL is a big user of MODVERSIONS for our > kabi protection work. Despite our best intentions we still have lots of > partners and customers that provide value-add out-of-tree drivers to their > customers. These module builders requested we have a mechanism to allow > rolling modules forward for each of our minor RHEL updates without breaking > their drivers. > > They requested this to save time and money on rebuilding and retesting. It > also helps deal with situations where RHEL puts out a security fix or new > minor release and the provider of OOT driver has not released the > appropriate update. Customers like the ability to roll their special > drivers forward quickly to their schedule. > > Now we don't protect every symbol, just a select few that our meets our > customers needs (and developers willing to support it). > > Anyway, MODVERSIONS is our way of protecting our kabi for the last 10 years. > It isn't perfect and we have fixed the genksyms tool over the years, but so > far it mostly works fine. Okay. It would be good to get all the distros in on this. What I want to do is work out exactly what it is that modversions is giving you. We know it's fairly nasty code to maintain and it does not detect ABI changes very well. But it's not such a burden that we can't maintain it if there are good reasons to keep it. > I am not sure what 'control vermagic' is, but it sounds like a string check, > which won't protect against the boatload of backports we do to structs, > enums, and functions. Basically vermagic is the string all modules and the kernel get, which must match in order to load modules. If you have modversions disabled, then vermagic includes the kernel version. If modversions is enabled, then vermagic does not include the kernel version but the CRCs have to also match. Controlling it explicitly is just a couple of lines where a distro can control it (so they can update their kernel version without breaking). It's not meant to solve everything, just the first one. > Currently we are exploring various ways to get smarter here. The genksyms > tool has its limitations and handling kabi hacks in RHEL is getting > tiresome. > > I think GregKH pointed to one such tool, libabigail? We are working on > others too. > > > Circling back to enabling MODVERSIONS in Fedora, that was to start the > process of syncing Fedora with RHEL stuff in preparation for smarter tools. > > > If you take away MODVERSIONS, that would put a damper in our work, but > easily carried privately (much like MODSIGNING for 8 years until it went > upstream :-) ). I don't think that's necessary. A feature requirement for a distro is just as valid as any other user of upstream. I don't want to hinder any distro, I'm just still not quite seeing the big picture of exactly what functionality you need from the kernel. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2016 05:13 AM, Don Zickus wrote: ... > I think GregKH pointed to one such tool, libabigail? We are working on > others too. I should mention one of the others here: https://github.com/skozina/kabi-dw It's quite comparable to libabigail in the way it works, the main differences are: - written in pure C - depends only on elf-utils and flex/yacc - it's much simpler (4k LOC) - stores the type information in the text files and compares those instead of directly comparing two sets of DWARF data Regards, -Stanislav -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 1 Dec 2016 11:48:09 +0100 Stanislav Kozina <skozina@redhat.com> wrote: > On 12/01/2016 05:13 AM, Don Zickus wrote: > > ... > > > I think GregKH pointed to one such tool, libabigail? We are working on > > others too. > > I should mention one of the others here: > https://github.com/skozina/kabi-dw > > It's quite comparable to libabigail in the way it works, the main > differences are: > - written in pure C > - depends only on elf-utils and flex/yacc > - it's much simpler (4k LOC) > - stores the type information in the text files and compares those > instead of directly comparing two sets of DWARF data Now this seems much better for distro ABI checking. The next question is, do they need any kernel support for rare cases where they do have to break the ABI of an export? Simple rename of the function with a _v2 postfix might be enough. We could retain some per symbol versioning in the kernel if needed, but how much would it actually help? Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2016 12:09 PM, Nicholas Piggin wrote: > On Thu, 1 Dec 2016 11:48:09 +0100 > Stanislav Kozina <skozina@redhat.com> wrote: > >> On 12/01/2016 05:13 AM, Don Zickus wrote: >> >> ... >> >>> I think GregKH pointed to one such tool, libabigail? We are working on >>> others too. >> I should mention one of the others here: >> https://github.com/skozina/kabi-dw >> >> It's quite comparable to libabigail in the way it works, the main >> differences are: >> - written in pure C >> - depends only on elf-utils and flex/yacc >> - it's much simpler (4k LOC) >> - stores the type information in the text files and compares those >> instead of directly comparing two sets of DWARF data > Now this seems much better for distro ABI checking. > > The next question is, do they need any kernel support for rare cases > where they do have to break the ABI of an export? Simple rename of the > function with a _v2 postfix might be enough. We could retain some per > symbol versioning in the kernel if needed, but how much would it > actually help? The biggest pain point AFAICT is to identify what types (functions, structs, enums, ...) should be considered a part of the stable ABI. And the problem with modversions is that it pulls in just everything which gets (accidentally?) #included in the source file. The actual ABI maintenance is a different problem, but there are many possible approaches, the _v2 suffix being one of them. Regards, -Stanislav -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 1 Dec 2016 12:33:02 +0100 Stanislav Kozina <skozina@redhat.com> wrote: > On 12/01/2016 12:09 PM, Nicholas Piggin wrote: > > On Thu, 1 Dec 2016 11:48:09 +0100 > > Stanislav Kozina <skozina@redhat.com> wrote: > > > >> On 12/01/2016 05:13 AM, Don Zickus wrote: > >> > >> ... > >> > >>> I think GregKH pointed to one such tool, libabigail? We are working on > >>> others too. > >> I should mention one of the others here: > >> https://github.com/skozina/kabi-dw > >> > >> It's quite comparable to libabigail in the way it works, the main > >> differences are: > >> - written in pure C > >> - depends only on elf-utils and flex/yacc > >> - it's much simpler (4k LOC) > >> - stores the type information in the text files and compares those > >> instead of directly comparing two sets of DWARF data > > Now this seems much better for distro ABI checking. > > > > The next question is, do they need any kernel support for rare cases > > where they do have to break the ABI of an export? Simple rename of the > > function with a _v2 postfix might be enough. We could retain some per > > symbol versioning in the kernel if needed, but how much would it > > actually help? > > The biggest pain point AFAICT is to identify what types (functions, > structs, enums, ...) should be considered a part of the stable ABI. Sure. This is something an automated checker can't solve completely. Any changes would have to be considered in terms of their impact to the ABI. It's not just data but also instruction changes involved. This is policy that should not be mandated by the kernel. Which is why I'm in favor of using tools like this and just providing mechanism so distros can implement their own polices. > And > the problem with modversions is that it pulls in just everything which > gets (accidentally?) #included in the source file. I think that's SRCVERSION which is something else. But modversions has problems too. > The actual ABI maintenance is a different problem, but there are many > possible approaches, the _v2 suffix being one of them. Would be good to get a consensus on that too. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Nicholas Piggin <npiggin@gmail.com> a écrit: [...] > On Thu, 1 Dec 2016 11:48:09 +0100 > Stanislav Kozina <skozina@redhat.com> wrote: > >> On 12/01/2016 05:13 AM, Don Zickus wrote: >> >> ... >> >> > I think GregKH pointed to one such tool, libabigail? We are working on >> > others too. >> >> I should mention one of the others here: >> https://github.com/skozina/kabi-dw [...] > Now this seems much better for distro ABI checking. Right, Incidentally, Fedora does distro-wide ABI verfication for userspace libraries updates in a given stable distribution. You can look at an example of output of the libabigail-based tool that compares a new version of an ELF library to it's latest stable version: https://taskotron.fedoraproject.org/artifacts/all/a55cbac8-ab64-11e6-94e0-525400120b80/task_output/curl-7.51.0-2.fc25.log The results of those ABI comparison can browsed at https://taskotron.fedoraproject.org/resultsdb/results?testcase_name=dist.abicheck. Of course, you can run the comparison yourself by using a libabigail-based tool like https://sourceware.org/libabigail/manual/abipkgdiff.html which takes .deb and .rpm packages. We are currently working on making libabigail-based tools understand Linux kernel specifities so that we can run that kind of analysis on Kernel updates too. Ideally, when this is done, you should be able to just use abipkgdiff on two Linux Kernel packages and get the same kind of output. > The next question is, do they need any kernel support for rare cases > where they do have to break the ABI of an export? Simple rename of the > function with a _v2 postfix might be enough. We could retain some per > symbol versioning in the kernel if needed, but how much would it > actually help? As a reviewer of the ABI change report emitted by the tool, if what you want is to say "I reviewed that ABI change and it's OK, so please do not show it to me next time", then you can feed the tool with a "suppression specification". It's a text file in the INI syntax in which you can specify the kind of change you want the tool to suppress from its output: https://sourceware.org/libabigail/manual/libabigail-concepts.html. So I don't think you need to do anything to the source code of the Kernel in the cases where you need to change the ABI. Just tell the tool about that change. Cheers,
On Thu, Dec 01, 2016 at 03:32:15PM +1100, Nicholas Piggin wrote: > > Anyway, MODVERSIONS is our way of protecting our kabi for the last 10 years. > > It isn't perfect and we have fixed the genksyms tool over the years, but so > > far it mostly works fine. > > Okay. It would be good to get all the distros in on this. > > What I want to do is work out exactly what it is that modversions is > giving you. > > We know it's fairly nasty code to maintain and it does not detect ABI > changes very well. But it's not such a burden that we can't maintain > it if there are good reasons to keep it. Hi Nick, I won't disagree with you there. :-) modversions is a pretty heavy handed approach that basically says if all the symbols and types haven't changed for a given EXPORT_SYMBOL (recursively checked), then there is a high degree of confidence the OOT driver will not only load, but run correctly. The question is how to provide a similar guarantee if a different way? We have plenty of customers with 10 year old drivers, where the expertise has long left the company. The engineers still around, recompile and make tweaks to get things working on the latest RHEL. Verify it passes testing and release it. Then they hope to not touch it again for a few years until the next RHEL comes along. Scary, huh? :-) Common examples, filesystems and storage drivers. There is no way that I see to provide a 100% guarantee, but if we do enough checks, we should be able to have a high degree of confidence the driver won't blow up. On the flip side, easy things in the kernel to do is: - provide the memory allocation (instead of having the driver staticly allocate) - provide functions to retrieve various internal data (instead of having the driver do direct referencing to deep internal elements) - cut down on some static inlines (and use accessory functions instead), etc. Those types of changes allow the OOT driver to be more ignorant of kernel changes and struct modifications. Look to Stanislav's responses for his ideas on new tooling. Thanks for helping! Cheers, Don > > > I am not sure what 'control vermagic' is, but it sounds like a string check, > > which won't protect against the boatload of backports we do to structs, > > enums, and functions. > > Basically vermagic is the string all modules and the kernel get, which > must match in order to load modules. If you have modversions disabled, > then vermagic includes the kernel version. If modversions is enabled, > then vermagic does not include the kernel version but the CRCs have to > also match. > > Controlling it explicitly is just a couple of lines where a distro can > control it (so they can update their kernel version without breaking). > It's not meant to solve everything, just the first one. > > > Currently we are exploring various ways to get smarter here. The genksyms > > tool has its limitations and handling kabi hacks in RHEL is getting > > tiresome. > > > > I think GregKH pointed to one such tool, libabigail? We are working on > > others too. > > > > > > Circling back to enabling MODVERSIONS in Fedora, that was to start the > > process of syncing Fedora with RHEL stuff in preparation for smarter tools. > > > > > > If you take away MODVERSIONS, that would put a damper in our work, but > > easily carried privately (much like MODSIGNING for 8 years until it went > > upstream :-) ). > > I don't think that's necessary. A feature requirement for a distro is just > as valid as any other user of upstream. I don't want to hinder any distro, > I'm just still not quite seeing the big picture of exactly what functionality > you need from the kernel. > > Thanks, > Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 01, 2016 at 10:20:39AM -0500, Don Zickus wrote: > > - provide the memory allocation (instead of having the driver staticly > allocate) > - provide functions to retrieve various internal data (instead of having the > driver do direct referencing to deep internal elements) > - cut down on some static inlines (and use accessory functions instead), > etc. > > Those types of changes allow the OOT driver to be more ignorant of kernel > changes and struct modifications. All that is counter to what we really want to have: a well integrated kernel that moves forward together so that we can see and improve the whole situation. No need to make things worse just to help leeches. Get your damn drivers upstream ASAP and let's stop this discussion.. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 01, 2016 at 07:26:09AM -0800, Christoph Hellwig wrote: > On Thu, Dec 01, 2016 at 10:20:39AM -0500, Don Zickus wrote: > > > > - provide the memory allocation (instead of having the driver staticly > > allocate) > > - provide functions to retrieve various internal data (instead of having the > > driver do direct referencing to deep internal elements) > > - cut down on some static inlines (and use accessory functions instead), > > etc. > > > > Those types of changes allow the OOT driver to be more ignorant of kernel > > changes and struct modifications. > > All that is counter to what we really want to have: a well integrated > kernel that moves forward together so that we can see and improve the > whole situation. No need to make things worse just to help leeches. > Get your damn drivers upstream ASAP and let's stop this discussion.. I understand and won't disagree with you. :-) Unfortunately, there are various drivers that will never go upstream - paid storage drivers that provide bells and whistles on top of inbox driver - old drivers/fs that application has been relying on for a long time but company doesn't have resources to migrate to current technology. We have been trying over the years to do what we can to move customers in the right direction. It is just a slow process, sadly. Cheers, Don -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 01, 2016 at 10:40:59AM -0500, Don Zickus wrote: > Unfortunately, there are various drivers that will never go upstream > > - paid storage drivers that provide bells and whistles on top of inbox > driver That's because the developer doesn't want them upstream, that's their fault, nothing we can do about them. > - old drivers/fs that application has been relying on for a long time but > company doesn't have resources to migrate to current technology. That's what drivers/staging/ is for, I'll take anything that builds (and sometimes stuff that doesn't build) as long as people are actually using it. So send the stuff that is in this category on to me and that will reduce your burden a _lot_. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-12-01 04:39, Nicholas Piggin wrote: > On Thu, 01 Dec 2016 02:35:54 +0000 > Ben Hutchings <ben@decadent.org.uk> wrote: >> As I understand it, genksyms incorporates the definitions of a >> function's parameter and return types - not just their names - and all >> the types they refer to, recursively. So a structure size change >> should change the version of all functions where the function and its >> caller pass that structure between them, however indirectly. It finds >> such indirect ABI breakage for me fairly regularly, though of course I >> don't know that it finds everything. > > It is only the type name. > > Not only that but even if you did extend it further to structure type > arrangement then you still have to deal with other structures followed > via pointers. Or (rarer but not unheard of): > > - changes to structures without changes of the types of their members > - changes to arguments without changes of their type This is already covered by genksyms. Try make V=1 with CONFIG_MODVERSIONS=y and add the -D option to one of the genksyms command. I wanted to paste the expanded signature for register_filesystem() as an example, but vger would probably drop the mail for being too big :). > - changes to semantics of functions > - data structures derived in ways other than exported symbols, e.g., > fixed register for `current` on some archs Right, this is something that genksyms has no idea about. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-12-01 05:13, Don Zickus wrote: > Sorry for chiming in late, but yes RHEL is a big user of MODVERSIONS for our > kabi protection work. Despite our best intentions we still have lots of > partners and customers that provide value-add out-of-tree drivers to their > customers. These module builders requested we have a mechanism to allow > rolling modules forward for each of our minor RHEL updates without breaking > their drivers. FWIW, in SLES we use CONFIG_MODVERSION for pretty much the same reasons you listed. We also enable it in openSUSE, but there it's not as crucial. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 01, 2016 at 05:06:11PM +0100, Greg Kroah-Hartman wrote: > On Thu, Dec 01, 2016 at 10:40:59AM -0500, Don Zickus wrote: > > Unfortunately, there are various drivers that will never go upstream > > > > - paid storage drivers that provide bells and whistles on top of inbox > > driver > > That's because the developer doesn't want them upstream, that's their > fault, nothing we can do about them. > > > - old drivers/fs that application has been relying on for a long time but > > company doesn't have resources to migrate to current technology. > > That's what drivers/staging/ is for, I'll take anything that builds (and > sometimes stuff that doesn't build) as long as people are actually using > it. So send the stuff that is in this category on to me and that will > reduce your burden a _lot_. Hi Greg, I will forward this offer to the right folks and see who we can get to bite. :-) Thanks! Cheers, Don -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01.12.2016 17:12, Michal Marek wrote: > On 2016-12-01 04:39, Nicholas Piggin wrote: >> On Thu, 01 Dec 2016 02:35:54 +0000 >> Ben Hutchings <ben@decadent.org.uk> wrote: >>> As I understand it, genksyms incorporates the definitions of a >>> function's parameter and return types - not just their names - and all >>> the types they refer to, recursively. So a structure size change >>> should change the version of all functions where the function and its >>> caller pass that structure between them, however indirectly. It finds >>> such indirect ABI breakage for me fairly regularly, though of course I >>> don't know that it finds everything. >> >> It is only the type name. >> >> Not only that but even if you did extend it further to structure type >> arrangement then you still have to deal with other structures followed >> via pointers. Or (rarer but not unheard of): >> >> - changes to structures without changes of the types of their members >> - changes to arguments without changes of their type > > This is already covered by genksyms. Try make V=1 with > CONFIG_MODVERSIONS=y and add the -D option to one of the genksyms > command. I wanted to paste the expanded signature for > register_filesystem() as an example, but vger would probably drop the > mail for being too big :). It is easier to just use e.g. `make net/core/dev.symtypes` and look at the generated file. Bye, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 1 Dec 2016 17:12:41 +0100 Michal Marek <mmarek@suse.com> wrote: > On 2016-12-01 04:39, Nicholas Piggin wrote: > > On Thu, 01 Dec 2016 02:35:54 +0000 > > Ben Hutchings <ben@decadent.org.uk> wrote: > >> As I understand it, genksyms incorporates the definitions of a > >> function's parameter and return types - not just their names - and all > >> the types they refer to, recursively. So a structure size change > >> should change the version of all functions where the function and its > >> caller pass that structure between them, however indirectly. It finds > >> such indirect ABI breakage for me fairly regularly, though of course I > >> don't know that it finds everything. > > > > It is only the type name. > > > > Not only that but even if you did extend it further to structure type > > arrangement then you still have to deal with other structures followed > > via pointers. Or (rarer but not unheard of): > > > > - changes to structures without changes of the types of their members > > - changes to arguments without changes of their type > > This is already covered by genksyms. Try make V=1 with > CONFIG_MODVERSIONS=y and add the -D option to one of the genksyms > command. I wanted to paste the expanded signature for > register_filesystem() as an example, but vger would probably drop the > mail for being too big :). Well I simply tested the outcome. If you have: struct blah { int x; }; int foo(struct blah *blah) { return blah->x; } EXPORT(foo); $ nm vmlinux | grep __crc_foo 00000000a0cf13a0 A __crc_foo Now change to struct blah { int y; int x; }; $ nm vmlinux | grep __crc_foo 00000000a0cf13a0 A __crc_foo It just doesn't catch these things. Honestly, stable ABI distros *have* to review all patches to ensure the ABI is unchanged. Some tools could help significantly, but for that, the debug info ABI checking tools that have been mentioned in this thread are far better tool for this job than modversions. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 1 Dec 2016 10:20:39 -0500 Don Zickus <dzickus@redhat.com> wrote: > On Thu, Dec 01, 2016 at 03:32:15PM +1100, Nicholas Piggin wrote: > > > Anyway, MODVERSIONS is our way of protecting our kabi for the last 10 years. > > > It isn't perfect and we have fixed the genksyms tool over the years, but so > > > far it mostly works fine. > > > > Okay. It would be good to get all the distros in on this. > > > > What I want to do is work out exactly what it is that modversions is > > giving you. > > > > We know it's fairly nasty code to maintain and it does not detect ABI > > changes very well. But it's not such a burden that we can't maintain > > it if there are good reasons to keep it. > > Hi Nick, > > I won't disagree with you there. :-) Sorry for the late reply, I was moving house and got side tracked. > modversions is a pretty heavy handed approach that basically says if all the > symbols and types haven't changed for a given EXPORT_SYMBOL (recursively > checked), then there is a high degree of confidence the OOT driver will not > only load, but run correctly. It's heavy handed in that it is quite complex in the kernel build system, but it is also light handed in that it does not do a very good job. I would say the degree of confidence is not very high. People have told me modversions follows pointers to objects in its calculation, but I have not seen that to be the case. Even if you did have that, it can not replace a code review for semantics of data and code. > The question is how to provide a similar guarantee if a different way? As a tool to aid distro reviewers, modversions has some value, but the debug info parsing tools that have been mentioned in this thread seem superior (not that I've tested them). > > We have plenty of customers with 10 year old drivers, where the expertise > has long left the company. The engineers still around, recompile and make > tweaks to get things working on the latest RHEL. Verify it passes testing > and release it. Then they hope to not touch it again for a few years until > the next RHEL comes along. > > Scary, huh? :-) Oh yeah my aim here is not to make distro or out of tree module vendors life harder, actually the opposite. If it turns out modversions really is the best approach, I'm not in a position to complain about its complexity because we have Suse and Redhat people maintaining the build and module systems :) I just want to see if we can do things better. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> The question is how to provide a similar guarantee if a different way? > As a tool to aid distro reviewers, modversions has some value, but the > debug info parsing tools that have been mentioned in this thread seem > superior (not that I've tested them). On the other hand the big advantage of modversions is that it also verifies the checksum during runtime (module loading). In other words, I believe that any other solution should still generate some form of checksum/watermark which can be easily checked for compatibility on module load. It should not be hard to add to the DWARF based tools though. We'd just parse DWARF data instead of the C code. Regards, -Stanislav -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 9 Dec 2016 08:55:51 +0100 Stanislav Kozina <skozina@redhat.com> wrote: > >> The question is how to provide a similar guarantee if a different way? > > As a tool to aid distro reviewers, modversions has some value, but the > > debug info parsing tools that have been mentioned in this thread seem > > superior (not that I've tested them). > > On the other hand the big advantage of modversions is that it also > verifies the checksum during runtime (module loading). In other words, I > believe that any other solution should still generate some form of > checksum/watermark which can be easily checked for compatibility on > module load. > It should not be hard to add to the DWARF based tools though. We'd just > parse DWARF data instead of the C code. A runtime check is still done, with per-module vermagic which distros can change when they bump the ABI version. Is it really necessary to have more than that (i.e., per-symbol versioning)? Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> The question is how to provide a similar guarantee if a different way? >>> As a tool to aid distro reviewers, modversions has some value, but the >>> debug info parsing tools that have been mentioned in this thread seem >>> superior (not that I've tested them). >> On the other hand the big advantage of modversions is that it also >> verifies the checksum during runtime (module loading). In other words, I >> believe that any other solution should still generate some form of >> checksum/watermark which can be easily checked for compatibility on >> module load. >> It should not be hard to add to the DWARF based tools though. We'd just >> parse DWARF data instead of the C code. > A runtime check is still done, with per-module vermagic which distros > can change when they bump the ABI version. Is it really necessary to > have more than that (i.e., per-symbol versioning)? From my point of view, it is. We need to allow changing ABI for some modules while maintaining it for others. In fact I think that there should be version not only for every exported symbol (in the EXPORT_SYMBOL() sense), but also for every public type (in the sense of eg. structure defined in the public header file). Thanks, -Stanislav -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2016-12-09 at 13:33 +1000, Nicholas Piggin wrote: > > Well I simply tested the outcome. If you have: > > struct blah { > int x; > }; > int foo(struct blah *blah) > { > return blah->x; > } > EXPORT(foo); > > $ nm vmlinux | grep __crc_foo > 00000000a0cf13a0 A __crc_foo > > Now change to > > struct blah { > int y; > int x; > }; > > $ nm vmlinux | grep __crc_foo > 00000000a0cf13a0 A __crc_foo > > It just doesn't catch these things. I found the same when I just added your snippet to init/main.c. _But_ when I moved the struct into include/types.h (which happened to be included by init/main.c) then, with just x in the struct: $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && nm init/main.o | grep __crc_foo s#blah struct blah { int x ; } foo int foo ( s#blah * ) 000000000cd0312e A __crc_foo but adding y: $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && nm init/main.o | grep __crc_foo s#blah struct blah { int x ; int y ; } foo int foo ( s#blah * ) 00000000eda220c6 A __crc_foo So it does catch things in that case. With struct blah inline in main.c it was: $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && nm init/main.o | grep __crc_foo s#blah struct blah { UNKNOWN } foo int foo ( s#blah * ) 00000000a0cf13a0 A __crc_foo So I suppose it only cares about structs which are in headers, which I guess makes sense. I think it is working in at least one of the important cases. Ian. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 9 Dec 2016 15:36:04 +0100 Stanislav Kozina <skozina@redhat.com> wrote: > >>>> The question is how to provide a similar guarantee if a different way? > >>> As a tool to aid distro reviewers, modversions has some value, but the > >>> debug info parsing tools that have been mentioned in this thread seem > >>> superior (not that I've tested them). > >> On the other hand the big advantage of modversions is that it also > >> verifies the checksum during runtime (module loading). In other words, I > >> believe that any other solution should still generate some form of > >> checksum/watermark which can be easily checked for compatibility on > >> module load. > >> It should not be hard to add to the DWARF based tools though. We'd just > >> parse DWARF data instead of the C code. > > A runtime check is still done, with per-module vermagic which distros > > can change when they bump the ABI version. Is it really necessary to > > have more than that (i.e., per-symbol versioning)? > > From my point of view, it is. We need to allow changing ABI for some > modules while maintaining it for others. > In fact I think that there should be version not only for every exported > symbol (in the EXPORT_SYMBOL() sense), but also for every public type > (in the sense of eg. structure defined in the public header file). Well the distro can just append _v2, _v3 to the name of the function or type if it has to break compat for some reason. Would that be enough? Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote: > On Fri, 9 Dec 2016 15:36:04 +0100 > Stanislav Kozina <skozina@redhat.com> wrote: > > > >>>> The question is how to provide a similar guarantee if a different way? > > >>> As a tool to aid distro reviewers, modversions has some value, but the > > >>> debug info parsing tools that have been mentioned in this thread seem > > >>> superior (not that I've tested them). > > >> On the other hand the big advantage of modversions is that it also > > >> verifies the checksum during runtime (module loading). In other words, I > > >> believe that any other solution should still generate some form of > > >> checksum/watermark which can be easily checked for compatibility on > > >> module load. > > >> It should not be hard to add to the DWARF based tools though. We'd just > > >> parse DWARF data instead of the C code. > > > A runtime check is still done, with per-module vermagic which distros > > > can change when they bump the ABI version. Is it really necessary to > > > have more than that (i.e., per-symbol versioning)? > > > > From my point of view, it is. We need to allow changing ABI for some > > modules while maintaining it for others. > > In fact I think that there should be version not only for every exported > > symbol (in the EXPORT_SYMBOL() sense), but also for every public type > > (in the sense of eg. structure defined in the public header file). > > Well the distro can just append _v2, _v3 to the name of the function > or type if it has to break compat for some reason. Would that be enough? There are other ways that distros can work around when upstream "breaks" the ABI, sometimes they can rename functions, and others they can "preload" structures with padding in anticipation for when/if fields get added to them. But that's all up to the distros, no need for us to worry about that at all :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 09 Dec 2016 15:21:33 +0000 Ian Campbell <ijc@hellion.org.uk> wrote: > On Fri, 2016-12-09 at 13:33 +1000, Nicholas Piggin wrote: > > > > Well I simply tested the outcome. If you have: > > > > struct blah { > > int x; > > }; > > int foo(struct blah *blah) > > { > > return blah->x; > > } > > EXPORT(foo); > > > > $ nm vmlinux | grep __crc_foo > > 00000000a0cf13a0 A __crc_foo > > > > Now change to > > > > struct blah { > > int y; > > int x; > > }; > > > > $ nm vmlinux | grep __crc_foo > > 00000000a0cf13a0 A __crc_foo > > > > It just doesn't catch these things. > > I found the same when I just added your snippet to init/main.c. > > _But_ when I moved the struct into include/types.h (which happened to > be included by init/main.c) then, with just x in the struct: > > $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && nm init/main.o | grep __crc_foo > s#blah struct blah { int x ; } > foo int foo ( s#blah * ) > 000000000cd0312e A __crc_foo > > but adding y: > > $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && nm init/main.o | grep __crc_foo > s#blah struct blah { int x ; int y ; } > foo int foo ( s#blah * ) > 00000000eda220c6 A __crc_foo > > So it does catch things in that case. > > With struct blah inline in main.c it was: > > $ make -s init/main.{o,symtypes} && grep -E foo\|blah init/main.symtypes && nm init/main.o | grep __crc_foo > s#blah struct blah { UNKNOWN } > foo int foo ( s#blah * ) > 00000000a0cf13a0 A __crc_foo > > So I suppose it only cares about structs which are in headers, which I > guess makes sense. I think it is working in at least one of the > important cases. Aha thanks, well that's my mistake. Clever little bugger, isn't it? Okay it's not so useless as I first thought! That said, a dwarf based checker tool should be able to do as good a job (maybe a bit better because report is very informative and it may pick up compiler alignments or padding options). So I still think it's worth looking at those if we can remove modversions. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 09, 2016 at 01:50:41PM +1000, Nicholas Piggin wrote: > > > > We have plenty of customers with 10 year old drivers, where the expertise > > has long left the company. The engineers still around, recompile and make > > tweaks to get things working on the latest RHEL. Verify it passes testing > > and release it. Then they hope to not touch it again for a few years until > > the next RHEL comes along. > > > > Scary, huh? :-) > > Oh yeah my aim here is not to make distro or out of tree module vendors > life harder, actually the opposite. If it turns out modversions really is > the best approach, I'm not in a position to complain about its complexity > because we have Suse and Redhat people maintaining the build and module > systems :) I just want to see if we can do things better. Hi Nick, I think we are in pretty good agreement here. We can do better than modversions. On the flip side, I would hate to see modversions ripped out until we have an alternate path forward as it does get us by for now. :-) Cheers, Don -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Nicholas Piggin <npiggin@gmail.com> a écrit: [...] > That said, a dwarf based checker tool should be able to do as good a job > (maybe a bit better because report is very informative and it may pick up > compiler alignments or padding options). So, Nicholas was kind enough to send me the two Linux Kernel binaries that he built with the tiny little interface change that we were discussing earlier. Here is what the abidiff[1] tools says about that interface change: $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi vmlinux.abi2.abi Functions changes summary: 0 Removed, 1 Changed, 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 1 function with some indirect sub-type change: [C]'function int foo(blah*)' at memory.c:82:1 has some indirect sub-type changes: parameter 1 of type 'blah*' has sub-type changes: in pointed to type 'struct blah' at memory.c:78:1: type size changed from 32 to 64 bits 1 data member insertion: 'int blah::y', at offset 0 (in bits) at memory.c:79:1 1 data member change: 'int blah::x' offset changed from 0 to 32 (in bits) (by +32 bits) real 0m2.595s user 0m2.489s sys 0m0.108s $ I kept the timing information to give you an idea of the time it takes on a non-optimized build of abidiff. One could for instance want that types that are not defined in header files be kept out of the change report. In that case it's possible to write a little suppression specification file like this one: $ cat vmlinux.abignore [suppress_type] source_location_not_regexp = .*\\.h $ You can then pass that suppression file to the tool: $ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr vmlinux.abignore vmlinux.abi1.abi vmlinux.abi2.abi Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable real 0m2.574s user 0m2.473s sys 0m0.102s $ So this is the kind of interface change analysis tool we are working on at the moment. One could also imagine a tool that would compute a CRC that takes the very same suppression specification files into account, letting people to decide that some interface changes are OK. That CRC would thus be added to the special ELF sections we already have today. We could keep the modversion machinery, but with a greater dose of flexibility. Whenever modversion detects a change, abidiff would tell people what the change is exactly. What do you guys think? [1]: https://sourceware.org/libabigail/manual/abidiff.html Okay, the abidiff I used in this message is one that is not yet released. It's source code is in the dodji/kabidiff branch of the Git repository at https://sourceware.org/git/?p=libabigail.git;a=summary Cheers,
On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote: > Hello, > > Nicholas Piggin <npiggin@gmail.com> a écrit: > > [...] > > > That said, a dwarf based checker tool should be able to do as good a job > > (maybe a bit better because report is very informative and it may pick up > > compiler alignments or padding options). > > So, Nicholas was kind enough to send me the two Linux Kernel binaries > that he built with the tiny little interface change that we were > discussing earlier. Here is what the abidiff[1] tools says about that > interface change: > > $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi vmlinux.abi2.abi > Functions changes summary: 0 Removed, 1 Changed, 0 Added function > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable > > 1 function with some indirect sub-type change: > > [C]'function int foo(blah*)' at memory.c:82:1 has some indirect sub-type changes: > parameter 1 of type 'blah*' has sub-type changes: > in pointed to type 'struct blah' at memory.c:78:1: > type size changed from 32 to 64 bits > 1 data member insertion: > 'int blah::y', at offset 0 (in bits) at memory.c:79:1 > 1 data member change: > 'int blah::x' offset changed from 0 to 32 (in bits) (by +32 bits) > > > > real 0m2.595s > user 0m2.489s > sys 0m0.108s > $ > > I kept the timing information to give you an idea of the time it takes > on a non-optimized build of abidiff. > > One could for instance want that types that are not defined in header > files be kept out of the change report. In that case it's possible to > write a little suppression specification file like this one: > > $ cat vmlinux.abignore > [suppress_type] > source_location_not_regexp = .*\\.h > $ > > You can then pass that suppression file to the tool: > > $ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr vmlinux.abignore vmlinux.abi1.abi vmlinux.abi2.abi > Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable > > > real 0m2.574s > user 0m2.473s > sys 0m0.102s > $ > > So this is the kind of interface change analysis tool we are working on > at the moment. > > One could also imagine a tool that would compute a CRC that takes the > very same suppression specification files into account, letting people > to decide that some interface changes are OK. That CRC would thus be > added to the special ELF sections we already have today. We could keep > the modversion machinery, but with a greater dose of flexibility. > Whenever modversion detects a change, abidiff would tell people what the > change is exactly. > > What do you guys think? YES YES YES!!! Now I don't work on a distro anymore, but I would think that something like this would be really useful, pointing out exactly what changed is very important for distro maintainers to determine what they want to do (either fix up the abi change with strange hacks, or ignore it due to the change being in an area they don't care at all about, i.e. a random driver subsystem.) So yes, I think this is really good stuff. But if the distro maintainers correct me and think it's useless, then I need to revisit my view of exactly what they do for their customers :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 10 Dec 2016 13:41:03 +0100 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Fri, Dec 09, 2016 at 11:46:54PM +0100, Dodji Seketeli wrote: > > Hello, > > > > Nicholas Piggin <npiggin@gmail.com> a écrit: > > > > [...] > > > > > That said, a dwarf based checker tool should be able to do as good a job > > > (maybe a bit better because report is very informative and it may pick up > > > compiler alignments or padding options). > > > > So, Nicholas was kind enough to send me the two Linux Kernel binaries > > that he built with the tiny little interface change that we were > > discussing earlier. Here is what the abidiff[1] tools says about that > > interface change: > > > > $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi vmlinux.abi2.abi > > Functions changes summary: 0 Removed, 1 Changed, 0 Added function > > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable > > > > 1 function with some indirect sub-type change: > > > > [C]'function int foo(blah*)' at memory.c:82:1 has some indirect sub-type changes: > > parameter 1 of type 'blah*' has sub-type changes: > > in pointed to type 'struct blah' at memory.c:78:1: > > type size changed from 32 to 64 bits > > 1 data member insertion: > > 'int blah::y', at offset 0 (in bits) at memory.c:79:1 > > 1 data member change: > > 'int blah::x' offset changed from 0 to 32 (in bits) (by +32 bits) > > > > > > > > real 0m2.595s > > user 0m2.489s > > sys 0m0.108s > > $ > > > > I kept the timing information to give you an idea of the time it takes > > on a non-optimized build of abidiff. > > > > One could for instance want that types that are not defined in header > > files be kept out of the change report. In that case it's possible to > > write a little suppression specification file like this one: > > > > $ cat vmlinux.abignore > > [suppress_type] > > source_location_not_regexp = .*\\.h > > $ > > > > You can then pass that suppression file to the tool: > > > > $ ~/git/libabigail/kabidiff/build/tools/abidiff --suppr vmlinux.abignore vmlinux.abi1.abi vmlinux.abi2.abi > > Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function > > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable > > > > > > real 0m2.574s > > user 0m2.473s > > sys 0m0.102s > > $ > > > > So this is the kind of interface change analysis tool we are working on > > at the moment. > > > > One could also imagine a tool that would compute a CRC that takes the > > very same suppression specification files into account, letting people > > to decide that some interface changes are OK. That CRC would thus be > > added to the special ELF sections we already have today. We could keep > > the modversion machinery, but with a greater dose of flexibility. > > Whenever modversion detects a change, abidiff would tell people what the > > change is exactly. > > > > What do you guys think? > > YES YES YES!!! > > Now I don't work on a distro anymore, but I would think that something > like this would be really useful, pointing out exactly what changed is > very important for distro maintainers to determine what they want to do > (either fix up the abi change with strange hacks, or ignore it due to > the change being in an area they don't care at all about, i.e. a random > driver subsystem.) > > So yes, I think this is really good stuff. But if the distro > maintainers correct me and think it's useless, then I need to revisit my > view of exactly what they do for their customers :) Agree completely. BTW (for those who might be looking into these tools), we also have https://github.com/skozina/kabi-dw that Stanislav (cc'ed) mentioned earlier. It's true that the current modversions __crc_ matching infrastructure is "just" a symbol versioning system, and we could keep it and just populate it with something other than genksyms (e.g., a symbol version list provided by distros). But the starting point should be *no* versioning and simply using names to break linkage. Unless there's a compelling reason not to, symbols are simpler, easier, everyone knows how they work. The other question would be whether to pull a minimal tool into the kernel source or keep them out of tree (but possibly add some helper scripts etc). I guess we'll need to see what distros want. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2016-12-10 at 13:41 +0100, Greg Kroah-Hartman wrote: > Now I don't work on a distro anymore, but I would think that something > like this would be really useful, pointing out exactly what changed is > very important for distro maintainers to determine what they want to do The .symvers produced by the current scheme aren't completely useless from this PoV, although they aren't ideal since you need both before an d after trees and if the changes are large or far reaching the diff can get a bit unwieldy, so better tooling which points directly to the actual relevant change would be no bad thing. Ian. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> A runtime check is still done, with per-module vermagic which distros >>>> can change when they bump the ABI version. Is it really necessary to >>>> have more than that (i.e., per-symbol versioning)? >>> From my point of view, it is. We need to allow changing ABI for some >>> modules while maintaining it for others. >>> In fact I think that there should be version not only for every exported >>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type >>> (in the sense of eg. structure defined in the public header file). >> Well the distro can just append _v2, _v3 to the name of the function >> or type if it has to break compat for some reason. Would that be enough? > There are other ways that distros can work around when upstream "breaks" > the ABI, sometimes they can rename functions, and others they can > "preload" structures with padding in anticipation for when/if fields get > added to them. But that's all up to the distros, no need for us to > worry about that at all :) Currently, the ABI version (checksum) is stored outside of the actual code in the __ksymtab section. That means that the distributions can still apply upstream patches cleanly and only update the version checksum if these break ABI. With the _v2, _v3 suffixes (or similar solutions) we'd be effectively storing the ABI versions directly in the code and that would cause conflicts when pulling further patches from upstream. My view is that it would be than easier to maintain out-of-tree modversions (or similar tool) rather than to solve all these conflicts. Warm Regards, -Stanislav -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, >> That said, a dwarf based checker tool should be able to do as good a job >> (maybe a bit better because report is very informative and it may pick up >> compiler alignments or padding options). > So, Nicholas was kind enough to send me the two Linux Kernel binaries > that he built with the tiny little interface change that we were > discussing earlier. Here is what the abidiff[1] tools says about that > interface change: Thanks Nicholas and Dodji for this great example, for comparison I think it would be nice to share the example run with kabi-dw too. kabi-dw first dumps and unifies all type information into a set of text files, the unification takes a significant time. Then the two sets of these text files can be compared. An example run would look like: $ time ~/Code/kabi-dw/kabi-dw generate -o abi1 vmlinux.abi1 Generating symbol defs from vmlinux.abi1... real 0m29.057s user 0m13.929s sys 0m14.862s $ time ~/Code/kabi-dw/kabi-dw generate -o abi2 vmlinux.abi2 Generating symbol defs from vmlinux.abi2... real 0m29.134s user 0m13.961s sys 0m14.921s $ time ~/Code/kabi-dw/kabi-dw compare abi1 abi2 Changes detected in: home/npiggin/src/linux.vectors/mm/memory.c/struct--blah.txt Inserted: +0x0 int y; Shifted: -0x0 int x; +0x4 int x; real 0m0.176s user 0m0.135s sys 0m0.040s The size of the generated text files with all the relevant type information is as follows: $ du -hs abi1 16M abi1 $ find abi1 -type f | wc -l 3162 Warm regards, -Stanislav -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 12 Dec 2016 10:48:47 +0100 Stanislav Kozina <skozina@redhat.com> wrote: > >>>> A runtime check is still done, with per-module vermagic which distros > >>>> can change when they bump the ABI version. Is it really necessary to > >>>> have more than that (i.e., per-symbol versioning)? > >>> From my point of view, it is. We need to allow changing ABI for some > >>> modules while maintaining it for others. > >>> In fact I think that there should be version not only for every exported > >>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type > >>> (in the sense of eg. structure defined in the public header file). > >> Well the distro can just append _v2, _v3 to the name of the function > >> or type if it has to break compat for some reason. Would that be enough? > > There are other ways that distros can work around when upstream "breaks" > > the ABI, sometimes they can rename functions, and others they can > > "preload" structures with padding in anticipation for when/if fields get > > added to them. But that's all up to the distros, no need for us to > > worry about that at all :) > > Currently, the ABI version (checksum) is stored outside of the actual > code in the __ksymtab section. That means that the distributions can > still apply upstream patches cleanly and only update the version > checksum if these break ABI. > > With the _v2, _v3 suffixes (or similar solutions) we'd be effectively > storing the ABI versions directly in the code and that would cause > conflicts when pulling further patches from upstream. > > My view is that it would be than easier to maintain out-of-tree > modversions (or similar tool) rather than to solve all these conflicts. That kind of name clash should hardly be an issue, in comparison with the care it requires to merge fixes on top of a backport which has already changed ABI. It tends to be trivially fixable just by search/replace in the patchfile before applying, if nothing else. But you probably *want* to be flagged on those things and merge by hand anyway. Backporting alone I don't think can justify symbol versioning, but I'd like to hear from distro maintainers if any disagree. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dne 9.12.2016 v 23:46 Dodji Seketeli napsal(a): > Hello, > > Nicholas Piggin <npiggin@gmail.com> a écrit: > > [...] > >> That said, a dwarf based checker tool should be able to do as good a job >> (maybe a bit better because report is very informative and it may pick up >> compiler alignments or padding options). > > So, Nicholas was kind enough to send me the two Linux Kernel binaries > that he built with the tiny little interface change that we were > discussing earlier. Here is what the abidiff[1] tools says about that > interface change: > > $ time ~/git/libabigail/kabidiff/build/tools/abidiff vmlinux.abi1.abi vmlinux.abi2.abi > Functions changes summary: 0 Removed, 1 Changed, 0 Added function > Variables changes summary: 0 Removed, 0 Changed, 0 Added variable > > 1 function with some indirect sub-type change: > > [C]'function int foo(blah*)' at memory.c:82:1 has some indirect sub-type changes: > parameter 1 of type 'blah*' has sub-type changes: > in pointed to type 'struct blah' at memory.c:78:1: > type size changed from 32 to 64 bits > 1 data member insertion: > 'int blah::y', at offset 0 (in bits) at memory.c:79:1 > 1 data member change: > 'int blah::x' offset changed from 0 to 32 (in bits) (by +32 bits) For completeness, with a foo.symref file in the tree, genksyms would print foo.c: warning: foo: modversion changed because of changes in struct blah So there is some sort of diagnostics already. Does the abidiff tool handle the case when an exported symbol is moved between .c files? This is always a mess with genksyms, because the two .c files have different includes and thus the type expansion stops at different points. So typically the move needs to be reverted as a workaround. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Michal Marek <mmarek@suse.com> a écrit: [...] > Does the abidiff tool handle the case when an exported symbol is moved > between .c files? This is always a mess with genksyms, because the two > .c files have different includes and thus the type expansion stops at > different points. So typically the move needs to be reverted as a > workaround. Let's consider the function: 'void foo(struct S*);' If two ELF binaries contain a definition of that function foo which ELF symbol is exported, if the type struct S hasn't changed, and if the only difference between the ELF binaries is that foo was defined in the translation unit a.c in the first binary and in b.c in the second binary, then the comparison engine of libabigail (which is the library that abidiff uses) will consider the declarations of the two foo functions as being equal -- no matter what include file comes before the definition point of foo in a.c and b.c. If it does not, then it's a bug that ought to be fixed. If you feel that I haven't understood your question, then I guess a minimal standalone example (in the form of C source code) that illustrates your use case could be helpful to me. Thanks.
On 2016-12-14 09:58, Dodji Seketeli wrote: > Michal Marek <mmarek@suse.com> a écrit: > > [...] > >> Does the abidiff tool handle the case when an exported symbol is moved >> between .c files? This is always a mess with genksyms, because the two >> .c files have different includes and thus the type expansion stops at >> different points. So typically the move needs to be reverted as a >> workaround. > > Let's consider the function: > > 'void foo(struct S*);' > > If two ELF binaries contain a definition of that function foo which ELF > symbol is exported, if the type struct S hasn't changed, and if the only > difference between the ELF binaries is that foo was defined in the > translation unit a.c in the first binary and in b.c in the second > binary, then the comparison engine of libabigail (which is the library > that abidiff uses) will consider the declarations of the two foo > functions as being equal -- no matter what include file comes before the > definition point of foo in a.c and b.c. If it does not, then it's a bug > that ought to be fixed. > > If you feel that I haven't understood your question, then I guess a > minimal standalone example (in the form of C source code) that > illustrates your use case could be helpful to me. A minimal example would be t1.c: struct s1; struct s2 { int i; } struct s3 { struct s1 *ptr1; struct s2 *ptr2; } void foo(struct s3*); EXPORT_SYMBOL(foo); t2.c: struct s1 { int j; } struct s2; struct s3 { struct s1 *ptr1; struct s2 *ptr2; } void foo(struct s3*); EXPORT_SYMBOL(foo); genksyms expands this to void foo ( struct s3 { struct s1 { UNKNOWN } * ptr1 ; struct s2 { int i ; } * ptr2 ; } * ) or void foo ( struct s3 { struct s1 { int j ; } * ptr1 ; struct s2 { UNKNOWN } * ptr2 ; } * ) respectively. The types are the same, but their visibility in the different compilation units differs. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Michal Marek <mmarek@suse.com> a écrit: [...] > A minimal example would be > > t1.c: > struct s1; > struct s2 { > int i; > } > struct s3 { > struct s1 *ptr1; > struct s2 *ptr2; > } > void foo(struct s3*); > EXPORT_SYMBOL(foo); > > t2.c: > struct s1 { > int j; > } > struct s2; > struct s3 { > struct s1 *ptr1; > struct s2 *ptr2; > } > void foo(struct s3*); > EXPORT_SYMBOL(foo); > > genksyms expands this to > void foo ( struct s3 { struct s1 { UNKNOWN } * ptr1 ; struct s2 { int i ; } * ptr2 ; } * ) > > or > > void foo ( struct s3 { struct s1 { int j ; } * ptr1 ; struct s2 { UNKNOWN } * ptr2 ; } * ) > respectively. Thanks, I have built an independant test case from this: $ cat t1.c struct s1; struct s2 { int i; }; struct s3 { struct s1 *ptr1; struct s2 *ptr2; }; void foo(struct s3*); $ cat t2.c struct s1 { int j; }; struct s2; struct s3 { struct s1 *ptr1; struct s2 *ptr2; }; void foo(struct s3*); $ gcc -g -c t1.c $ gcc -g -c t2.c $ abidiff t1.o t2.o $ So, as you see here, abidiff considers t1.o and t2.o has having the same ABI, so it considers the two foo functions to be equivalent. > The types are the same, but their visibility in the different > compilation units differs. I see, for genksyms, the order of declarations matters, especially when forward declarations are involved. Libabigail does a "whole binary" analysis of types. So, consider the point of use of the type 'struct s1*'. Even if 'struct s' is just forward-declared at that point, the declaration of struct s1 is "resolved" to its definition. Even if the definition comes later in the binary. In other words, if struct s1 is defined in the binary, you'll never have that "struct s1 {UNKNOWN} *ptr1;" that you see in genksyms's representation. Cheers,
On 2016-12-14 10:15, Michal Marek wrote: > A minimal example would be > > t1.c: > struct s1; > struct s2 { > int i; > } > struct s3 { > struct s1 *ptr1; > struct s2 *ptr2; > } > void foo(struct s3*); > EXPORT_SYMBOL(foo); > > t2.c: > struct s1 { > int j; > } > struct s2; > struct s3 { > struct s1 *ptr1; > struct s2 *ptr2; > } > void foo(struct s3*); > EXPORT_SYMBOL(foo); Note that the above, if passed to genksyms verbatim, would result in genksyms treating all the types as internal. Here is a complete example including linemarkers: $ cat t1.i # 1 "t1.c" # 1 "<built-in>" # 1 "<command-line>" # 1 "t1.c" # 1 "t1.h" 1 # 1 "t.h" 1 struct s1; struct s2; struct s3 { struct s1 *ptr1; struct s2 *ptr2; }; # 2 "t1.h" 2 struct s2 { int i; }; # 2 "t1.c" 2 void foo(struct s3 *s) { } EXPORT_SYMBOL(foo); $ cat t2.i # 1 "t2.c" # 1 "<built-in>" # 1 "<command-line>" # 1 "t2.c" # 1 "t2.h" 1 # 1 "t.h" 1 struct s1; struct s2; struct s3 { struct s1 *ptr1; struct s2 *ptr2; }; # 2 "t2.h" 2 struct s1 { int j; }; # 2 "t2.c" 2 void foo(struct s3 *s) { } EXPORT_SYMBOL(foo); $ ./scripts/genksyms/genksyms -D <t1.i Export foo == <void foo ( struct s3 { struct s1 { UNKNOWN } * ptr1 ; struct s2 { int i ; } * ptr2 ; } * ) > __crc_foo = 0xf731cef8 ; $ ./scripts/genksyms/genksyms -D <t2.i Export foo == <void foo ( struct s3 { struct s1 { int j ; } * ptr1 ; struct s2 { UNKNOWN } * ptr2 ; } * ) > __crc_foo = 0xc925dae5 ; Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-12-14 10:36, Dodji Seketeli wrote: > Michal Marek <mmarek@suse.com> a écrit: > > [...] > >> A minimal example would be >> >> t1.c: >> struct s1; >> struct s2 { >> int i; >> } >> struct s3 { >> struct s1 *ptr1; >> struct s2 *ptr2; >> } >> void foo(struct s3*); >> EXPORT_SYMBOL(foo); >> >> t2.c: >> struct s1 { >> int j; >> } >> struct s2; >> struct s3 { >> struct s1 *ptr1; >> struct s2 *ptr2; >> } >> void foo(struct s3*); >> EXPORT_SYMBOL(foo); >> >> genksyms expands this to >> void foo ( struct s3 { struct s1 { UNKNOWN } * ptr1 ; struct s2 { int i ; } * ptr2 ; } * ) >> >> or >> >> void foo ( struct s3 { struct s1 { int j ; } * ptr1 ; struct s2 { UNKNOWN } * ptr2 ; } * ) >> respectively. > > Thanks, I have built an independant test case from this: > > $ cat t1.c > struct s1; > struct s2 { > int i; > }; > struct s3 { > struct s1 *ptr1; > struct s2 *ptr2; > }; > void foo(struct s3*); > $ cat t2.c > struct s1 { > int j; > }; > struct s2; > struct s3 { > struct s1 *ptr1; > struct s2 *ptr2; > }; > void foo(struct s3*); > $ gcc -g -c t1.c > $ gcc -g -c t2.c > $ abidiff t1.o t2.o > $ > > So, as you see here, abidiff considers t1.o and t2.o has having the same > ABI, so it considers the two foo functions to be equivalent. Wow. That sounds too good to be true. >> The types are the same, but their visibility in the different >> compilation units differs. > > I see, for genksyms, the order of declarations matters, especially when > forward declarations are involved. > > Libabigail does a "whole binary" analysis of types. > > So, consider the point of use of the type 'struct s1*'. Even if 'struct > s' is just forward-declared at that point, the declaration of struct s1 > is "resolved" to its definition. Even if the definition comes later in > the binary. But there isn't any definition of struct s1 in t1.o. Does abidiff "steal" the definition from the other object file? That would be legitimate, I'm just curious. Thanks, Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dodji Seketeli <dodji@seketeli.org> a écrit:
Grr, I did paste the wrong content of t1.c and t2.c in my last message sorry.
Here are the correct ones:
$ cat t1.c
struct s1;
struct s2 {
int i;
};
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
};
void foo(struct s3* s __attribute__((unused)))
{
}
$ cat t2.c
struct s1 {
int j;
};
struct s2;
struct s3 {
struct s1 *ptr1;
struct s2 *ptr2;
};
void foo(struct s3* s __attribute__((unused)))
{
}
$ gcc -g -c t1.c
$ gcc -g -c t2.c
$ abidiff t1.o t2.o
$
The rest of my previous message still applies :-)
> So, as you see here, abidiff considers t1.o and t2.o has having the same
> ABI, so it considers the two foo functions to be equivalent.
>
>> The types are the same, but their visibility in the different
>> compilation units differs.
>
> I see, for genksyms, the order of declarations matters, especially when
> forward declarations are involved.
>
> Libabigail does a "whole binary" analysis of types.
>
> So, consider the point of use of the type 'struct s1*'. Even if 'struct
> s' is just forward-declared at that point, the declaration of struct s1
> is "resolved" to its definition. Even if the definition comes later in
> the binary.
>
> In other words, if struct s1 is defined in the binary, you'll never have
> that "struct s1 {UNKNOWN} *ptr1;" that you see in genksyms's
> representation.
Thanks.
Michal Marek <mmarek@suse.com> a écrit: >> Libabigail does a "whole binary" analysis of types. >> >> So, consider the point of use of the type 'struct s1*'. Even if 'struct >> s' is just forward-declared at that point, the declaration of struct s1 >> is "resolved" to its definition. Even if the definition comes later in >> the binary. > > But there isn't any definition of struct s1 in t1.o. Does abidiff > "steal" the definition from the other object file? That would be > legitimate, I'm just curious. If there is another translation unit in the *same* binary that defines struct s1, then yes, it's "stolen", as you say. But if in the entire binary, struct s1 is just declared (not defined), then it'll compare equal to any struct s1 that is defined in the *second* binary. Cheers,
On 2016-12-14 11:02, Dodji Seketeli wrote: > Michal Marek <mmarek@suse.com> a écrit: > >>> Libabigail does a "whole binary" analysis of types. >>> >>> So, consider the point of use of the type 'struct s1*'. Even if 'struct >>> s' is just forward-declared at that point, the declaration of struct s1 >>> is "resolved" to its definition. Even if the definition comes later in >>> the binary. >> >> But there isn't any definition of struct s1 in t1.o. Does abidiff >> "steal" the definition from the other object file? That would be >> legitimate, I'm just curious. > > If there is another translation unit in the *same* binary that defines > struct s1, then yes, it's "stolen", as you say. > > But if in the entire binary, struct s1 is just declared (not defined), > then it'll compare equal to any struct s1 that is defined in the > *second* binary. That makes sense, thanks. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09.12.2016 17:03, Greg Kroah-Hartman wrote: > On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote: >> On Fri, 9 Dec 2016 15:36:04 +0100 >> Stanislav Kozina <skozina@redhat.com> wrote: >> >>>>>>> The question is how to provide a similar guarantee if a different way? >>>>>> As a tool to aid distro reviewers, modversions has some value, but the >>>>>> debug info parsing tools that have been mentioned in this thread seem >>>>>> superior (not that I've tested them). >>>>> On the other hand the big advantage of modversions is that it also >>>>> verifies the checksum during runtime (module loading). In other words, I >>>>> believe that any other solution should still generate some form of >>>>> checksum/watermark which can be easily checked for compatibility on >>>>> module load. >>>>> It should not be hard to add to the DWARF based tools though. We'd just >>>>> parse DWARF data instead of the C code. >>>> A runtime check is still done, with per-module vermagic which distros >>>> can change when they bump the ABI version. Is it really necessary to >>>> have more than that (i.e., per-symbol versioning)? >>> >>> From my point of view, it is. We need to allow changing ABI for some >>> modules while maintaining it for others. >>> In fact I think that there should be version not only for every exported >>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type >>> (in the sense of eg. structure defined in the public header file). >> >> Well the distro can just append _v2, _v3 to the name of the function >> or type if it has to break compat for some reason. Would that be enough? > > There are other ways that distros can work around when upstream "breaks" > the ABI, sometimes they can rename functions, and others they can > "preload" structures with padding in anticipation for when/if fields get > added to them. But that's all up to the distros, no need for us to > worry about that at all :) The _v2 and _v3 functions are probably the ones that also get used by future backports in the distro kernel itself and are probably the reason for the ABI change in the first place. Thus going down this route will basically require distros to touch every future backport patch and will in general generate a big mess internally. I think it is important to keep versioning information outside of the source code. Some kind of modversions will still be required, but distros should be able to decide if they put in some kind of checksum or a string, what suites them most. Thanks, Hannes (who is still impressed by the genksyms tools) -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 14 Dec 2016 15:04:36 +0100 Hannes Frederic Sowa <hannes@redhat.com> wrote: > On 09.12.2016 17:03, Greg Kroah-Hartman wrote: > > On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote: > >> On Fri, 9 Dec 2016 15:36:04 +0100 > >> Stanislav Kozina <skozina@redhat.com> wrote: > >> > >>>>>>> The question is how to provide a similar guarantee if a different way? > >>>>>> As a tool to aid distro reviewers, modversions has some value, but the > >>>>>> debug info parsing tools that have been mentioned in this thread seem > >>>>>> superior (not that I've tested them). > >>>>> On the other hand the big advantage of modversions is that it also > >>>>> verifies the checksum during runtime (module loading). In other words, I > >>>>> believe that any other solution should still generate some form of > >>>>> checksum/watermark which can be easily checked for compatibility on > >>>>> module load. > >>>>> It should not be hard to add to the DWARF based tools though. We'd just > >>>>> parse DWARF data instead of the C code. > >>>> A runtime check is still done, with per-module vermagic which distros > >>>> can change when they bump the ABI version. Is it really necessary to > >>>> have more than that (i.e., per-symbol versioning)? > >>> > >>> From my point of view, it is. We need to allow changing ABI for some > >>> modules while maintaining it for others. > >>> In fact I think that there should be version not only for every exported > >>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type > >>> (in the sense of eg. structure defined in the public header file). > >> > >> Well the distro can just append _v2, _v3 to the name of the function > >> or type if it has to break compat for some reason. Would that be enough? > > > > There are other ways that distros can work around when upstream "breaks" > > the ABI, sometimes they can rename functions, and others they can > > "preload" structures with padding in anticipation for when/if fields get > > added to them. But that's all up to the distros, no need for us to > > worry about that at all :) > > The _v2 and _v3 functions are probably the ones that also get used by > future backports in the distro kernel itself and are probably the reason > for the ABI change in the first place. Thus going down this route will > basically require distros to touch every future backport patch and will > in general generate a big mess internally. What kind of big mess? You have to check the logic of each backport even if it does apply cleanly, so the added overhead of the name change should be relatively tiny, no? > > I think it is important to keep versioning information outside of the > source code. Some kind of modversions will still be required, but > distros should be able to decide if they put in some kind of checksum or > a string, what suites them most. The module crc symbols are just an integer that requires a match, so it could easily be populated by a list that the distro keeps, rather than by genksyms. Most of the complexity is on the build side, so that would still be an improvement for the kernel. So we *could* do this if the distros need it. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15.12.2016 03:06, Nicholas Piggin wrote: > On Wed, 14 Dec 2016 15:04:36 +0100 > Hannes Frederic Sowa <hannes@redhat.com> wrote: > >> On 09.12.2016 17:03, Greg Kroah-Hartman wrote: >>> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote: >>>> On Fri, 9 Dec 2016 15:36:04 +0100 >>>> Stanislav Kozina <skozina@redhat.com> wrote: >>>> >>>>>>>>> The question is how to provide a similar guarantee if a different way? >>>>>>>> As a tool to aid distro reviewers, modversions has some value, but the >>>>>>>> debug info parsing tools that have been mentioned in this thread seem >>>>>>>> superior (not that I've tested them). >>>>>>> On the other hand the big advantage of modversions is that it also >>>>>>> verifies the checksum during runtime (module loading). In other words, I >>>>>>> believe that any other solution should still generate some form of >>>>>>> checksum/watermark which can be easily checked for compatibility on >>>>>>> module load. >>>>>>> It should not be hard to add to the DWARF based tools though. We'd just >>>>>>> parse DWARF data instead of the C code. >>>>>> A runtime check is still done, with per-module vermagic which distros >>>>>> can change when they bump the ABI version. Is it really necessary to >>>>>> have more than that (i.e., per-symbol versioning)? >>>>> >>>>> From my point of view, it is. We need to allow changing ABI for some >>>>> modules while maintaining it for others. >>>>> In fact I think that there should be version not only for every exported >>>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type >>>>> (in the sense of eg. structure defined in the public header file). >>>> >>>> Well the distro can just append _v2, _v3 to the name of the function >>>> or type if it has to break compat for some reason. Would that be enough? >>> >>> There are other ways that distros can work around when upstream "breaks" >>> the ABI, sometimes they can rename functions, and others they can >>> "preload" structures with padding in anticipation for when/if fields get >>> added to them. But that's all up to the distros, no need for us to >>> worry about that at all :) >> >> The _v2 and _v3 functions are probably the ones that also get used by >> future backports in the distro kernel itself and are probably the reason >> for the ABI change in the first place. Thus going down this route will >> basically require distros to touch every future backport patch and will >> in general generate a big mess internally. > > What kind of big mess? You have to check the logic of each backport even > if it does apply cleanly, so the added overhead of the name change should > be relatively tiny, no? Basically single patches are backported in huge series. Reviewing each single patch also definitely makes sense, a review of the series as a whole is much more worthwhile because it focuses more on logic. The patches themselves are checked by individual robots or humans against merge conflict introduced mistakes which ring alarm bells for people to look more closely during review. Merge conflicts introduced mistakes definitely can happen because developers/backporters lose the focus from the actual logic but deal with shifting lines around or just fixing up postfixes to function names. We still try to align the kernel as much as possible with upstream, because most developers can't really hold the differences between upstream and the internal functions in their heads (is this function RMW safe in this version but not that kernel version...). Anyway, I don't think we will at any time have multiple versions of a function exported to 3rd party kernel modules. The headaches are just too big. Basically we would have to version structs and not functions (this is our bigger problem), thus exporting new versions of functions don't really help at all. Having multiple versions of structs really scares me. ;) We already pad structs to allow for additional struct members to be added, which helps a lot. If versioning of function symbols would be an issue we probably would have switched to ELF function versioning (like glibc does it) long time ago. >> I think it is important to keep versioning information outside of the >> source code. Some kind of modversions will still be required, but >> distros should be able to decide if they put in some kind of checksum or >> a string, what suites them most. > > The module crc symbols are just an integer that requires a match, so it > could easily be populated by a list that the distro keeps, rather than > by genksyms. Most of the complexity is on the build side, so that would > still be an improvement for the kernel. So we *could* do this if the > distros need it. Like Don also already said, genksyms already did a pretty good job so far. We are right now working with Dodji to come up with a way to replace genksyms, in case people really want to have very specific control about what causes the symbol version to be changed. Also I wonder what Ben's opinion on this is.. As I understood that he wants to maintain a super-long-term stable kernel with kabi guarantees. Note, what we want is to weaken the check for kabi, by excluding parts of the struct from genksyms with libabigail. For Red Hat genksyms is too strict in the checks. Bye, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 15 Dec 2016 12:19:02 +0100 Hannes Frederic Sowa <hannes@redhat.com> wrote: > On 15.12.2016 03:06, Nicholas Piggin wrote: > > On Wed, 14 Dec 2016 15:04:36 +0100 > > Hannes Frederic Sowa <hannes@redhat.com> wrote: > > > >> On 09.12.2016 17:03, Greg Kroah-Hartman wrote: > >>> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote: > >>>> On Fri, 9 Dec 2016 15:36:04 +0100 > >>>> Stanislav Kozina <skozina@redhat.com> wrote: > >>>> > >>>>>>>>> The question is how to provide a similar guarantee if a different way? > >>>>>>>> As a tool to aid distro reviewers, modversions has some value, but the > >>>>>>>> debug info parsing tools that have been mentioned in this thread seem > >>>>>>>> superior (not that I've tested them). > >>>>>>> On the other hand the big advantage of modversions is that it also > >>>>>>> verifies the checksum during runtime (module loading). In other words, I > >>>>>>> believe that any other solution should still generate some form of > >>>>>>> checksum/watermark which can be easily checked for compatibility on > >>>>>>> module load. > >>>>>>> It should not be hard to add to the DWARF based tools though. We'd just > >>>>>>> parse DWARF data instead of the C code. > >>>>>> A runtime check is still done, with per-module vermagic which distros > >>>>>> can change when they bump the ABI version. Is it really necessary to > >>>>>> have more than that (i.e., per-symbol versioning)? > >>>>> > >>>>> From my point of view, it is. We need to allow changing ABI for some > >>>>> modules while maintaining it for others. > >>>>> In fact I think that there should be version not only for every exported > >>>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type > >>>>> (in the sense of eg. structure defined in the public header file). > >>>> > >>>> Well the distro can just append _v2, _v3 to the name of the function > >>>> or type if it has to break compat for some reason. Would that be enough? > >>> > >>> There are other ways that distros can work around when upstream "breaks" > >>> the ABI, sometimes they can rename functions, and others they can > >>> "preload" structures with padding in anticipation for when/if fields get > >>> added to them. But that's all up to the distros, no need for us to > >>> worry about that at all :) > >> > >> The _v2 and _v3 functions are probably the ones that also get used by > >> future backports in the distro kernel itself and are probably the reason > >> for the ABI change in the first place. Thus going down this route will > >> basically require distros to touch every future backport patch and will > >> in general generate a big mess internally. > > > > What kind of big mess? You have to check the logic of each backport even > > if it does apply cleanly, so the added overhead of the name change should > > be relatively tiny, no? > > Basically single patches are backported in huge series. Reviewing each > single patch also definitely makes sense, a review of the series as a > whole is much more worthwhile because it focuses more on logic. > > The patches themselves are checked by individual robots or humans > against merge conflict introduced mistakes which ring alarm bells for > people to look more closely during review. > > Merge conflicts introduced mistakes definitely can happen because > developers/backporters lose the focus from the actual logic but deal > with shifting lines around or just fixing up postfixes to function names. > > We still try to align the kernel as much as possible with upstream, > because most developers can't really hold the differences between > upstream and the internal functions in their heads (is this function RMW > safe in this version but not that kernel version...). I agree with all this, but in the case of a function rename, you can automate it all with scripts if that's what you want. When you have your list of exported symbols with non-zero version number, then you can script that __abivXXX into the changeset applying process, or alternatively apply the rename after your patches are applied, or use the c preprocessor to define names to something else. > > Anyway, I don't think we will at any time have multiple versions of a > function exported to 3rd party kernel modules. The headaches are just > too big. Basically we would have to version structs and not functions > (this is our bigger problem), thus exporting new versions of functions > don't really help at all. Having multiple versions of structs really > scares me. ;) > > We already pad structs to allow for additional struct members to be > added, which helps a lot. > > If versioning of function symbols would be an issue we probably would > have switched to ELF function versioning (like glibc does it) long time ago. > > >> I think it is important to keep versioning information outside of the > >> source code. Some kind of modversions will still be required, but > >> distros should be able to decide if they put in some kind of checksum or > >> a string, what suites them most. > > > > The module crc symbols are just an integer that requires a match, so it > > could easily be populated by a list that the distro keeps, rather than > > by genksyms. Most of the complexity is on the build side, so that would > > still be an improvement for the kernel. So we *could* do this if the > > distros need it. > > Like Don also already said, genksyms already did a pretty good job so > far. We are right now working with Dodji to come up with a way to > replace genksyms, in case people really want to have very specific > control about what causes the symbol version to be changed. Yeah it's great work, so is Stanislav's checker. I wouldn't mind having a kernel-centric checker tool merged in the kernel if it is small, maintained, and does a sufficient job for distros. > Also I wonder what Ben's opinion on this is.. As I understood that he > wants to maintain a super-long-term stable kernel with kabi guarantees. > > Note, what we want is to weaken the check for kabi, by excluding parts > of the struct from genksyms with libabigail. For Red Hat genksyms is too > strict in the checks. Sure, that makes sense. So if I understand where we are, moving the ABI compatibility checking to one of these tools looks possible. What to do when we have an ABI change is not settled, but feeding version numbers explicitly into modversions is an option that would be close to what distros do today. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15.12.2016 13:03, Nicholas Piggin wrote: > On Thu, 15 Dec 2016 12:19:02 +0100 > Hannes Frederic Sowa <hannes@redhat.com> wrote: > >> On 15.12.2016 03:06, Nicholas Piggin wrote: >>> On Wed, 14 Dec 2016 15:04:36 +0100 >>> Hannes Frederic Sowa <hannes@redhat.com> wrote: >>> >>>> On 09.12.2016 17:03, Greg Kroah-Hartman wrote: >>>>> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote: >>>>>> On Fri, 9 Dec 2016 15:36:04 +0100 >>>>>> Stanislav Kozina <skozina@redhat.com> wrote: >>>>>> >>>>>>>>>>> The question is how to provide a similar guarantee if a different way? >>>>>>>>>> As a tool to aid distro reviewers, modversions has some value, but the >>>>>>>>>> debug info parsing tools that have been mentioned in this thread seem >>>>>>>>>> superior (not that I've tested them). >>>>>>>>> On the other hand the big advantage of modversions is that it also >>>>>>>>> verifies the checksum during runtime (module loading). In other words, I >>>>>>>>> believe that any other solution should still generate some form of >>>>>>>>> checksum/watermark which can be easily checked for compatibility on >>>>>>>>> module load. >>>>>>>>> It should not be hard to add to the DWARF based tools though. We'd just >>>>>>>>> parse DWARF data instead of the C code. >>>>>>>> A runtime check is still done, with per-module vermagic which distros >>>>>>>> can change when they bump the ABI version. Is it really necessary to >>>>>>>> have more than that (i.e., per-symbol versioning)? >>>>>>> >>>>>>> From my point of view, it is. We need to allow changing ABI for some >>>>>>> modules while maintaining it for others. >>>>>>> In fact I think that there should be version not only for every exported >>>>>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type >>>>>>> (in the sense of eg. structure defined in the public header file). >>>>>> >>>>>> Well the distro can just append _v2, _v3 to the name of the function >>>>>> or type if it has to break compat for some reason. Would that be enough? >>>>> >>>>> There are other ways that distros can work around when upstream "breaks" >>>>> the ABI, sometimes they can rename functions, and others they can >>>>> "preload" structures with padding in anticipation for when/if fields get >>>>> added to them. But that's all up to the distros, no need for us to >>>>> worry about that at all :) >>>> >>>> The _v2 and _v3 functions are probably the ones that also get used by >>>> future backports in the distro kernel itself and are probably the reason >>>> for the ABI change in the first place. Thus going down this route will >>>> basically require distros to touch every future backport patch and will >>>> in general generate a big mess internally. >>> >>> What kind of big mess? You have to check the logic of each backport even >>> if it does apply cleanly, so the added overhead of the name change should >>> be relatively tiny, no? >> >> Basically single patches are backported in huge series. Reviewing each >> single patch also definitely makes sense, a review of the series as a >> whole is much more worthwhile because it focuses more on logic. >> >> The patches themselves are checked by individual robots or humans >> against merge conflict introduced mistakes which ring alarm bells for >> people to look more closely during review. >> >> Merge conflicts introduced mistakes definitely can happen because >> developers/backporters lose the focus from the actual logic but deal >> with shifting lines around or just fixing up postfixes to function names. >> >> We still try to align the kernel as much as possible with upstream, >> because most developers can't really hold the differences between >> upstream and the internal functions in their heads (is this function RMW >> safe in this version but not that kernel version...). > > I agree with all this, but in the case of a function rename, you can > automate it all with scripts if that's what you want. > > When you have your list of exported symbols with non-zero version number, > then you can script that __abivXXX into the changeset applying process, > or alternatively apply the rename after your patches are applied, or > use the c preprocessor to define names to something else. Yes, probably one could come up with coccinelle patches to do this, preprocessing/string matching could have false positives. But as I wrote above, we need one stable ABI and not multiple for our particular kernels, so it seems like a lot of overhead to rename particular functions internally all the time to make them inaccessible for external modules. >> Anyway, I don't think we will at any time have multiple versions of a >> function exported to 3rd party kernel modules. The headaches are just >> too big. Basically we would have to version structs and not functions >> (this is our bigger problem), thus exporting new versions of functions >> don't really help at all. Having multiple versions of structs really >> scares me. ;) >> >> We already pad structs to allow for additional struct members to be >> added, which helps a lot. >> >> If versioning of function symbols would be an issue we probably would >> have switched to ELF function versioning (like glibc does it) long time ago. >> >>>> I think it is important to keep versioning information outside of the >>>> source code. Some kind of modversions will still be required, but >>>> distros should be able to decide if they put in some kind of checksum or >>>> a string, what suites them most. >>> >>> The module crc symbols are just an integer that requires a match, so it >>> could easily be populated by a list that the distro keeps, rather than >>> by genksyms. Most of the complexity is on the build side, so that would >>> still be an improvement for the kernel. So we *could* do this if the >>> distros need it. >> >> Like Don also already said, genksyms already did a pretty good job so >> far. We are right now working with Dodji to come up with a way to >> replace genksyms, in case people really want to have very specific >> control about what causes the symbol version to be changed. > > Yeah it's great work, so is Stanislav's checker. I wouldn't mind having > a kernel-centric checker tool merged in the kernel if it is small, > maintained, and does a sufficient job for distros. Yes, I think this needs more experimentation and thought right now before we can make a decision. >> Also I wonder what Ben's opinion on this is.. As I understood that he >> wants to maintain a super-long-term stable kernel with kabi guarantees. >> >> Note, what we want is to weaken the check for kabi, by excluding parts >> of the struct from genksyms with libabigail. For Red Hat genksyms is too >> strict in the checks. > > Sure, that makes sense. > > So if I understand where we are, moving the ABI compatibility checking > to one of these tools looks possible. What to do when we have an ABI change > is not settled, but feeding version numbers explicitly into modversions > is an option that would be close to what distros do today. Agreed! Thanks also, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Yeah it's great work, so is Stanislav's checker. I wouldn't mind having > a kernel-centric checker tool merged in the kernel if it is small, > maintained, and does a sufficient job for distros. I'd be very happy to see the resulting tool in the kernel tree, as it needs to be kept in sync with any significant changes done to the kernel layout in the future. It's not important if the result is based off libabigail or kabi-dw code, I'm sure both can be adjusted to serve the needs. kabi-dw tends to be smaller and still rather proof-of-concept, libabigail is definitely more mature. The only real difference is C vs. C++ code. > So if I understand where we are, moving the ABI compatibility checking > to one of these tools looks possible. What to do when we have an ABI change > is not settled, but feeding version numbers explicitly into modversions > is an option that would be close to what distros do today. Sounds great! Thank you! -Stanislav -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 15 Dec 2016 14:15:31 +0100 Hannes Frederic Sowa <hannes@redhat.com> wrote: > On 15.12.2016 13:03, Nicholas Piggin wrote: > > On Thu, 15 Dec 2016 12:19:02 +0100 > > Hannes Frederic Sowa <hannes@redhat.com> wrote: > > > >> On 15.12.2016 03:06, Nicholas Piggin wrote: > >>> On Wed, 14 Dec 2016 15:04:36 +0100 > >>> Hannes Frederic Sowa <hannes@redhat.com> wrote: > >>> > >>>> On 09.12.2016 17:03, Greg Kroah-Hartman wrote: > >>>>> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote: > >>>>>> On Fri, 9 Dec 2016 15:36:04 +0100 > >>>>>> Stanislav Kozina <skozina@redhat.com> wrote: > >>>>>> > >>>>>>>>>>> The question is how to provide a similar guarantee if a different way? > >>>>>>>>>> As a tool to aid distro reviewers, modversions has some value, but the > >>>>>>>>>> debug info parsing tools that have been mentioned in this thread seem > >>>>>>>>>> superior (not that I've tested them). > >>>>>>>>> On the other hand the big advantage of modversions is that it also > >>>>>>>>> verifies the checksum during runtime (module loading). In other words, I > >>>>>>>>> believe that any other solution should still generate some form of > >>>>>>>>> checksum/watermark which can be easily checked for compatibility on > >>>>>>>>> module load. > >>>>>>>>> It should not be hard to add to the DWARF based tools though. We'd just > >>>>>>>>> parse DWARF data instead of the C code. > >>>>>>>> A runtime check is still done, with per-module vermagic which distros > >>>>>>>> can change when they bump the ABI version. Is it really necessary to > >>>>>>>> have more than that (i.e., per-symbol versioning)? > >>>>>>> > >>>>>>> From my point of view, it is. We need to allow changing ABI for some > >>>>>>> modules while maintaining it for others. > >>>>>>> In fact I think that there should be version not only for every exported > >>>>>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type > >>>>>>> (in the sense of eg. structure defined in the public header file). > >>>>>> > >>>>>> Well the distro can just append _v2, _v3 to the name of the function > >>>>>> or type if it has to break compat for some reason. Would that be enough? > >>>>> > >>>>> There are other ways that distros can work around when upstream "breaks" > >>>>> the ABI, sometimes they can rename functions, and others they can > >>>>> "preload" structures with padding in anticipation for when/if fields get > >>>>> added to them. But that's all up to the distros, no need for us to > >>>>> worry about that at all :) > >>>> > >>>> The _v2 and _v3 functions are probably the ones that also get used by > >>>> future backports in the distro kernel itself and are probably the reason > >>>> for the ABI change in the first place. Thus going down this route will > >>>> basically require distros to touch every future backport patch and will > >>>> in general generate a big mess internally. > >>> > >>> What kind of big mess? You have to check the logic of each backport even > >>> if it does apply cleanly, so the added overhead of the name change should > >>> be relatively tiny, no? > >> > >> Basically single patches are backported in huge series. Reviewing each > >> single patch also definitely makes sense, a review of the series as a > >> whole is much more worthwhile because it focuses more on logic. > >> > >> The patches themselves are checked by individual robots or humans > >> against merge conflict introduced mistakes which ring alarm bells for > >> people to look more closely during review. > >> > >> Merge conflicts introduced mistakes definitely can happen because > >> developers/backporters lose the focus from the actual logic but deal > >> with shifting lines around or just fixing up postfixes to function names. > >> > >> We still try to align the kernel as much as possible with upstream, > >> because most developers can't really hold the differences between > >> upstream and the internal functions in their heads (is this function RMW > >> safe in this version but not that kernel version...). > > > > I agree with all this, but in the case of a function rename, you can > > automate it all with scripts if that's what you want. > > > > When you have your list of exported symbols with non-zero version number, > > then you can script that __abivXXX into the changeset applying process, > > or alternatively apply the rename after your patches are applied, or > > use the c preprocessor to define names to something else. > > Yes, probably one could come up with coccinelle patches to do this, > preprocessing/string matching could have false positives. But as I wrote > above, we need one stable ABI and not multiple for our particular > kernels, so it seems like a lot of overhead to rename particular > functions internally all the time to make them inaccessible for external > modules. I can't be sure until it's implemented in a workflow that distros are happy with of course, but I just don't see why it would be a lot of overhead. Particularly if you just scripted everything. How frequently do symbols become incompatible within an ostensibly ABI- stable release? > >> Like Don also already said, genksyms already did a pretty good job so > >> far. We are right now working with Dodji to come up with a way to > >> replace genksyms, in case people really want to have very specific > >> control about what causes the symbol version to be changed. > > > > Yeah it's great work, so is Stanislav's checker. I wouldn't mind having > > a kernel-centric checker tool merged in the kernel if it is small, > > maintained, and does a sufficient job for distros. > > Yes, I think this needs more experimentation and thought right now > before we can make a decision. Sure, I wanted to mention it in case people had a concern about out of tree tools. It will depend on what distros end up settling with. > >> Also I wonder what Ben's opinion on this is.. As I understood that he > >> wants to maintain a super-long-term stable kernel with kabi guarantees. > >> > >> Note, what we want is to weaken the check for kabi, by excluding parts > >> of the struct from genksyms with libabigail. For Red Hat genksyms is too > >> strict in the checks. > > > > Sure, that makes sense. > > > > So if I understand where we are, moving the ABI compatibility checking > > to one of these tools looks possible. What to do when we have an ABI change > > is not settled, but feeding version numbers explicitly into modversions > > is an option that would be close to what distros do today. > > Agreed! Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15.12.2016 15:15, Nicholas Piggin wrote: > On Thu, 15 Dec 2016 14:15:31 +0100 > Hannes Frederic Sowa <hannes@redhat.com> wrote: > >> On 15.12.2016 13:03, Nicholas Piggin wrote: >>> On Thu, 15 Dec 2016 12:19:02 +0100 >>> Hannes Frederic Sowa <hannes@redhat.com> wrote: >>> >>>> On 15.12.2016 03:06, Nicholas Piggin wrote: >>>>> On Wed, 14 Dec 2016 15:04:36 +0100 >>>>> Hannes Frederic Sowa <hannes@redhat.com> wrote: >>>>> >>>>>> On 09.12.2016 17:03, Greg Kroah-Hartman wrote: >>>>>>> On Sat, Dec 10, 2016 at 01:56:53AM +1000, Nicholas Piggin wrote: >>>>>>>> On Fri, 9 Dec 2016 15:36:04 +0100 >>>>>>>> Stanislav Kozina <skozina@redhat.com> wrote: >>>>>>>> >>>>>>>>>>>>> The question is how to provide a similar guarantee if a different way? >>>>>>>>>>>> As a tool to aid distro reviewers, modversions has some value, but the >>>>>>>>>>>> debug info parsing tools that have been mentioned in this thread seem >>>>>>>>>>>> superior (not that I've tested them). >>>>>>>>>>> On the other hand the big advantage of modversions is that it also >>>>>>>>>>> verifies the checksum during runtime (module loading). In other words, I >>>>>>>>>>> believe that any other solution should still generate some form of >>>>>>>>>>> checksum/watermark which can be easily checked for compatibility on >>>>>>>>>>> module load. >>>>>>>>>>> It should not be hard to add to the DWARF based tools though. We'd just >>>>>>>>>>> parse DWARF data instead of the C code. >>>>>>>>>> A runtime check is still done, with per-module vermagic which distros >>>>>>>>>> can change when they bump the ABI version. Is it really necessary to >>>>>>>>>> have more than that (i.e., per-symbol versioning)? >>>>>>>>> >>>>>>>>> From my point of view, it is. We need to allow changing ABI for some >>>>>>>>> modules while maintaining it for others. >>>>>>>>> In fact I think that there should be version not only for every exported >>>>>>>>> symbol (in the EXPORT_SYMBOL() sense), but also for every public type >>>>>>>>> (in the sense of eg. structure defined in the public header file). >>>>>>>> >>>>>>>> Well the distro can just append _v2, _v3 to the name of the function >>>>>>>> or type if it has to break compat for some reason. Would that be enough? >>>>>>> >>>>>>> There are other ways that distros can work around when upstream "breaks" >>>>>>> the ABI, sometimes they can rename functions, and others they can >>>>>>> "preload" structures with padding in anticipation for when/if fields get >>>>>>> added to them. But that's all up to the distros, no need for us to >>>>>>> worry about that at all :) >>>>>> >>>>>> The _v2 and _v3 functions are probably the ones that also get used by >>>>>> future backports in the distro kernel itself and are probably the reason >>>>>> for the ABI change in the first place. Thus going down this route will >>>>>> basically require distros to touch every future backport patch and will >>>>>> in general generate a big mess internally. >>>>> >>>>> What kind of big mess? You have to check the logic of each backport even >>>>> if it does apply cleanly, so the added overhead of the name change should >>>>> be relatively tiny, no? >>>> >>>> Basically single patches are backported in huge series. Reviewing each >>>> single patch also definitely makes sense, a review of the series as a >>>> whole is much more worthwhile because it focuses more on logic. >>>> >>>> The patches themselves are checked by individual robots or humans >>>> against merge conflict introduced mistakes which ring alarm bells for >>>> people to look more closely during review. >>>> >>>> Merge conflicts introduced mistakes definitely can happen because >>>> developers/backporters lose the focus from the actual logic but deal >>>> with shifting lines around or just fixing up postfixes to function names. >>>> >>>> We still try to align the kernel as much as possible with upstream, >>>> because most developers can't really hold the differences between >>>> upstream and the internal functions in their heads (is this function RMW >>>> safe in this version but not that kernel version...). >>> >>> I agree with all this, but in the case of a function rename, you can >>> automate it all with scripts if that's what you want. >>> >>> When you have your list of exported symbols with non-zero version number, >>> then you can script that __abivXXX into the changeset applying process, >>> or alternatively apply the rename after your patches are applied, or >>> use the c preprocessor to define names to something else. >> >> Yes, probably one could come up with coccinelle patches to do this, >> preprocessing/string matching could have false positives. But as I wrote >> above, we need one stable ABI and not multiple for our particular >> kernels, so it seems like a lot of overhead to rename particular >> functions internally all the time to make them inaccessible for external >> modules. > > I can't be sure until it's implemented in a workflow that distros are > happy with of course, but I just don't see why it would be a lot of > overhead. Particularly if you just scripted everything. Yes, me neither, of course. I just would favor to get away with the scripting if possible. ;) > How frequently do symbols become incompatible within an ostensibly ABI- > stable release? Given how genksyms works right now, I would say quite frequently. I would even go that far to say that every larger backport I had to deal with in networking had those problems because of some very important top level structs that make everything reachable (struct net_device or struct sock), so each change to those structs basically cause a kabi change. Even if we replace padding in a structure we added before a release, this is actually considered a kabi breakage, we must annotate this appropriately because of genksyms. E.g. I also thought about reordering the structs. As already discussed in this thread, genksyms takes ordering of its definition vs. declarations into account, e.g.: struct foo; struct bar { struct foo *f; }; << private >> struct foo { }; wouldn't actually include struct foo as part of the symvers, but the other way around it would. Instead of burden upstream with this kind of reordering, a suppression engine would be favored by me which we can drive by review and discussions. Thus upstream doesn't have any burden on that, but it is solely the burden of the distributions, where the burden belongs. ;) >>>> Like Don also already said, genksyms already did a pretty good job so >>>> far. We are right now working with Dodji to come up with a way to >>>> replace genksyms, in case people really want to have very specific >>>> control about what causes the symbol version to be changed. >>> >>> Yeah it's great work, so is Stanislav's checker. I wouldn't mind having >>> a kernel-centric checker tool merged in the kernel if it is small, >>> maintained, and does a sufficient job for distros. >> >> Yes, I think this needs more experimentation and thought right now >> before we can make a decision. > > Sure, I wanted to mention it in case people had a concern about out > of tree tools. It will depend on what distros end up settling with. The possibilities I see right now: * we leave genksyms where it is for the time being and add a KCONFIG knob to be able to call a replacement tool with well formed command line syntax and stdout semantics. * we import one of the alternatives instead of genksyms if it fits to the kernel. I am still a bit concerned about differences in dwarf vs. source code parsing. In the past I had twice bugs in dwarf generation, but this probably can be dealt with, but just needs a bit more experience in my opinion. * we remove genksyms and just use one versioning vector for upstream but still allow some kind of modversions per symbol being loaded from a third party program like kabi-dw or libabigail. Personally, because I see genksyms working quite well right now, I would stick to it until we (the distributions) came to a conclusion. Bye, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/.gitignore b/.gitignore index c2ed4ec..cde8773 100644 --- a/.gitignore +++ b/.gitignore @@ -20,7 +20,6 @@ *.mod.c *.i *.lst -*.symtypes *.order *.elf *.bin diff --git a/Documentation/dontdiff b/Documentation/dontdiff index 5385cba..40eb3e0 100644 --- a/Documentation/dontdiff +++ b/Documentation/dontdiff @@ -47,7 +47,6 @@ *.sgml *.so *.so.dbg -*.symtypes *.tab.c *.tab.h *.tex @@ -180,7 +179,6 @@ mktree modpost modules.builtin modules.order -modversions.h* nconf ncscope.* offset.h diff --git a/Documentation/kbuild/modules.txt b/Documentation/kbuild/modules.txt index 3fb39e0..1568b8a 100644 --- a/Documentation/kbuild/modules.txt +++ b/Documentation/kbuild/modules.txt @@ -61,10 +61,6 @@ make sure the kernel contains the information required. The target exists solely as a simple way to prepare a kernel source tree for building external modules. -NOTE: "modules_prepare" will not build Module.symvers even if -CONFIG_MODVERSIONS is set; therefore, a full kernel build needs to be -executed to make module versioning work. - --- 2.1 Command Syntax The command to build an external module is: diff --git a/Makefile b/Makefile index 694111b..2066419 100644 --- a/Makefile +++ b/Makefile @@ -316,13 +316,6 @@ KBUILD_MODULES := KBUILD_BUILTIN := 1 # If we have only "make modules", don't compile built-in objects. -# When we're building modules with modversions, we need to consider -# the built-in objects during the descend as well, in order to -# make sure the checksums are up to date before we record them. - -ifeq ($(MAKECMDGOALS),modules) - KBUILD_BUILTIN := $(if $(CONFIG_MODVERSIONS),1) -endif # If we have "make <whatever> modules", compile modules # in addition to whatever we do anyway. diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h index 06ff7fd..253f6ca 100644 --- a/arch/arm64/include/asm/module.h +++ b/arch/arm64/include/asm/module.h @@ -33,10 +33,6 @@ u64 module_emit_plt_entry(struct module *mod, const Elf64_Rela *rela, Elf64_Sym *sym); #ifdef CONFIG_RANDOMIZE_BASE -#ifdef CONFIG_MODVERSIONS -#define ARCH_RELOCATES_KCRCTAB -#define reloc_start (kimage_vaddr - KIMAGE_VADDR) -#endif extern u64 module_alloc_base; #else #define module_alloc_base ((u64)_etext - MODULES_VSIZE) diff --git a/arch/avr32/boot/images/Makefile b/arch/avr32/boot/images/Makefile index 2a3b539..0878bef 100644 --- a/arch/avr32/boot/images/Makefile +++ b/arch/avr32/boot/images/Makefile @@ -37,8 +37,6 @@ OBJCOPYFLAGS_vmlinux.elf := --change-section-lma .text-0x80000000 \ --change-section-lma __param-0x80000000 \ --change-section-lma __ksymtab-0x80000000 \ --change-section-lma __ksymtab_gpl-0x80000000 \ - --change-section-lma __kcrctab-0x80000000 \ - --change-section-lma __kcrctab_gpl-0x80000000 \ --change-section-lma __ksymtab_strings-0x80000000 \ --set-start 0xa0000000 $(obj)/vmlinux.elf: vmlinux FORCE diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h index cd4ffd8..94a7f7a 100644 --- a/arch/powerpc/include/asm/module.h +++ b/arch/powerpc/include/asm/module.h @@ -94,9 +94,5 @@ struct exception_table_entry; void sort_ex_table(struct exception_table_entry *start, struct exception_table_entry *finish); -#if defined(CONFIG_MODVERSIONS) && defined(CONFIG_PPC64) -#define ARCH_RELOCATES_KCRCTAB -#define reloc_start PHYSICAL_START -#endif #endif /* __KERNEL__ */ #endif /* _ASM_POWERPC_MODULE_H */ diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index 183368e..3e59856 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -277,30 +277,11 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr, return relocs * sizeof(struct ppc64_stub_entry); } -/* Still needed for ELFv2, for .TOC. */ -static void dedotify_versions(struct modversion_info *vers, - unsigned long size) -{ - struct modversion_info *end; - - for (end = (void *)vers + size; vers < end; vers++) - if (vers->name[0] == '.') { - memmove(vers->name, vers->name+1, strlen(vers->name)); -#ifdef ARCH_RELOCATES_KCRCTAB - /* The TOC symbol has no CRC computed. To avoid CRC - * check failing, we must force it to the expected - * value (see CRC check in module.c). - */ - if (!strcmp(vers->name, "TOC.")) - vers->crc = -(unsigned long)reloc_start; -#endif - } -} - /* * Undefined symbols which refer to .funcname, hack to funcname. Make .TOC. * seem to be defined (value set later). */ +/* XXX: remove for ELFv2? */ static void dedotify(Elf64_Sym *syms, unsigned int numsyms, char *strtab) { unsigned int i; @@ -349,9 +330,6 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr, me->arch.stubs_section = i; else if (strcmp(secstrings + sechdrs[i].sh_name, ".toc") == 0) me->arch.toc_section = i; - else if (strcmp(secstrings+sechdrs[i].sh_name,"__versions")==0) - dedotify_versions((void *)hdr + sechdrs[i].sh_offset, - sechdrs[i].sh_size); /* We don't handle .init for the moment: rename to _init */ while ((p = strstr(secstrings + sechdrs[i].sh_name, ".init"))) diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c index 0c2fae8..4c15dc0 100644 --- a/arch/x86/tools/relocs.c +++ b/arch/x86/tools/relocs.c @@ -59,7 +59,6 @@ static const char * const sym_regex_kernel[S_NSYMTYPES] = { "__(start|end)_pci_.*|" "__(start|end)_builtin_fw|" "__(start|stop)___ksymtab(|_gpl|_unused|_unused_gpl|_gpl_future)|" - "__(start|stop)___kcrctab(|_gpl|_unused|_unused_gpl|_gpl_future)|" "__(start|stop)___param|" "__(start|stop)___modver|" "__(start|stop)___bug_table|" diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 5673fff..89c3ece 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -30,11 +30,6 @@ * TODO: Figure out how to use SMB alerts. This will require a new * interface into the I2C driver, I believe. */ - -#if defined(MODVERSIONS) -#include <linux/modversions.h> -#endif - #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/sched.h> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile index 5e23e2d..0d9d84e 100644 --- a/drivers/firmware/efi/libstub/Makefile +++ b/drivers/firmware/efi/libstub/Makefile @@ -60,7 +60,7 @@ CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET) extra-$(CONFIG_EFI_ARMSTUB) := $(lib-y) lib-$(CONFIG_EFI_ARMSTUB) := $(patsubst %.o,%.stub.o,$(lib-y)) -STUBCOPY_FLAGS-y := -R .debug* -R *ksymtab* -R *kcrctab* +STUBCOPY_FLAGS-y := -R .debug* -R *ksymtab* STUBCOPY_FLAGS-$(CONFIG_ARM64) += --prefix-alloc-sections=.init \ --prefix-symbols=__efistub_ STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS @@ -80,5 +80,5 @@ quiet_cmd_stubcopy = STUBCPY $@ # explicitly by the decompressor linker script. # STUBCOPY_FLAGS-$(CONFIG_ARM) += --rename-section .data=.data.efistub \ - -R ___ksymtab+sort -R ___kcrctab+sort + -R ___ksymtab+sort STUBCOPY_RELOC-$(CONFIG_ARM) := R_ARM_ABS diff --git a/drivers/message/fusion/mptlan.h b/drivers/message/fusion/mptlan.h index 69e9d54..f976a5b 100644 --- a/drivers/message/fusion/mptlan.h +++ b/drivers/message/fusion/mptlan.h @@ -51,10 +51,7 @@ #define LINUX_MPTLAN_H_INCLUDED /*****************************************************************************/ -#if !defined(__GENKSYMS__) #include <linux/module.h> -#endif - #include <linux/netdevice.h> #include <linux/errno.h> // #include <linux/etherdevice.h> diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h index 63554e9..cb0c6cf 100644 --- a/include/asm-generic/export.h +++ b/include/asm-generic/export.h @@ -48,14 +48,6 @@ KSYM(__kstrtab_\name): .asciz "\name" #endif .previous -#ifdef CONFIG_MODVERSIONS - .section ___kcrctab\sec+\name,"a" - .balign KCRC_ALIGN -KSYM(__kcrctab_\name): - __put KSYM(__crc_\name) - .weak KSYM(__crc_\name) - .previous -#endif #endif .endm #undef __put diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 6d5d35f..8b52b57 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -360,41 +360,6 @@ VMLINUX_SYMBOL(__stop___ksymtab_gpl_future) = .; \ } \ \ - /* Kernel symbol table: Normal symbols */ \ - __kcrctab : AT(ADDR(__kcrctab) - LOAD_OFFSET) { \ - VMLINUX_SYMBOL(__start___kcrctab) = .; \ - KEEP(*(SORT(___kcrctab+*))) \ - VMLINUX_SYMBOL(__stop___kcrctab) = .; \ - } \ - \ - /* Kernel symbol table: GPL-only symbols */ \ - __kcrctab_gpl : AT(ADDR(__kcrctab_gpl) - LOAD_OFFSET) { \ - VMLINUX_SYMBOL(__start___kcrctab_gpl) = .; \ - KEEP(*(SORT(___kcrctab_gpl+*))) \ - VMLINUX_SYMBOL(__stop___kcrctab_gpl) = .; \ - } \ - \ - /* Kernel symbol table: Normal unused symbols */ \ - __kcrctab_unused : AT(ADDR(__kcrctab_unused) - LOAD_OFFSET) { \ - VMLINUX_SYMBOL(__start___kcrctab_unused) = .; \ - KEEP(*(SORT(___kcrctab_unused+*))) \ - VMLINUX_SYMBOL(__stop___kcrctab_unused) = .; \ - } \ - \ - /* Kernel symbol table: GPL-only unused symbols */ \ - __kcrctab_unused_gpl : AT(ADDR(__kcrctab_unused_gpl) - LOAD_OFFSET) { \ - VMLINUX_SYMBOL(__start___kcrctab_unused_gpl) = .; \ - KEEP(*(SORT(___kcrctab_unused_gpl+*))) \ - VMLINUX_SYMBOL(__stop___kcrctab_unused_gpl) = .; \ - } \ - \ - /* Kernel symbol table: GPL-future-only symbols */ \ - __kcrctab_gpl_future : AT(ADDR(__kcrctab_gpl_future) - LOAD_OFFSET) { \ - VMLINUX_SYMBOL(__start___kcrctab_gpl_future) = .; \ - KEEP(*(SORT(___kcrctab_gpl_future+*))) \ - VMLINUX_SYMBOL(__stop___kcrctab_gpl_future) = .; \ - } \ - \ /* Kernel symbol table: strings */ \ __ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) { \ KEEP(*(__ksymtab_strings)) \ diff --git a/include/linux/export.h b/include/linux/export.h index 2a0f61f..ba359cf 100644 --- a/include/linux/export.h +++ b/include/linux/export.h @@ -39,24 +39,11 @@ extern struct module __this_module; #ifdef CONFIG_MODULES -#if defined(__KERNEL__) && !defined(__GENKSYMS__) -#ifdef CONFIG_MODVERSIONS -/* Mark the CRC weak since genksyms apparently decides not to - * generate a checksums for some symbols */ -#define __CRC_SYMBOL(sym, sec) \ - extern __visible void *__crc_##sym __attribute__((weak)); \ - static const unsigned long __kcrctab_##sym \ - __used \ - __attribute__((section("___kcrctab" sec "+" #sym), used)) \ - = (unsigned long) &__crc_##sym; -#else -#define __CRC_SYMBOL(sym, sec) -#endif +#if defined(__KERNEL__) /* For every exported symbol, place a struct in the __ksymtab section */ #define ___EXPORT_SYMBOL(sym, sec) \ extern typeof(sym) sym; \ - __CRC_SYMBOL(sym, sec) \ static const char __kstrtab_##sym[] \ __attribute__((section("__ksymtab_strings"), aligned(1))) \ = VMLINUX_SYMBOL_STR(sym); \ @@ -110,7 +97,7 @@ extern struct module __this_module; #define EXPORT_UNUSED_SYMBOL_GPL(sym) #endif -#endif /* __GENKSYMS__ */ +#endif /* __KERNEL__ */ #else /* !CONFIG_MODULES... */ diff --git a/include/linux/module.h b/include/linux/module.h index 0c3207d..ee121f2 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -32,11 +32,6 @@ #define MODULE_NAME_LEN MAX_PARAM_PREFIX_LEN -struct modversion_info { - unsigned long crc; - char name[MODULE_NAME_LEN]; -}; - struct module; struct exception_table_entry; @@ -346,7 +341,6 @@ struct module { /* Exported symbols */ const struct kernel_symbol *syms; - const unsigned long *crcs; unsigned int num_syms; /* Kernel parameters. */ @@ -359,18 +353,15 @@ struct module { /* GPL-only exported symbols. */ unsigned int num_gpl_syms; const struct kernel_symbol *gpl_syms; - const unsigned long *gpl_crcs; #ifdef CONFIG_UNUSED_SYMBOLS /* unused exported symbols. */ const struct kernel_symbol *unused_syms; - const unsigned long *unused_crcs; unsigned int num_unused_syms; /* GPL-only, unused exported symbols. */ unsigned int num_unused_gpl_syms; const struct kernel_symbol *unused_gpl_syms; - const unsigned long *unused_gpl_crcs; #endif #ifdef CONFIG_MODULE_SIG @@ -382,7 +373,6 @@ struct module { /* symbols that will be GPL-only in the near future. */ const struct kernel_symbol *gpl_future_syms; - const unsigned long *gpl_future_crcs; unsigned int num_gpl_future_syms; /* Exception table */ @@ -523,7 +513,6 @@ struct module *find_module(const char *name); struct symsearch { const struct kernel_symbol *start, *stop; - const unsigned long *crcs; enum { NOT_GPL_ONLY, GPL_ONLY, @@ -539,7 +528,6 @@ struct symsearch { */ const struct kernel_symbol *find_symbol(const char *name, struct module **owner, - const unsigned long **crc, bool gplok, bool warn); diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h index 6f8fbcf..fafeea0 100644 --- a/include/linux/vermagic.h +++ b/include/linux/vermagic.h @@ -16,18 +16,16 @@ #else #define MODULE_VERMAGIC_MODULE_UNLOAD "" #endif -#ifdef CONFIG_MODVERSIONS -#define MODULE_VERMAGIC_MODVERSIONS "modversions " -#else -#define MODULE_VERMAGIC_MODVERSIONS "" -#endif #ifndef MODULE_ARCH_VERMAGIC #define MODULE_ARCH_VERMAGIC "" #endif +#ifdef CONFIG_MODULE_ABI_EXPLICIT +#define VERMAGIC_STRING CONFIG_MODULE_ABI_EXPLICIT_STRING +#else #define VERMAGIC_STRING \ UTS_RELEASE " " \ MODULE_VERMAGIC_SMP MODULE_VERMAGIC_PREEMPT \ - MODULE_VERMAGIC_MODULE_UNLOAD MODULE_VERMAGIC_MODVERSIONS \ + MODULE_VERMAGIC_MODULE_UNLOAD \ MODULE_ARCH_VERMAGIC - +#endif diff --git a/init/Kconfig b/init/Kconfig index 34407f1..7a1b6ac 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -1943,15 +1943,25 @@ config MODULE_FORCE_UNLOAD rmmod). This is mainly for kernel developers and desperate users. If unsure, say N. -config MODVERSIONS - bool "Module versioning support" - help - Usually, you have to use modules compiled with your kernel. - Saying Y here makes it sometimes possible to use modules - compiled for different kernels, by adding enough information - to the modules to (hopefully) spot any changes which would - make them incompatible with the kernel you are running. If - unsure, say N. +config MODULE_ABI_EXPLICIT + bool "Provide explicit module ABI version string" if EXPERT + default n + help + This option is mostly for distro maintainers. + + When this option is NOT set, the kernel/module ABI version string + is based on the kernel version (among other things), which means + modules are not compatible when there is any change to kernel + version. This is what most people want. + + Enabling this option allows you to provide an explicit module + ABI version string. This allows modules to be usable between kernel + version bumps. + + If unusure, say N. + +config MODULE_ABI_EXPLICIT_STRING + string "Explicit module ABI version string" if MODULE_ABI_EXPLICIT config MODULE_SRCVERSION_ALL bool "Source checksum for all modules" diff --git a/kernel/module.c b/kernel/module.c index 0e54d5b..ff68854 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -309,7 +309,7 @@ struct load_info { unsigned long mod_kallsyms_init_off; #endif struct { - unsigned int sym, str, mod, vers, info, pcpu; + unsigned int sym, str, mod, info, pcpu; } index; }; @@ -386,22 +386,11 @@ extern const struct kernel_symbol __start___ksymtab_gpl[]; extern const struct kernel_symbol __stop___ksymtab_gpl[]; extern const struct kernel_symbol __start___ksymtab_gpl_future[]; extern const struct kernel_symbol __stop___ksymtab_gpl_future[]; -extern const unsigned long __start___kcrctab[]; -extern const unsigned long __start___kcrctab_gpl[]; -extern const unsigned long __start___kcrctab_gpl_future[]; #ifdef CONFIG_UNUSED_SYMBOLS extern const struct kernel_symbol __start___ksymtab_unused[]; extern const struct kernel_symbol __stop___ksymtab_unused[]; extern const struct kernel_symbol __start___ksymtab_unused_gpl[]; extern const struct kernel_symbol __stop___ksymtab_unused_gpl[]; -extern const unsigned long __start___kcrctab_unused[]; -extern const unsigned long __start___kcrctab_unused_gpl[]; -#endif - -#ifndef CONFIG_MODVERSIONS -#define symversion(base, idx) NULL -#else -#define symversion(base, idx) ((base != NULL) ? ((base) + (idx)) : NULL) #endif static bool each_symbol_in_section(const struct symsearch *arr, @@ -430,20 +419,16 @@ bool each_symbol_section(bool (*fn)(const struct symsearch *arr, { struct module *mod; static const struct symsearch arr[] = { - { __start___ksymtab, __stop___ksymtab, __start___kcrctab, + { __start___ksymtab, __stop___ksymtab, NOT_GPL_ONLY, false }, { __start___ksymtab_gpl, __stop___ksymtab_gpl, - __start___kcrctab_gpl, GPL_ONLY, false }, { __start___ksymtab_gpl_future, __stop___ksymtab_gpl_future, - __start___kcrctab_gpl_future, WILL_BE_GPL_ONLY, false }, #ifdef CONFIG_UNUSED_SYMBOLS { __start___ksymtab_unused, __stop___ksymtab_unused, - __start___kcrctab_unused, NOT_GPL_ONLY, true }, { __start___ksymtab_unused_gpl, __stop___ksymtab_unused_gpl, - __start___kcrctab_unused_gpl, GPL_ONLY, true }, #endif }; @@ -455,23 +440,19 @@ bool each_symbol_section(bool (*fn)(const struct symsearch *arr, list_for_each_entry_rcu(mod, &modules, list) { struct symsearch arr[] = { - { mod->syms, mod->syms + mod->num_syms, mod->crcs, + { mod->syms, mod->syms + mod->num_syms, NOT_GPL_ONLY, false }, { mod->gpl_syms, mod->gpl_syms + mod->num_gpl_syms, - mod->gpl_crcs, GPL_ONLY, false }, { mod->gpl_future_syms, mod->gpl_future_syms + mod->num_gpl_future_syms, - mod->gpl_future_crcs, WILL_BE_GPL_ONLY, false }, #ifdef CONFIG_UNUSED_SYMBOLS { mod->unused_syms, mod->unused_syms + mod->num_unused_syms, - mod->unused_crcs, NOT_GPL_ONLY, true }, { mod->unused_gpl_syms, mod->unused_gpl_syms + mod->num_unused_gpl_syms, - mod->unused_gpl_crcs, GPL_ONLY, true }, #endif }; @@ -494,7 +475,6 @@ struct find_symbol_arg { /* Output */ struct module *owner; - const unsigned long *crc; const struct kernel_symbol *sym; }; @@ -527,7 +507,6 @@ static bool check_symbol(const struct symsearch *syms, #endif fsa->owner = owner; - fsa->crc = symversion(syms->crcs, symnum); fsa->sym = &syms->start[symnum]; return true; } @@ -556,11 +535,12 @@ static bool find_symbol_in_section(const struct symsearch *syms, return false; } -/* Find a symbol and return it, along with, (optional) crc and - * (optional) module which owns it. Needs preempt disabled or module_mutex. */ +/* + * Find a symbol and return it, along with, and (optional) module which owns + * it. Needs preempt disabled or module_mutex. + */ const struct kernel_symbol *find_symbol(const char *name, struct module **owner, - const unsigned long **crc, bool gplok, bool warn) { @@ -573,8 +553,6 @@ const struct kernel_symbol *find_symbol(const char *name, if (each_symbol_section(find_symbol_in_section, &fsa)) { if (owner) *owner = fsa.owner; - if (crc) - *crc = fsa.crc; return fsa.sym; } @@ -1031,7 +1009,7 @@ void __symbol_put(const char *symbol) struct module *owner; preempt_disable(); - if (!find_symbol(symbol, &owner, NULL, true, false)) + if (!find_symbol(symbol, &owner, true, false)) BUG(); module_put(owner); preempt_enable(); @@ -1256,118 +1234,6 @@ static int try_to_force_load(struct module *mod, const char *reason) #endif } -#ifdef CONFIG_MODVERSIONS -/* If the arch applies (non-zero) relocations to kernel kcrctab, unapply it. */ -static unsigned long maybe_relocated(unsigned long crc, - const struct module *crc_owner) -{ -#ifdef ARCH_RELOCATES_KCRCTAB - if (crc_owner == NULL) - return crc - (unsigned long)reloc_start; -#endif - return crc; -} - -static int check_version(Elf_Shdr *sechdrs, - unsigned int versindex, - const char *symname, - struct module *mod, - const unsigned long *crc, - const struct module *crc_owner) -{ - unsigned int i, num_versions; - struct modversion_info *versions; - - /* Exporting module didn't supply crcs? OK, we're already tainted. */ - if (!crc) - return 1; - - /* No versions at all? modprobe --force does this. */ - if (versindex == 0) - return try_to_force_load(mod, symname) == 0; - - versions = (void *) sechdrs[versindex].sh_addr; - num_versions = sechdrs[versindex].sh_size - / sizeof(struct modversion_info); - - for (i = 0; i < num_versions; i++) { - if (strcmp(versions[i].name, symname) != 0) - continue; - - if (versions[i].crc == maybe_relocated(*crc, crc_owner)) - return 1; - pr_debug("Found checksum %lX vs module %lX\n", - maybe_relocated(*crc, crc_owner), versions[i].crc); - goto bad_version; - } - - /* Broken toolchain. Warn once, then let it go.. */ - pr_warn_once("%s: no symbol version for %s\n", mod->name, symname); - return 1; - -bad_version: - pr_warn("%s: disagrees about version of symbol %s\n", - mod->name, symname); - return 0; -} - -static inline int check_modstruct_version(Elf_Shdr *sechdrs, - unsigned int versindex, - struct module *mod) -{ - const unsigned long *crc; - - /* - * Since this should be found in kernel (which can't be removed), no - * locking is necessary -- use preempt_disable() to placate lockdep. - */ - preempt_disable(); - if (!find_symbol(VMLINUX_SYMBOL_STR(module_layout), NULL, - &crc, true, false)) { - preempt_enable(); - BUG(); - } - preempt_enable(); - return check_version(sechdrs, versindex, - VMLINUX_SYMBOL_STR(module_layout), mod, crc, - NULL); -} - -/* First part is kernel version, which we ignore if module has crcs. */ -static inline int same_magic(const char *amagic, const char *bmagic, - bool has_crcs) -{ - if (has_crcs) { - amagic += strcspn(amagic, " "); - bmagic += strcspn(bmagic, " "); - } - return strcmp(amagic, bmagic) == 0; -} -#else -static inline int check_version(Elf_Shdr *sechdrs, - unsigned int versindex, - const char *symname, - struct module *mod, - const unsigned long *crc, - const struct module *crc_owner) -{ - return 1; -} - -static inline int check_modstruct_version(Elf_Shdr *sechdrs, - unsigned int versindex, - struct module *mod) -{ - return 1; -} - -static inline int same_magic(const char *amagic, const char *bmagic, - bool has_crcs) -{ - return strcmp(amagic, bmagic) == 0; -} -#endif /* CONFIG_MODVERSIONS */ - /* Resolve a symbol for this module. I.e. if we find one, record usage. */ static const struct kernel_symbol *resolve_symbol(struct module *mod, const struct load_info *info, @@ -1376,7 +1242,6 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod, { struct module *owner; const struct kernel_symbol *sym; - const unsigned long *crc; int err; /* @@ -1386,24 +1251,15 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod, */ sched_annotate_sleep(); mutex_lock(&module_mutex); - sym = find_symbol(name, &owner, &crc, - !(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true); + sym = find_symbol(name, &owner, + !(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true); if (!sym) goto unlock; - if (!check_version(info->sechdrs, info->index.vers, name, mod, crc, - owner)) { - sym = ERR_PTR(-EINVAL); - goto getname; - } - err = ref_module(mod, owner); - if (err) { + if (err) sym = ERR_PTR(err); - goto getname; - } -getname: /* We must make copy under the lock if we failed to get ref. */ strncpy(ownername, module_name(owner), MODULE_NAME_LEN); unlock: @@ -2149,7 +2005,7 @@ void *__symbol_get(const char *symbol) const struct kernel_symbol *sym; preempt_disable(); - sym = find_symbol(symbol, &owner, NULL, true, true); + sym = find_symbol(symbol, &owner, true, true); if (sym && strong_try_module_get(owner)) sym = NULL; preempt_enable(); @@ -2184,7 +2040,7 @@ static int verify_export_symbols(struct module *mod) for (i = 0; i < ARRAY_SIZE(arr); i++) { for (s = arr[i].sym; s < arr[i].sym + arr[i].num; s++) { - if (find_symbol(s->name, &owner, NULL, true, false)) { + if (find_symbol(s->name, &owner, true, false)) { pr_err("%s: exports duplicate symbol %s" " (owned by %s)\n", mod->name, s->name, module_name(owner)); @@ -2876,14 +2732,9 @@ static int rewrite_section_headers(struct load_info *info, int flags) #endif } - /* Track but don't keep modinfo and version sections. */ - if (flags & MODULE_INIT_IGNORE_MODVERSIONS) - info->index.vers = 0; /* Pretend no __versions section! */ - else - info->index.vers = find_sec(info, "__versions"); info->index.info = find_sec(info, ".modinfo"); info->sechdrs[info->index.info].sh_flags &= ~(unsigned long)SHF_ALLOC; - info->sechdrs[info->index.vers].sh_flags &= ~(unsigned long)SHF_ALLOC; + return 0; } @@ -2936,10 +2787,6 @@ static struct module *setup_load_info(struct load_info *info, int flags) info->index.pcpu = find_pcpusec(info); - /* Check module struct version now, before we try to use module. */ - if (!check_modstruct_version(info->sechdrs, info->index.vers, mod)) - return ERR_PTR(-ENOEXEC); - return mod; } @@ -2956,7 +2803,7 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags) err = try_to_force_load(mod, "bad vermagic"); if (err) return err; - } else if (!same_magic(modmagic, vermagic, info->index.vers)) { + } else if (strcmp(modmagic, vermagic)) { pr_err("%s: version magic '%s' should be '%s'\n", mod->name, modmagic, vermagic); return -ENOEXEC; @@ -2991,26 +2838,21 @@ static int find_module_sections(struct module *mod, struct load_info *info) sizeof(*mod->kp), &mod->num_kp); mod->syms = section_objs(info, "__ksymtab", sizeof(*mod->syms), &mod->num_syms); - mod->crcs = section_addr(info, "__kcrctab"); mod->gpl_syms = section_objs(info, "__ksymtab_gpl", sizeof(*mod->gpl_syms), &mod->num_gpl_syms); - mod->gpl_crcs = section_addr(info, "__kcrctab_gpl"); mod->gpl_future_syms = section_objs(info, "__ksymtab_gpl_future", sizeof(*mod->gpl_future_syms), &mod->num_gpl_future_syms); - mod->gpl_future_crcs = section_addr(info, "__kcrctab_gpl_future"); #ifdef CONFIG_UNUSED_SYMBOLS mod->unused_syms = section_objs(info, "__ksymtab_unused", sizeof(*mod->unused_syms), &mod->num_unused_syms); - mod->unused_crcs = section_addr(info, "__kcrctab_unused"); mod->unused_gpl_syms = section_objs(info, "__ksymtab_unused_gpl", sizeof(*mod->unused_gpl_syms), &mod->num_unused_gpl_syms); - mod->unused_gpl_crcs = section_addr(info, "__kcrctab_unused_gpl"); #endif #ifdef CONFIG_CONSTRUCTORS mod->ctors = section_objs(info, ".ctors", @@ -3159,19 +3001,6 @@ static int check_module_license_and_versions(struct module *mod) if (!prev_taint && test_taint(TAINT_PROPRIETARY_MODULE)) pr_warn("%s: module license taints kernel.\n", mod->name); -#ifdef CONFIG_MODVERSIONS - if ((mod->num_syms && !mod->crcs) - || (mod->num_gpl_syms && !mod->gpl_crcs) - || (mod->num_gpl_future_syms && !mod->gpl_future_crcs) -#ifdef CONFIG_UNUSED_SYMBOLS - || (mod->num_unused_syms && !mod->unused_crcs) - || (mod->num_unused_gpl_syms && !mod->unused_gpl_crcs) -#endif - ) { - return try_to_force_load(mod, - "no versions for exported symbols"); - } -#endif return 0; } @@ -4273,15 +4102,3 @@ void print_modules(void) pr_cont("\n"); } -#ifdef CONFIG_MODVERSIONS -/* Generate the signature for all relevant module structures here. - * If these change, we don't want to try to parse the module. */ -void module_layout(struct module *mod, - struct modversion_info *ver, - struct kernel_param *kp, - struct kernel_symbol *ks, - struct tracepoint * const *tp) -{ -} -EXPORT_SYMBOL(module_layout); -#endif diff --git a/scripts/Makefile b/scripts/Makefile index 1d80897..675a266 100644 --- a/scripts/Makefile +++ b/scripts/Makefile @@ -40,7 +40,6 @@ build_docproc: $(obj)/docproc build_check-lc_ctype: $(obj)/check-lc_ctype @: -subdir-$(CONFIG_MODVERSIONS) += genksyms subdir-y += mod subdir-$(CONFIG_SECURITY_SELINUX) += selinux subdir-$(CONFIG_DTC) += dtc diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 7675d11..5aa0fea 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -159,60 +159,12 @@ cmd_cpp_i_c = $(CPP) $(c_flags) -o $@ $< $(obj)/%.i: $(src)/%.c FORCE $(call if_changed_dep,cpp_i_c) -# These mirror gensymtypes_S and co below, keep them in synch. -cmd_gensymtypes_c = \ - $(CPP) -D__GENKSYMS__ $(c_flags) $< | \ - $(GENKSYMS) $(if $(1), -T $(2)) \ - $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX)) \ - $(if $(KBUILD_PRESERVE),-p) \ - -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null)) - -quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@ -cmd_cc_symtypes_c = \ - set -e; \ - $(call cmd_gensymtypes_c,true,$@) >/dev/null; \ - test -s $@ || rm -f $@ - -$(obj)/%.symtypes : $(src)/%.c FORCE - $(call cmd,cc_symtypes_c) - # C (.c) files # The C file is compiled and updated dependency information is generated. # (See cmd_cc_o_c + relevant part of rule_cc_o_c) - quiet_cmd_cc_o_c = CC $(quiet_modtag) $@ - -ifndef CONFIG_MODVERSIONS cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $< -else -# When module versioning is enabled the following steps are executed: -# o compile a .tmp_<file>.o from <file>.c -# o if .tmp_<file>.o doesn't contain a __ksymtab version, i.e. does -# not export symbols, we just rename .tmp_<file>.o to <file>.o and -# are done. -# o otherwise, we calculate symbol versions using the good old -# genksyms on the preprocessed source and postprocess them in a way -# that they are usable as a linker script -# o generate <file>.o from .tmp_<file>.o using the linker to -# replace the unresolved symbols __crc_exported_symbol with -# the actual value of the checksum generated by genksyms - -cmd_cc_o_c = $(CC) $(c_flags) -c -o $(@D)/.tmp_$(@F) $< - -cmd_modversions_c = \ - if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then \ - $(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \ - > $(@D)/.tmp_$(@F:.o=.ver); \ - \ - $(LD) $(LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) \ - -T $(@D)/.tmp_$(@F:.o=.ver); \ - rm -f $(@D)/.tmp_$(@F) $(@D)/.tmp_$(@F:.o=.ver); \ - else \ - mv -f $(@D)/.tmp_$(@F) $@; \ - fi; -endif - ifdef CONFIG_FTRACE_MCOUNT_RECORD ifdef BUILD_C_RECORDMCOUNT ifeq ("$(origin RECORDMCOUNT_WARN)", "command line") @@ -270,14 +222,12 @@ endif # CONFIG_STACK_VALIDATION define rule_cc_o_c $(call echo-cmd,checksrc) $(cmd_checksrc) \ $(call cmd_and_fixdep,cc_o_c) \ - $(cmd_modversions_c) \ $(cmd_objtool) \ $(call echo-cmd,record_mcount) $(cmd_record_mcount) endef define rule_as_o_S $(call cmd_and_fixdep,as_o_S) \ - $(cmd_modversions_S) \ $(cmd_objtool) endef @@ -317,39 +267,6 @@ modkern_aflags := $(KBUILD_AFLAGS_KERNEL) $(AFLAGS_KERNEL) $(real-objs-m) : modkern_aflags := $(KBUILD_AFLAGS_MODULE) $(AFLAGS_MODULE) $(real-objs-m:.o=.s): modkern_aflags := $(KBUILD_AFLAGS_MODULE) $(AFLAGS_MODULE) -# .S file exports must have their C prototypes defined in asm/asm-prototypes.h -# or a file that it includes, in order to get versioned symbols. We build a -# dummy C file that includes asm-prototypes and the EXPORT_SYMBOL lines from -# the .S file (with trailing ';'), and run genksyms on that, to extract vers. -# -# This is convoluted. The .S file must first be preprocessed to run guards and -# expand names, then the resulting exports must be constructed into plain -# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed -# to make the genksyms input. -# -# These mirror gensymtypes_c and co above, keep them in synch. -cmd_gensymtypes_S = \ - (echo "\#include <linux/kernel.h>" ; \ - echo "\#include <asm/asm-prototypes.h>" ; \ - $(CPP) $(a_flags) $< | \ - grep "\<___EXPORT_SYMBOL\>" | \ - sed 's/.*___EXPORT_SYMBOL[[:space:]]*\([a-zA-Z0-9_]*\)[[:space:]]*,.*/EXPORT_SYMBOL(\1);/' ) | \ - $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | \ - $(GENKSYMS) $(if $(1), -T $(2)) \ - $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX)) \ - $(if $(KBUILD_PRESERVE),-p) \ - -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null)) - -quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@ -cmd_cc_symtypes_S = \ - set -e; \ - $(call cmd_gensymtypes_S,true,$@) >/dev/null; \ - test -s $@ || rm -f $@ - -$(obj)/%.symtypes : $(src)/%.S FORCE - $(call cmd,cc_symtypes_S) - - quiet_cmd_cpp_s_S = CPP $(quiet_modtag) $@ cmd_cpp_s_S = $(CPP) $(a_flags) -o $@ $< @@ -357,38 +274,8 @@ $(obj)/%.s: $(src)/%.S FORCE $(call if_changed_dep,cpp_s_S) quiet_cmd_as_o_S = AS $(quiet_modtag) $@ - -ifndef CONFIG_MODVERSIONS cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $< -else - -ASM_PROTOTYPES := $(wildcard $(srctree)/arch/$(SRCARCH)/include/asm/asm-prototypes.h) - -ifeq ($(ASM_PROTOTYPES),) -cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $< - -else - -# versioning matches the C process described above, with difference that -# we parse asm-prototypes.h C header to get function definitions. - -cmd_as_o_S = $(CC) $(a_flags) -c -o $(@D)/.tmp_$(@F) $< - -cmd_modversions_S = \ - if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then \ - $(call cmd_gensymtypes_S,$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \ - > $(@D)/.tmp_$(@F:.o=.ver); \ - \ - $(LD) $(LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) \ - -T $(@D)/.tmp_$(@F:.o=.ver); \ - rm -f $(@D)/.tmp_$(@F) $(@D)/.tmp_$(@F:.o=.ver); \ - else \ - mv -f $(@D)/.tmp_$(@F) $@; \ - fi; -endif -endif - $(obj)/%.o: $(src)/%.S $(objtool_obj) FORCE $(call if_changed_rule,as_o_S) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 0a07f90..68018a6 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -89,8 +89,7 @@ multi-objs-m := $(addprefix $(obj)/,$(multi-objs-m)) subdir-ym := $(addprefix $(obj)/,$(subdir-ym)) obj-dirs := $(addprefix $(obj)/,$(obj-dirs)) -# These flags are needed for modversions and compiling, so we define them here -# already +# These flags are needed for compiling, so we define them here already # $(modname_flags) #defines KBUILD_MODNAME as the name of the module it will # end up in (or would, if it gets compiled in) # Note: Files that end up in two or more modules are compiled without the diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost index 16923ba..eb3d3c6 100644 --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -1,5 +1,5 @@ # =========================================================================== -# Module versions +# Module final link # =========================================================================== # # Stage one of module building created the following: diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh index 8dc1918..944c0bf 100755 --- a/scripts/adjust_autoksyms.sh +++ b/scripts/adjust_autoksyms.sh @@ -67,11 +67,6 @@ while read sym; do echo "#define __KSYM_${sym} 1" done >> "$new_ksyms_file" -# Special case for modversions (see modpost.c) -if [ -n "$CONFIG_MODVERSIONS" ]; then - echo "#define __KSYM_module_layout 1" >> "$new_ksyms_file" -fi - # Extract changes between old and new list and touch corresponding # dependency files. changed=$( diff --git a/scripts/export_report.pl b/scripts/export_report.pl index 8f79b70..255ef81 100755 --- a/scripts/export_report.pl +++ b/scripts/export_report.pl @@ -102,8 +102,6 @@ close($module_symvers); # # collect the usage count of each symbol. # -my $modversion_warnings = 0; - foreach my $thismod (@allcfiles) { my $module; @@ -112,30 +110,15 @@ foreach my $thismod (@allcfiles) { next; } - my $state=0; while ( <$module> ) { chomp; - if ($state == 0) { - $state = 1 if ($_ =~ /static const struct modversion_info/); - next; - } - if ($state == 1) { - $state = 2 if ($_ =~ /__attribute__\(\(section\("__versions"\)\)\)/); + if ( $_ !~ /0x[0-9a-f]+,/ ) { next; } - if ($state == 2) { - if ( $_ !~ /0x[0-9a-f]+,/ ) { - next; - } - my $sym = (split /([,"])/,)[4]; - my ($module, $value, $symbol, $gpl) = @{$SYMBOL{$sym}}; - $SYMBOL{ $sym } = [ $module, $value+1, $symbol, $gpl]; - push(@{$MODULE{$thismod}} , $sym); - } - } - if ($state != 2) { - warn "WARNING:$thismod is not built with CONFIG_MODVERSIONS enabled\n"; - $modversion_warnings++; + my $sym = (split /([,"])/,)[4]; + my ($module, $value, $symbol, $gpl) = @{$SYMBOL{$sym}}; + $SYMBOL{ $sym } = [ $module, $value+1, $symbol, $gpl]; + push(@{$MODULE{$thismod}} , $sym); } close($module); } @@ -169,9 +152,6 @@ printf("SECTION 2:\n\tThis section reports export-symbol-usage of in-kernel modules. Each module lists the modules, and the symbols from that module that it uses. Each listed symbol reports the number of modules using it\n"); -print "\nNOTE: Got $modversion_warnings CONFIG_MODVERSIONS warnings\n\n" - if $modversion_warnings; - print "~"x80 , "\n"; for my $thismod (sort keys %MODULE) { my $list = $MODULE{$thismod}; diff --git a/scripts/mksysmap b/scripts/mksysmap index a35acc0..7226ade 100755 --- a/scripts/mksysmap +++ b/scripts/mksysmap @@ -37,8 +37,6 @@ # readprofile starts reading symbols when _stext is found, and # continue until it finds a symbol which is not either of 'T', 't', -# 'W' or 'w'. __crc_ are 'A' and placed in the middle -# so we just ignore them to let readprofile continue to work. -# (At least sparc64 has __crc_ in the middle). +# 'W' or 'w'. -$NM -n $1 | grep -v '\( [aNUw] \)\|\(__crc_\)\|\( \$[adt]\)\|\( .L\)' > $2 +$NM -n $1 | grep -v '\( [aNUw] \)\|\( \$[adt]\)\|\( .L\)' > $2 diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index bd83497..c929794 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -23,8 +23,6 @@ #include "../../include/linux/license.h" #include "../../include/linux/export.h" -/* Are we using CONFIG_MODVERSIONS? */ -static int modversions = 0; /* Warn about undefined symbols? (do so if we have vmlinux) */ static int have_vmlinux = 0; /* Is CONFIG_MODULE_SRCVERSION_ALL set? */ @@ -159,13 +157,11 @@ static struct module *new_module(const char *modname) struct symbol { struct symbol *next; struct module *module; - unsigned int crc; - int crc_valid; unsigned int weak:1; unsigned int vmlinux:1; /* 1 if symbol is defined in vmlinux */ unsigned int kernel:1; /* 1 if symbol is from kernel * (only for external modules) **/ - unsigned int preloaded:1; /* 1 if symbol from Module.symvers, or crc */ + unsigned int preloaded:1; /* 1 if symbol from Module.symvers */ enum export export; /* Type of export */ char name[0]; }; @@ -328,20 +324,6 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod, return s; } -static void sym_update_crc(const char *name, struct module *mod, - unsigned int crc, enum export export) -{ - struct symbol *s = find_symbol(name); - - if (!s) { - s = new_symbol(name, mod, export); - /* Don't complain when we find it later. */ - s->preloaded = 1; - } - s->crc = crc; - s->crc_valid = 1; -} - void *grab_file(const char *filename, unsigned long *size) { struct stat st; @@ -601,13 +583,11 @@ static int ignore_undef_symbol(struct elf_info *info, const char *symname) return 0; } -#define CRC_PFX VMLINUX_SYMBOL_STR(__crc_) #define KSYMTAB_PFX VMLINUX_SYMBOL_STR(__ksymtab_) static void handle_modversions(struct module *mod, struct elf_info *info, Elf_Sym *sym, const char *symname) { - unsigned int crc; enum export export; if ((!is_vmlinux(mod->name) || mod->is_dot_o) && @@ -616,13 +596,6 @@ static void handle_modversions(struct module *mod, struct elf_info *info, else export = export_from_sec(info, get_secindex(info, sym)); - /* CRC'd symbol */ - if (strncmp(symname, CRC_PFX, strlen(CRC_PFX)) == 0) { - crc = (unsigned int) sym->st_value; - sym_update_crc(symname + strlen(CRC_PFX), mod, crc, - export); - } - switch (sym->st_shndx) { case SHN_COMMON: if (!strncmp(symname, "__gnu_lto_", sizeof("__gnu_lto_")-1)) { @@ -1979,13 +1952,6 @@ static void read_symbols(char *modname) sizeof(mod->srcversion)-1); parse_elf_finish(&info); - - /* Our trick to get versioning for module struct etc. - it's - * never passed as an argument to an exported function, so - * the automatic versioning doesn't pick it up, but it's really - * important anyhow */ - if (modversions) - mod->unres = alloc_symbol("module_layout", 0, mod->unres); } static void read_symbols_from_files(const char *filename) @@ -2137,70 +2103,6 @@ static void add_staging_flag(struct buffer *b, const char *name) buf_printf(b, "\nMODULE_INFO(staging, \"Y\");\n"); } -/* In kernel, this size is defined in linux/module.h; - * here we use Elf_Addr instead of long for covering cross-compile - */ -#define MODULE_NAME_LEN (64 - sizeof(Elf_Addr)) - -/** - * Record CRCs for unresolved symbols - **/ -static int add_versions(struct buffer *b, struct module *mod) -{ - struct symbol *s, *exp; - int err = 0; - - for (s = mod->unres; s; s = s->next) { - exp = find_symbol(s->name); - if (!exp || exp->module == mod) { - if (have_vmlinux && !s->weak) { - if (warn_unresolved) { - warn("\"%s\" [%s.ko] undefined!\n", - s->name, mod->name); - } else { - merror("\"%s\" [%s.ko] undefined!\n", - s->name, mod->name); - err = 1; - } - } - continue; - } - s->module = exp->module; - s->crc_valid = exp->crc_valid; - s->crc = exp->crc; - } - - if (!modversions) - return err; - - buf_printf(b, "\n"); - buf_printf(b, "static const struct modversion_info ____versions[]\n"); - buf_printf(b, "__used\n"); - buf_printf(b, "__attribute__((section(\"__versions\"))) = {\n"); - - for (s = mod->unres; s; s = s->next) { - if (!s->module) - continue; - if (!s->crc_valid) { - warn("\"%s\" [%s.ko] has no CRC!\n", - s->name, mod->name); - continue; - } - if (strlen(s->name) >= MODULE_NAME_LEN) { - merror("too long symbol \"%s\" [%s.ko]\n", - s->name, mod->name); - err = 1; - break; - } - buf_printf(b, "\t{ %#8x, __VMLINUX_SYMBOL_STR(%s) },\n", - s->crc, s->name); - } - - buf_printf(b, "};\n"); - - return err; -} - static void add_depends(struct buffer *b, struct module *mod, struct module *modules) { @@ -2290,7 +2192,7 @@ static void write_if_changed(struct buffer *b, const char *fname) } /* parse Module.symvers file. line format: - * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something] + * symbol<tab>module[[<tab>export]<tab>something] **/ static void read_dump(const char *fname, unsigned int kernel) { @@ -2303,14 +2205,11 @@ static void read_dump(const char *fname, unsigned int kernel) return; while ((line = get_next_line(&pos, file, size))) { - char *symname, *modname, *d, *export, *end; - unsigned int crc; + char *symname, *modname, *export, *end; struct module *mod; struct symbol *s; - if (!(symname = strchr(line, '\t'))) - goto fail; - *symname++ = '\0'; + symname = line; if (!(modname = strchr(symname, '\t'))) goto fail; *modname++ = '\0'; @@ -2318,8 +2217,7 @@ static void read_dump(const char *fname, unsigned int kernel) *export++ = '\0'; if (export && ((end = strchr(export, '\t')) != NULL)) *end = '\0'; - crc = strtoul(line, &d, 16); - if (*symname == '\0' || *modname == '\0' || *d != '\0') + if (*symname == '\0' || *modname == '\0') goto fail; mod = find_module(modname); if (!mod) { @@ -2331,7 +2229,6 @@ static void read_dump(const char *fname, unsigned int kernel) s = sym_add_exported(symname, mod, export_no(export)); s->kernel = kernel; s->preloaded = 1; - sym_update_crc(symname, mod, crc, export_no(export)); } release_file(file, size); return; @@ -2363,9 +2260,8 @@ static void write_dump(const char *fname) symbol = symbolhash[n]; while (symbol) { if (dump_sym(symbol)) - buf_printf(&buf, "0x%08x\t%s\t%s\t%s\n", - symbol->crc, symbol->name, - symbol->module->name, + buf_printf(&buf, "%s\t%s\t%s\n", + symbol->name, symbol->module->name, export_str(symbol->export)); symbol = symbol->next; } @@ -2406,9 +2302,6 @@ int main(int argc, char **argv) extsym_iter->file = optarg; extsym_start = extsym_iter; break; - case 'm': - modversions = 1; - break; case 'n': ignore_missing_files = 1; break; @@ -2474,7 +2367,6 @@ int main(int argc, char **argv) add_header(&buf, mod); add_intree_flag(&buf, !external_module); add_staging_flag(&buf, mod->name); - err |= add_versions(&buf, mod); add_depends(&buf, mod, modules); add_moddevtable(&buf, mod); add_srcversion(&buf, mod); diff --git a/scripts/module-common.lds b/scripts/module-common.lds index 73a2c7d..da76f519a 100644 --- a/scripts/module-common.lds +++ b/scripts/module-common.lds @@ -11,11 +11,6 @@ SECTIONS { __ksymtab_unused 0 : { *(SORT(___ksymtab_unused+*)) } __ksymtab_unused_gpl 0 : { *(SORT(___ksymtab_unused_gpl+*)) } __ksymtab_gpl_future 0 : { *(SORT(___ksymtab_gpl_future+*)) } - __kcrctab 0 : { *(SORT(___kcrctab+*)) } - __kcrctab_gpl 0 : { *(SORT(___kcrctab_gpl+*)) } - __kcrctab_unused 0 : { *(SORT(___kcrctab_unused+*)) } - __kcrctab_unused_gpl 0 : { *(SORT(___kcrctab_unused_gpl+*)) } - __kcrctab_gpl_future 0 : { *(SORT(___kcrctab_gpl_future+*)) } . = ALIGN(8); .init_array 0 : { *(SORT(.init_array.*)) *(.init_array) } diff --git a/scripts/namespace.pl b/scripts/namespace.pl index 9f3c9d4..e76f858 100755 --- a/scripts/namespace.pl +++ b/scripts/namespace.pl @@ -306,14 +306,12 @@ sub do_nm $name !~ /^__parm_/ && $name !~ /^__kstrtab/ && $name !~ /^__ksymtab/ && - $name !~ /^__kcrctab_/ && $name !~ /^__exitcall_/ && $name !~ /^__initcall_/ && $name !~ /^__kdb_initcall_/ && $name !~ /^__kdb_exitcall_/ && $name !~ /^__module_/ && $name !~ /^__mod_/ && - $name !~ /^__crc_/ && $name ne '__this_module' && $name ne 'kernel_version') { if (!exists($def{$name})) {