From: Andrea Righi <andrea.righi@canonical.com>
To: "Michal Suchánek" <msuchanek@suse.de>
Cc: "Gary Guo" <gary@garyguo.net>,
"Kees Cook" <keescook@chromium.org>,
"Masahiro Yamada" <masahiroy@kernel.org>,
linux-kbuild@vger.kernel.org,
"Wedson Almeida Filho" <wedsonaf@google.com>,
"Joel Stanley" <joel@jms.id.au>,
"Alex Gaynor" <alex.gaynor@gmail.com>,
"Miguel Ojeda" <ojeda@kernel.org>,
"Wedson Almeida Filho" <wedsonaf@gmail.com>,
"Nicolas Schier" <nicolas@fjasle.eu>,
rust-for-linux@vger.kernel.org,
"Guo Zhengkui" <guozhengkui@vivo.com>,
"Boqun Feng" <boqun.feng@gmail.com>,
"Nicholas Piggin" <npiggin@gmail.com>,
"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
"Nathan Chancellor" <nathan@kernel.org>,
"Nick Desaulniers" <ndesaulniers@google.com>,
linux-kernel@vger.kernel.org,
"Julia Lawall" <Julia.Lawall@inria.fr>,
"Luis Chamberlain" <mcgrof@kernel.org>,
linuxppc-dev@lists.ozlabs.org, linux-modules@vger.kernel.org
Subject: Re: [PATCH] modpost: support arbitrary symbol length in modversion
Date: Tue, 14 Mar 2023 15:59:32 +0100 [thread overview]
Message-ID: <ZBCL1G+8z8c6zLI0@righiandr-XPS-13-7390> (raw)
In-Reply-To: <ZBCG3ykG1q4GEcIf@righiandr-XPS-13-7390>
On Tue, Mar 14, 2023 at 03:38:24PM +0100, Andrea Righi wrote:
> On Mon, Mar 13, 2023 at 11:09:31PM +0100, Andrea Righi wrote:
> > On Mon, Mar 13, 2023 at 11:02:34PM +0100, Michal Suchánek wrote:
> > > On Mon, Mar 13, 2023 at 10:53:34PM +0100, Andrea Righi wrote:
> > > > On Mon, Mar 13, 2023 at 10:48:53PM +0100, Michal Suchánek wrote:
> > > > > Hello,
> > > > >
> > > > > On Mon, Mar 13, 2023 at 09:32:16PM +0100, Andrea Righi wrote:
> > > > > > 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>
> > > > > >
> > > > > > Is there any newer version of this patch?
> > > > > >
> > > > > > I'm doing some tests with it, but I'm getting boot failures on ppc64
> > > > > > with this applied (at boot kernel is spitting out lots of oops'es and
> > > > > > unfortunately it's really hard to copy paste or just read them from the
> > > > > > console).
> > > > >
> > > > > Are you using the ELF ABI v1 or v2?
> > > > >
> > > > > v1 may have some additional issues when it comes to these symbol tables.
> > > > >
> > > > > Thanks
> > > > >
> > > > > Michal
> > > >
> > > > I have CONFIG_PPC64_ELF_ABI_V2=y in my .config, so I guess I'm using v2.
> > > >
> > > > BTW, the issue seems to be in dedotify_versions(), as a silly test I
> > > > tried to comment out this function completely to be a no-op and now my
> > > > system boots fine (but I guess I'm probably breaking something else).
> > >
> > > Probably not. You should not have the extra leading dot on ABI v2. So if
> > > dedotify does something that means something generates and then expects
> > > back symbols with a leading dot, and this workaround for ABI v1 breaks
> > > that. Or maybe it is called when it shouldn't.
> >
> > Hm.. I'll add some debugging to this function to see what happens exactly.
>
> Alright I've done more tests across different architectures. My problem
> with ppc64 is that this architecture is evaluating sechdrs[i].sh_size
> using get_stubs_size(), that apparently can add some extra padding, so
> doing (vers + vers->next < end) isn't a reliable check to determine the
> end of the variable array, because sometimes "end" can be greater than
> the last "vers + vers->next" entry.
>
> In general I think it'd be more reliable to add a dummy NULL entry at
> the end of the modversion array.
>
> Moreover, I think we also need to enforce struct modversion_info to be
> __packed, just to make sure that no extra padding is added (otherwise it
> may break our logic to determine the offset of the next entry).
>
> > @@ -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);
>
> ^ Here's another issue that I found, you can't use TO_NATIVE() in this
> way, some compilers are complaining (like on s390x this doesn't build).
>
> So we need to do something like:
>
> /* Offset to next entry */
> tmp = 8 + name_len_padded
> tmp = TO_NATIVE(tmp);
>
> I'll do some additional tests with these changes and send an updated
> patch (for those that are interested).
>
> -Andrea
In practice, this is what I'm testing at the moment:
---
arch/powerpc/kernel/module_64.c | 5 +++--
include/linux/module.h | 8 +++++---
kernel/module/version.c | 22 ++++++++++-----------
scripts/export_report.pl | 9 +++++----
scripts/mod/modpost.c | 35 +++++++++++++++++++++++----------
5 files changed, 48 insertions(+), 31 deletions(-)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index ff045644f13f..ea6c830ed1e7 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -234,12 +234,13 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
static void dedotify_versions(struct modversion_info *vers,
unsigned long size)
{
- struct modversion_info *end;
+ struct modversion_info *end = (void *)vers + size;
- for (end = (void *)vers + size; vers < end; vers++)
+ for (; vers < end && vers->next; 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..4744901bdf63 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -34,9 +34,11 @@
#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];
+} __packed;
struct module;
struct exception_table_entry;
diff --git a/kernel/module/version.c b/kernel/module/version.c
index 53f43ac5a73e..5528f98c42dc 100644
--- a/kernel/module/version.c
+++ b/kernel/module/version.c
@@ -17,32 +17,30 @@ 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->next;
+ 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..55335ae98f4f 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,27 @@ 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 = 8 + name_len_padded;
+ tmp = TO_NATIVE(tmp);
+ 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");
+ /* Always end with a dummy NULL entry */
+ buf_printf(b, "\t\"\\x00\\x00\\x00\\x00\\x00\\x00\\x00\\x00\";\n");
}
static void add_depends(struct buffer *b, struct module *mod)
--
2.39.2
next prev parent reply other threads:[~2023-03-14 14:59 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-11 16:11 [PATCH] modpost: support arbitrary symbol length in modversion Gary Guo
2023-01-12 21:40 ` Lucas De Marchi
2023-01-13 18:18 ` Gary Guo
2023-01-17 17:51 ` Michal Suchánek
2023-01-17 19:22 ` Lucas De Marchi
2023-01-18 7:01 ` Masahiro Yamada
2023-01-19 19:02 ` Lucas De Marchi
2023-01-19 19:19 ` Miguel Ojeda
2023-01-19 15:09 ` Gary Guo
2023-01-19 15:18 ` Michal Suchánek
2023-01-19 15:57 ` Gary Guo
2023-03-13 20:32 ` Andrea Righi
2023-03-13 21:48 ` Michal Suchánek
2023-03-13 21:53 ` Andrea Righi
2023-03-13 22:02 ` Michal Suchánek
2023-03-13 22:09 ` Andrea Righi
2023-03-14 14:38 ` Andrea Righi
2023-03-14 14:59 ` Andrea Righi [this message]
2023-03-15 0:15 ` Vincenzo Palazzo
2023-03-15 6:07 ` Andrea Righi
2023-03-15 16:18 ` Masahiro Yamada
2023-03-15 21:00 ` Andrea Righi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZBCL1G+8z8c6zLI0@righiandr-XPS-13-7390 \
--to=andrea.righi@canonical.com \
--cc=Julia.Lawall@inria.fr \
--cc=alex.gaynor@gmail.com \
--cc=bjorn3_gh@protonmail.com \
--cc=boqun.feng@gmail.com \
--cc=gary@garyguo.net \
--cc=guozhengkui@vivo.com \
--cc=joel@jms.id.au \
--cc=keescook@chromium.org \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-modules@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=masahiroy@kernel.org \
--cc=mcgrof@kernel.org \
--cc=msuchanek@suse.de \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=nicolas@fjasle.eu \
--cc=npiggin@gmail.com \
--cc=ojeda@kernel.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=wedsonaf@gmail.com \
--cc=wedsonaf@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).