| Message ID | 20241030-extended-modversions-v8-2-93acdef62ce8@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Extended MODVERSIONS Support | expand |
On Wed, Oct 30, 2024 at 11:05:03PM +0000, 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
> 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?
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
On Fri, Nov 1, 2024 at 2:10 PM Luis Chamberlain <mcgrof@kernel.org> 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
On Tue, Nov 05, 2024 at 04:26:51PM -0800, Matthew Maurer wrote: > On Fri, Nov 1, 2024 at 2:10 PM Luis Chamberlain <mcgrof@kernel.org> 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
> > > 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
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
On Wed, Nov 6, 2024 at 10:27 PM Lucas De Marchi <lucas.demarchi@intel.com> 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
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
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
> 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.
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
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. + config MODULE_SRCVERSION_ALL bool "Source checksum for all modules" help diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost index 44936ebad161e914cbcc40ac74a2d651596d7b07..765da63d592be56fe93c0f4a35f1bfbcb924541a 100644 --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -43,6 +43,7 @@ MODPOST = scripts/mod/modpost modpost-args = \ $(if $(CONFIG_MODULES),-M) \ $(if $(CONFIG_MODVERSIONS),-m) \ + $(if $(CONFIG_EXTENDED_MODVERSIONS),-x) \ $(if $(CONFIG_MODULE_SRCVERSION_ALL),-a) \ $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E) \ $(if $(KBUILD_MODPOST_WARN),-w) \ diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index 107393a8c48a5993dbe456702fec0652a967ee86..bd38f33fd41fbd98bce34f8924b2fb0ac04297ee 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -32,6 +32,8 @@ static bool module_enabled; static bool modversions; /* Is CONFIG_MODULE_SRCVERSION_ALL set? */ static bool all_versions; +/* Is CONFIG_EXTENDED_MODVERSIONS set? */ +static bool extended_modversions; /* If we are modposting external module set to 1 */ static bool external_module; /* Only warn about unresolved symbols */ @@ -1817,6 +1819,52 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod) } } +/** + * Record CRCs for unresolved symbols, supporting long names + */ +static void add_extended_versions(struct buffer *b, struct module *mod) +{ + struct symbol *s; + + if (!extended_modversions) + return; + + buf_printf(b, "\n"); + buf_printf(b, "static const s32 ____version_ext_crcs[]\n"); + buf_printf(b, "__used __section(\"__version_ext_crcs\") = {\n"); + list_for_each_entry(s, &mod->unresolved_symbols, list) { + if (!s->module) + continue; + if (!s->crc_valid) { + /* + * We already warned on this when producing the legacy + * modversions table. + */ + continue; + } + buf_printf(b, "\t%#8x,\n", s->crc); + } + buf_printf(b, "};\n"); + + buf_printf(b, "static const char ____version_ext_names[]\n"); + buf_printf(b, "__used __section(\"__version_ext_names\") =\n"); + list_for_each_entry(s, &mod->unresolved_symbols, list) { + if (!s->module) + continue; + if (!s->crc_valid) { + /* + * We already warned on this when producing the legacy + * modversions table. + * We need to skip its name too, as the indexes in + * both tables need to align. + */ + continue; + } + buf_printf(b, "\t\"%s\\0\"\n", s->name); + } + buf_printf(b, ";\n"); +} + /** * Record CRCs for unresolved symbols **/ @@ -1840,9 +1888,14 @@ static void add_versions(struct buffer *b, struct module *mod) continue; } if (strlen(s->name) >= MODULE_NAME_LEN) { - error("too long symbol \"%s\" [%s.ko]\n", - s->name, mod->name); - break; + if (extended_modversions) + /* this symbol will only be in the extended info */ + continue; + else { + error("too long symbol \"%s\" [%s.ko]\n", + s->name, mod->name); + break; + } } buf_printf(b, "\t{ %#8x, \"%s\" },\n", s->crc, s->name); @@ -1972,6 +2025,7 @@ static void write_mod_c_file(struct module *mod) add_header(&buf, mod); add_exported_symbols(&buf, mod); add_versions(&buf, mod); + add_extended_versions(&buf, mod); add_depends(&buf, mod); add_moddevtable(&buf, mod); add_srcversion(&buf, mod); @@ -2130,7 +2184,7 @@ int main(int argc, char **argv) LIST_HEAD(dump_lists); struct dump_list *dl, *dl2; - while ((opt = getopt(argc, argv, "ei:MmnT:to:au:WwENd:")) != -1) { + while ((opt = getopt(argc, argv, "ei:MmnT:to:au:WwENd:x")) != -1) { switch (opt) { case 'e': external_module = true; @@ -2179,6 +2233,9 @@ int main(int argc, char **argv) case 'd': missing_namespace_deps = optarg; break; + case 'x': + extended_modversions = true; + break; default: exit(1); }