Messages in this thread Patch in this message | | | Date | Thu, 12 Jan 2023 14:40:59 -0700 | From | Lucas De Marchi <> | Subject | Re: [PATCH] modpost: support arbitrary symbol length in modversion |
| |
On Wed, Jan 11, 2023 at 04:11:51PM +0000, Gary Guo wrote: >Currently modversion uses a fixed size array of size (64 - sizeof(long)) >to store symbol names, thus placing a hard limit on length of symbols. >Rust symbols (which encodes crate and module names) can be quite a bit >longer. The length limit in kallsyms is increased to 512 for this reason. > >It's a waste of space to simply expand the fixed array size to 512 in >modversion info entries. I therefore make it variably sized, with offset >to the next entry indicated by the initial "next" field. > >In addition to supporting longer-than-56/60 byte symbols, this patch also >reduce the size for short symbols by getting rid of excessive 0 paddings. >There are still some zero paddings to ensure "next" and "crc" fields are >properly aligned. > >This patch does have a tiny drawback that it makes ".mod.c" files generated >a bit less easy to read, as code like > > "\x08\x00\x00\x00\x78\x56\x34\x12" > "symbol\0\0" > >is generated as opposed to > > { 0x12345678, "symbol" }, > >because the structure is now variable-length. But hopefully nobody reads >the generated file :) > >Link: b8a94bfb3395 ("kallsyms: increase maximum kernel symbol length to 512") >Link: https://github.com/Rust-for-Linux/linux/pull/379 > >Signed-off-by: Gary Guo <gary@garyguo.net> >--- > arch/powerpc/kernel/module_64.c | 3 ++- > include/linux/module.h | 6 ++++-- > kernel/module/version.c | 21 +++++++++------------ > scripts/export_report.pl | 9 +++++---- > scripts/mod/modpost.c | 33 +++++++++++++++++++++++---------- > 5 files changed, 43 insertions(+), 29 deletions(-) > >diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c >index ff045644f13f..eac23c11d579 100644 >--- a/arch/powerpc/kernel/module_64.c >+++ b/arch/powerpc/kernel/module_64.c >@@ -236,10 +236,11 @@ static void dedotify_versions(struct modversion_info *vers, > { > struct modversion_info *end; > >- for (end = (void *)vers + size; vers < end; vers++) >+ for (end = (void *)vers + size; vers < end; vers = (void *)vers + vers->next) { > if (vers->name[0] == '.') { > memmove(vers->name, vers->name+1, strlen(vers->name)); > } >+ } > } > > /* >diff --git a/include/linux/module.h b/include/linux/module.h >index 8c5909c0076c..37cb25af9099 100644 >--- a/include/linux/module.h >+++ b/include/linux/module.h >@@ -34,8 +34,10 @@ > #define MODULE_NAME_LEN MAX_PARAM_PREFIX_LEN > > struct modversion_info { >- unsigned long crc; >- char name[MODULE_NAME_LEN]; >+ /* Offset of the next modversion entry in relation to this one. */ >+ u32 next; >+ u32 crc; >+ char name[0];
although not really exported as uapi, this will break userspace as this is used in the elf file generated for the modules. I think this change must be made in a backward compatible way and kmod updated to deal with the variable name length:
kmod $ git grep "\[64" libkmod/libkmod-elf.c: char name[64 - sizeof(uint32_t)]; libkmod/libkmod-elf.c: char name[64 - sizeof(uint64_t)];
in kmod we have both 32 and 64 because a 64-bit kmod can read both 32 and 64 bit module, and vice versa.
Lucas De Marchi
> }; > > struct module; >diff --git a/kernel/module/version.c b/kernel/module/version.c >index 53f43ac5a73e..af7478dcc158 100644 >--- a/kernel/module/version.c >+++ b/kernel/module/version.c >@@ -17,32 +17,29 @@ int check_version(const struct load_info *info, > { > Elf_Shdr *sechdrs = info->sechdrs; > unsigned int versindex = info->index.vers; >- unsigned int i, num_versions; >- struct modversion_info *versions; >+ struct modversion_info *versions, *end; >+ u32 crcval; > > /* Exporting module didn't supply crcs? OK, we're already tainted. */ > if (!crc) > return 1; >+ crcval = *crc; > > /* 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); >+ end = (void *)versions + sechdrs[versindex].sh_size; > >- for (i = 0; i < num_versions; i++) { >- u32 crcval; >- >- if (strcmp(versions[i].name, symname) != 0) >+ for (; versions < end; versions = (void *)versions + versions->next) { >+ if (strcmp(versions->name, symname) != 0) > continue; > >- crcval = *crc; >- if (versions[i].crc == crcval) >+ if (versions->crc == crcval) > return 1; >- pr_debug("Found checksum %X vs module %lX\n", >- crcval, versions[i].crc); >+ pr_debug("Found checksum %X vs module %X\n", >+ crcval, versions->crc); > goto bad_version; > } > >diff --git a/scripts/export_report.pl b/scripts/export_report.pl >index feb3d5542a62..1117646f3141 100755 >--- a/scripts/export_report.pl >+++ b/scripts/export_report.pl >@@ -116,18 +116,19 @@ foreach my $thismod (@allcfiles) { > while ( <$module> ) { > chomp; > if ($state == 0) { >- $state = 1 if ($_ =~ /static const struct modversion_info/); >+ $state = 1 if ($_ =~ /static const char ____versions/); > next; > } > if ($state == 1) { >- $state = 2 if ($_ =~ /__attribute__\(\(section\("__versions"\)\)\)/); >+ $state = 2 if ($_ =~ /__used __section\("__versions"\)/); > next; > } > if ($state == 2) { >- if ( $_ !~ /0x[0-9a-f]+,/ ) { >+ if ( $_ !~ /\\0"/ ) { >+ last if ($_ =~ /;/); > next; > } >- my $sym = (split /([,"])/,)[4]; >+ my $sym = (split /(["\\])/,)[2]; > my ($module, $value, $symbol, $gpl) = @{$SYMBOL{$sym}}; > $SYMBOL{ $sym } = [ $module, $value+1, $symbol, $gpl]; > push(@{$MODULE{$thismod}} , $sym); >diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c >index efff8078e395..334d170de31f 100644 >--- a/scripts/mod/modpost.c >+++ b/scripts/mod/modpost.c >@@ -2046,13 +2046,17 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod) > static void add_versions(struct buffer *b, struct module *mod) > { > struct symbol *s; >+ unsigned int name_len; >+ unsigned int name_len_padded; >+ unsigned int tmp; >+ unsigned char *tmp_view = (unsigned char *)&tmp; > > if (!modversions) > return; > > buf_printf(b, "\n"); >- buf_printf(b, "static const struct modversion_info ____versions[]\n"); >- buf_printf(b, "__used __section(\"__versions\") = {\n"); >+ buf_printf(b, "static const char ____versions[]\n"); >+ buf_printf(b, "__used __section(\"__versions\") =\n"); > > list_for_each_entry(s, &mod->unresolved_symbols, list) { > if (!s->module) >@@ -2062,16 +2066,25 @@ static void add_versions(struct buffer *b, struct module *mod) > s->name, mod->name); > continue; > } >- if (strlen(s->name) >= MODULE_NAME_LEN) { >- error("too long symbol \"%s\" [%s.ko]\n", >- s->name, mod->name); >- break; >- } >- buf_printf(b, "\t{ %#8x, \"%s\" },\n", >- s->crc, s->name); >+ name_len = strlen(s->name); >+ name_len_padded = (name_len + 1 + 3) & ~3; >+ >+ /* Offset to next entry */ >+ tmp = TO_NATIVE(8 + name_len_padded); >+ buf_printf(b, "\t\"\\x%02x\\x%02x\\x%02x\\x%02x", >+ tmp_view[0], tmp_view[1], tmp_view[2], tmp_view[3]); >+ >+ tmp = TO_NATIVE(s->crc); >+ buf_printf(b, "\\x%02x\\x%02x\\x%02x\\x%02x\"\n", >+ tmp_view[0], tmp_view[1], tmp_view[2], tmp_view[3]); >+ >+ buf_printf(b, "\t\"%s", s->name); >+ for (; name_len < name_len_padded; name_len++) >+ buf_printf(b, "\\0"); >+ buf_printf(b, "\"\n"); > } > >- buf_printf(b, "};\n"); >+ buf_printf(b, ";\n"); > } > > static void add_depends(struct buffer *b, struct module *mod) >-- >2.34.1 >
| |