Re: [PATCH] modpost: support arbitrary symbol length in modversion

From: Lucas De Marchi
Date: Thu Jan 12 2023 - 16:49:17 EST

Next message: Michael Kelley: "[PATCH v5 01/14] x86/ioapic: Gate decrypted mapping on cc_platform_has() attribute"
Previous message: Namhyung Kim: "[PATCH 8/8] perf/core: Call perf_prepare_sample() before running BPF"
In reply to: Gary Guo: "[PATCH] modpost: support arbitrary symbol length in modversion"
Next in thread: Gary Guo: "Re: [PATCH] modpost: support arbitrary symbol length in modversion"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Michael Kelley: "[PATCH v5 01/14] x86/ioapic: Gate decrypted mapping on cc_platform_has() attribute"
Previous message: Namhyung Kim: "[PATCH 8/8] perf/core: Call perf_prepare_sample() before running BPF"
In reply to: Gary Guo: "[PATCH] modpost: support arbitrary symbol length in modversion"
Next in thread: Gary Guo: "Re: [PATCH] modpost: support arbitrary symbol length in modversion"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]