Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
On Thu, Nov 7, 2024 at 2:38 PM Luis Chamberlain wrote: > > On Wed, Nov 06, 2024 at 02:19:38PM -0800, Matthew Maurer wrote: > > > > > > > If booted against an old kernel, it will > > > > behave as though there is no modversions information. > > > > > > Huh? This I don't get. If you have the new libkmod and boot > > > an old kernel, that should just not break becauase well, long > > > symbols were not ever supported properly anyway, so no regression. > > > > Specifically, if you set NO_BASIC_MODVERSIONS, build a module, and > > then load said module with a kernel *before* EXTENDED_MODVERSIONS > > existed, it will see no modversion info on the module to check. This > > will be true regardless of symbol length. > > Isn't that just the same as disabling modverisons? > > If you select modversions, you get the options to choose: > > - old modversions > - old modversions + extended modversions > - extended modversions only Yes, what I'm pointing out is that kernels before the introduction of extended modversions will not know how to read extended modversions, and so they will treat modules with *only* extended modversions as though they have no modversions. > > > > I'm not quite sure I understood your last comment here though, > > > can you clarify what you meant? > > > > > > Anyway, so now that this is all cleared up, the next question I have > > > is, let's compare a NO_BASIC_MODVERSIONS world now, given that the > > > userspace requirements aren't large at all, what actual benefits does > > > using this new extended mod versions have? Why wouldn't a distro end > > > up preferring this for say a future release for all modules? > > > > I think a distro will end up preferring using this for all modules, > > but was intending to put both in for a transitional period until the > > new format was more accepted. > > The only thing left I think to test is the impact at runtime, and the > only thing I can think of is first we use find_symbol() on resolve_symbol() > which it took me a while to review and realize that this just uses a > completely different ELF section, the the ksymtab sections which are split up > between the old and the gpl section. But after that we use check_version(). > I suspect the major overhead here is in find_symbol() and that's in no way > shape > or form affected by your changes, and I also suspect that since the > way you implemented for_each_modversion_info_ext() is just *one* search > there shouldn't be any penalty here at all. Given it took *me* a while > to review all this, I think it would be good for you to also expand your > cover letter to be crystal clear on these expectations to users and > developers and if anything expand on the Kconfig / and add documentation > if we don't document any of this. I can add a commit extending modules.rst, but it's not clear to me what piece was surprising here - the existing MODVERSIONS format is *also* in a separate section. Nothing written in the "Module Versioning" section has been invalidated that I can see. Things I could think to add: * Summary of the internal data format (seems odd, since the previous one isn't here, and I'd think that an implementation detail anyways) * A warning about the effects of NO_BASIC_MODVERSIONS (probably better in Kconfig, isn't in the current changeset because the flag isn't there) > > I'd still like to see you guys test all this with the new TEST_KALLSYMS. I've attached the results of running TEST_KALLSYMS - it appears to be irrelevant to performance, as you expected. > > Luis extended-log Description: Binary data noextend-log Description: Binary data
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
On Mon, Nov 18, 2024 at 04:09:34PM -0800, Matthew Maurer wrote: > > Thinking about this some more, if we're going down enabling a new > > option, it seems to beg the question if the old *two* ksymtab sections > > could just be folded into the a new one where the "gpl only" thing > > becomes just one "column" as you call it. Reasons I ask, it seems like > > we're duplicating symbol names on ksymtab and for modeversions. Could > > you review this a bit? > > Short answer: We could do this, but I don't necessarily think it's a good > idea. Thanks for your review on this. I agree the complexities you outline don't yet justify the churn. Luis
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
On Thu, Nov 07, 2024 at 02:38:13PM -0800, Luis Chamberlain wrote: > The only thing left I think to test is the impact at runtime, and the > only thing I can think of is first we use find_symbol() on resolve_symbol() > which it took me a while to review and realize that this just uses a > completely different ELF section, the the ksymtab sections which are split up > between the old and the gpl section. Thinking about this some more, if we're going down enabling a new option, it seems to beg the question if the old *two* ksymtab sections could just be folded into the a new one where the "gpl only" thing becomes just one "column" as you call it. Reasons I ask, it seems like we're duplicating symbol names on ksymtab and for modeversions. Could you review this a bit? Luis
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
> Thinking about this some more, if we're going down enabling a new > option, it seems to beg the question if the old *two* ksymtab sections > could just be folded into the a new one where the "gpl only" thing > becomes just one "column" as you call it. Reasons I ask, it seems like > we're duplicating symbol names on ksymtab and for modeversions. Could > you review this a bit? Short answer: We could do this, but I don't necessarily think it's a good idea. ksymtab and modversions aren't duplicating names even with this patch series - We have two different formats, one for importing symbols, and one for exporting them. `__ksymtab`, `__ksymtab_gpl`, and `__ksymtab_strings` are used to export symbols. `__versions` or the new `__version_ext_names` and `__version_ext_crcs` are used to import them. For this reason, in any given compilation unit, a string should only appear either in the ksymtab (providing it), or in versions (consuming it). There also isn't as much immediate technical need for that kind of rework of the ksymtab format - ksymtab uses a string table for their names, so the "long name support" that extended modversions provides to modversions is already present in ksymtab. Combined, this means that there would be few technical benefits to this - the primary potential benefit I could see to something like this would be code complexity reduction, which is a bit of a matter of personal taste, and mine might not match others'. However, we could do some things similar to what's going on here: A. We could try to unify versions and ksymtab (this seems most viable, but the change in meaning of this data structure has me wary) B. We could make ksymtab use columnar storage for more things - it already does so for CRCs, we could theoretically make any or all of licensing, namespaces, or symbol values columnar. With the caveat that I am not convinced this restructuring is worth the churn, the way I would do A would be: 1. Add a field to the `kernel_symbol` that indicates whether the symbol is import/export (or possibly re-use `value` with a 0 value after linker resolution to mean "import" instead of export). 2. Generate `kernel_symbol` entries for imported symbols, not just exported ones. 3. Read `kcrctab` for import symbols to figure out what the expected crc value is when importing, rather than using versions. 4. Stop generating/reading any of `__versions`, `__version_ext_names`, `__versions_ext_crcs`, etc. There are two downsides I can see to this: 1. You cannot make this backwards compatible with existing `kmod`. (This was the argument given against just enlarging MODVERSIONS symbol names.) 2. It's hard to be certain that we know about all users of `ksymtab` in order to ensure they all know the new convention around imported vs exported symbols. I think that B would actually make things worse because symbols always today always have a value, a namespace, a name, and a license. The only thing that's optional is the CRC, and that's already columnar. Making the other ones columnar would hurt locality. We'd still need the strtab sections, or we'd end up with many copies of each namespace, where today that should get deduped down by the linker. Columns are good for things that are extensions, optional, or variable length. If there are other reasons *for* doing this that I'm not aware of, what I'd do would be: 1. Use the name as the primary index, same as modversions. 2. Split each other piece into its own column, with a joint iterator. 3. Convert license into a column, with an enum value (currently only fully exported or GPL). 4. Replace places in the coe where a `struct kernel_symbol *` is used today with an iterator over the joint columns. Again, to reiterate, I *do not* think that B is a good idea. A might be, but the improvement seems sufficiently marginal to me that I don't know if it's worth the churn.
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
On Wed, Nov 06, 2024 at 02:19:38PM -0800, Matthew Maurer wrote: > > > > > If booted against an old kernel, it will > > > behave as though there is no modversions information. > > > > Huh? This I don't get. If you have the new libkmod and boot > > an old kernel, that should just not break becauase well, long > > symbols were not ever supported properly anyway, so no regression. > > Specifically, if you set NO_BASIC_MODVERSIONS, build a module, and > then load said module with a kernel *before* EXTENDED_MODVERSIONS > existed, it will see no modversion info on the module to check. This > will be true regardless of symbol length. Isn't that just the same as disabling modverisons? If you select modversions, you get the options to choose: - old modversions - old modversions + extended modversions - extended modversions only > > I'm not quite sure I understood your last comment here though, > > can you clarify what you meant? > > > > Anyway, so now that this is all cleared up, the next question I have > > is, let's compare a NO_BASIC_MODVERSIONS world now, given that the > > userspace requirements aren't large at all, what actual benefits does > > using this new extended mod versions have? Why wouldn't a distro end > > up preferring this for say a future release for all modules? > > I think a distro will end up preferring using this for all modules, > but was intending to put both in for a transitional period until the > new format was more accepted. The only thing left I think to test is the impact at runtime, and the only thing I can think of is first we use find_symbol() on resolve_symbol() which it took me a while to review and realize that this just uses a completely different ELF section, the the ksymtab sections which are split up between the old and the gpl section. But after that we use check_version(). I suspect the major overhead here is in find_symbol() and that's in no way shape or form affected by your changes, and I also suspect that since the way you implemented for_each_modversion_info_ext() is just *one* search there shouldn't be any penalty here at all. Given it took *me* a while to review all this, I think it would be good for you to also expand your cover letter to be crystal clear on these expectations to users and developers and if anything expand on the Kconfig / and add documentation if we don't document any of this. I'd still like to see you guys test all this with the new TEST_KALLSYMS. Luis
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
On Wed, Nov 6, 2024 at 10:27 PM Lucas De Marchi wrote: > > On Wed, Nov 06, 2024 at 02:19:38PM -0800, Matthew Maurer wrote: > >> > >> > If booted against an old kernel, it will > >> > behave as though there is no modversions information. > >> > >> Huh? This I don't get. If you have the new libkmod and boot > >> an old kernel, that should just not break becauase well, long > >> symbols were not ever supported properly anyway, so no regression. > > > >Specifically, if you set NO_BASIC_MODVERSIONS, build a module, and > > how are you setting NO_BASIC_MODVERSIONS and loading it in a kernel > that still doesn't have that, i.e. before EXTENDED_MODVERSIONS? That action would involve e.g. building a module against a 6.13 series kernel with NO_BASIC_MODVERSIONS and trying insmod it on a 6.12 series kernel. I know it's not supported, I was just trying to describe the full matrix of what would happen differently with the proposed additional config flag. > > Please Cc me on the format change and if possible submit the libkmod > support. It seems awkward to adjust kmod to support a format that still hasn't been accepted to the kernel. I can send kmod patches to support it, but since this patch series hasn't been accepted yet, it seemed a bit premature. I'll explicitly add you to the format change (patch before this in the series) and add you to the whole series in v9 > > thanks > Lucas De Marchi > > >then load said module with a kernel *before* EXTENDED_MODVERSIONS > >existed, it will see no modversion info on the module to check. This > >will be true regardless of symbol length. > > > >> > >> I'm not quite sure I understood your last comment here though, > >> can you clarify what you meant? > >> > >> Anyway, so now that this is all cleared up, the next question I have > >> is, let's compare a NO_BASIC_MODVERSIONS world now, given that the > >> userspace requirements aren't large at all, what actual benefits does > >> using this new extended mod versions have? Why wouldn't a distro end > >> up preferring this for say a future release for all modules? > > > >I think a distro will end up preferring using this for all modules, > >but was intending to put both in for a transitional period until the > >new format was more accepted. > > > >> > >> Luis
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
On Wed, Nov 06, 2024 at 02:19:38PM -0800, Matthew Maurer wrote: > If booted against an old kernel, it will > behave as though there is no modversions information. Huh? This I don't get. If you have the new libkmod and boot an old kernel, that should just not break becauase well, long symbols were not ever supported properly anyway, so no regression. Specifically, if you set NO_BASIC_MODVERSIONS, build a module, and how are you setting NO_BASIC_MODVERSIONS and loading it in a kernel that still doesn't have that, i.e. before EXTENDED_MODVERSIONS? Please Cc me on the format change and if possible submit the libkmod support. thanks Lucas De Marchi then load said module with a kernel *before* EXTENDED_MODVERSIONS existed, it will see no modversion info on the module to check. This will be true regardless of symbol length. I'm not quite sure I understood your last comment here though, can you clarify what you meant? Anyway, so now that this is all cleared up, the next question I have is, let's compare a NO_BASIC_MODVERSIONS world now, given that the userspace requirements aren't large at all, what actual benefits does using this new extended mod versions have? Why wouldn't a distro end up preferring this for say a future release for all modules? I think a distro will end up preferring using this for all modules, but was intending to put both in for a transitional period until the new format was more accepted. Luis
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
> > > If booted against an old kernel, it will > > behave as though there is no modversions information. > > Huh? This I don't get. If you have the new libkmod and boot > an old kernel, that should just not break becauase well, long > symbols were not ever supported properly anyway, so no regression. Specifically, if you set NO_BASIC_MODVERSIONS, build a module, and then load said module with a kernel *before* EXTENDED_MODVERSIONS existed, it will see no modversion info on the module to check. This will be true regardless of symbol length. > > I'm not quite sure I understood your last comment here though, > can you clarify what you meant? > > Anyway, so now that this is all cleared up, the next question I have > is, let's compare a NO_BASIC_MODVERSIONS world now, given that the > userspace requirements aren't large at all, what actual benefits does > using this new extended mod versions have? Why wouldn't a distro end > up preferring this for say a future release for all modules? I think a distro will end up preferring using this for all modules, but was intending to put both in for a transitional period until the new format was more accepted. > > Luis
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
On Tue, Nov 05, 2024 at 04:26:51PM -0800, Matthew Maurer wrote: > On Fri, Nov 1, 2024 at 2:10 PM Luis Chamberlain wrote: > > > > On Thu, Oct 31, 2024 at 01:00:28PM -0700, Matthew Maurer wrote: > > > > The question is, if only extended moversions are used, what new tooling > > > > requirements are there? Can you test using only extended modversions? > > > > > > > > Luis > > > > > > I'm not sure precisely what you're asking for. Do you want: > > > 1. A kconfig that suppresses the emission of today's MODVERSIONS > > > format? > > > > Yes that's right, a brave new world, and with the warning of that. > > OK, I can send another revision with a suppression config, perhaps > CONFIG_NO_BASIC_MODVERSIONS Great. > > > This would be fairly easy to do, but I was leaving it enabled > > > for compatibility's sake, at least until extended modversions become > > > more common. This way existing `kmod` tools and kernels would continue > > > to be able to load new-style modules. > > > > Sure, understood why we'd have both. > > > > > 2. libkmod support for parsing the new format? I can do that fairly > > > easily too, but wanted the format actually decided on and accepted > > > before I started modifying things that read modversions. > > > > This is implied, what I'd like is for an A vs B comparison to be able to > > be done on even without rust modules, so that we can see if really > > libkmod changes are all that's needed. Does boot fail without a new > > libkmod for this? If so the Kconfig should specificy that for this new > > brave new world. > > libkmod changes are not needed for boot - the userspace tools do not > examine this data for anything inline with boot at the moment, libkmod > only looks at it for kmod_module_get_versions, and modprobe only looks > at that with --show-modversions or --dump-modversions, which are not > normally part of boot. > > With the code as is, the only change will be that if a module with > EXTENDED_MODVERSIONS set contains an over-length symbol (which > wouldn't have been possible before), the overlong symbol's modversion > data will not appear in --show-modversions. After patching `libkmod` > in a follow-up patch, long symbols would appear as well. If booted > against an old kernel, long symbols will not have their CRCs in the > list to be checked. However, the old kernel could not export these > symbols, so it will fail to resolve the symbol and fail the load > regardless. Thanks for checking all this. It is exactly what I was looking for. All this should be part of the cover letter and Kconfig documentation. > If we add and enable NO_BASIC_MODVERSIONS like you suggested above, > today's --show-modversions will claim there is no modversions data. > Applying a libkmod patch will result in modversions info being > displayed by that command again. If booted against a new kernel, > everything will be fine. *This* is is the sort of information I was also looking for and I think it would be good to make it clear for the upcoming NO_BASIC_MODVERSIONS. > If booted against an old kernel, it will > behave as though there is no modversions information. Huh? This I don't get. If you have the new libkmod and boot an old kernel, that should just not break becauase well, long symbols were not ever supported properly anyway, so no regression. I'm not quite sure I understood your last comment here though, can you clarify what you meant? Anyway, so now that this is all cleared up, the next question I have is, let's compare a NO_BASIC_MODVERSIONS world now, given that the userspace requirements aren't large at all, what actual benefits does using this new extended mod versions have? Why wouldn't a distro end up preferring this for say a future release for all modules? Luis
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
On Fri, Nov 1, 2024 at 2:10 PM Luis Chamberlain wrote: > > On Thu, Oct 31, 2024 at 01:00:28PM -0700, Matthew Maurer wrote: > > > The question is, if only extended moversions are used, what new tooling > > > requirements are there? Can you test using only extended modversions? > > > > > > Luis > > > > I'm not sure precisely what you're asking for. Do you want: > > 1. A kconfig that suppresses the emission of today's MODVERSIONS > > format? > > Yes that's right, a brave new world, and with the warning of that. OK, I can send another revision with a suppression config, perhaps CONFIG_NO_BASIC_MODVERSIONS > > > > This would be fairly easy to do, but I was leaving it enabled > > for compatibility's sake, at least until extended modversions become > > more common. This way existing `kmod` tools and kernels would continue > > to be able to load new-style modules. > > Sure, understood why we'd have both. > > > 2. libkmod support for parsing the new format? I can do that fairly > > easily too, but wanted the format actually decided on and accepted > > before I started modifying things that read modversions. > > This is implied, what I'd like is for an A vs B comparison to be able to > be done on even without rust modules, so that we can see if really > libkmod changes are all that's needed. Does boot fail without a new > libkmod for this? If so the Kconfig should specificy that for this new > brave new world. libkmod changes are not needed for boot - the userspace tools do not examine this data for anything inline with boot at the moment, libkmod only looks at it for kmod_module_get_versions, and modprobe only looks at that with --show-modversions or --dump-modversions, which are not normally part of boot. With the code as is, the only change will be that if a module with EXTENDED_MODVERSIONS set contains an over-length symbol (which wouldn't have been possible before), the overlong symbol's modversion data will not appear in --show-modversions. After patching `libkmod` in a follow-up patch, long symbols would appear as well. If booted against an old kernel, long symbols will not have their CRCs in the list to be checked. However, the old kernel could not export these symbols, so it will fail to resolve the symbol and fail the load regardless. If we add and enable NO_BASIC_MODVERSIONS like you suggested above, today's --show-modversions will claim there is no modversions data. Applying a libkmod patch will result in modversions info being displayed by that command again. If booted against a new kernel, everything will be fine. If booted against an old kernel, it will behave as though there is no modversions information. > > > If a distribution can leverage just one format, why would they not > consider it if they can ensure the proper tooling is in place. We > haven't itemized the differences in practice and this could help > with this. One clear difference so far is the kabi stuff, but that's The kabi stuff is at least partially decoupled - you can (and it sounds like from the responses to Sami's change, occasionally might want to) enable debug symbol based modversions even without extended modversions. You can also enable extended modversions without the debug symbol based modversions, though there are less clear use-cases for that. > just evaluating one way of doing things so far, I suspect we'll get > more review on that from Petr soon. > > Luis
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
On Thu, Oct 31, 2024 at 01:00:28PM -0700, Matthew Maurer wrote: > > The question is, if only extended moversions are used, what new tooling > > requirements are there? Can you test using only extended modversions? > > > > Luis > > I'm not sure precisely what you're asking for. Do you want: > 1. A kconfig that suppresses the emission of today's MODVERSIONS > format? Yes that's right, a brave new world, and with the warning of that. > This would be fairly easy to do, but I was leaving it enabled > for compatibility's sake, at least until extended modversions become > more common. This way existing `kmod` tools and kernels would continue > to be able to load new-style modules. Sure, understood why we'd have both. > 2. libkmod support for parsing the new format? I can do that fairly > easily too, but wanted the format actually decided on and accepted > before I started modifying things that read modversions. This is implied, what I'd like is for an A vs B comparison to be able to be done on even without rust modules, so that we can see if really libkmod changes are all that's needed. Does boot fail without a new libkmod for this? If so the Kconfig should specificy that for this new brave new world. If a distribution can leverage just one format, why would they not consider it if they can ensure the proper tooling is in place. We haven't itemized the differences in practice and this could help with this. One clear difference so far is the kabi stuff, but that's just evaluating one way of doing things so far, I suspect we'll get more review on that from Petr soon. Luis
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
> The question is, if only extended moversions are used, what new tooling > requirements are there? Can you test using only extended modversions? > > Luis I'm not sure precisely what you're asking for. Do you want: 1. A kconfig that suppresses the emission of today's MODVERSIONS format? This would be fairly easy to do, but I was leaving it enabled for compatibility's sake, at least until extended modversions become more common. This way existing `kmod` tools and kernels would continue to be able to load new-style modules. 2. libkmod support for parsing the new format? I can do that fairly easily too, but wanted the format actually decided on and accepted before I started modifying things that read modversions. 3. Something else? Maybe I'm not understanding your comment?
Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
On Wed, Oct 30, 2024 at 11:05:03PM +, Matthew Maurer wrote: > diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig > index > e6b2427e5c190aacf7b9c5c1bb57fca39d311564..a31c617cd67d3d66b24d2fba34cbd5cc9c53ab78 > 100644 > --- a/kernel/module/Kconfig > +++ b/kernel/module/Kconfig > @@ -208,6 +208,16 @@ config ASM_MODVERSIONS > assembly. This can be enabled only when the target architecture > supports it. > > +config EXTENDED_MODVERSIONS > + bool "Extended Module Versioning Support" > + depends on MODVERSIONS > + help > + This enables extended MODVERSIONs support, allowing long symbol > + names to be versioned. > + > + The most likely reason you would enable this is to enable Rust > + support. If unsure, say N. > + The question is, if only extended moversions are used, what new tooling requirements are there? Can you test using only extended modversions? Luis
