Add support for symbols that are exported into namespaces. For that,
extract any namespace suffix from the symbol name. In addition, emit a
warning whenever a module refers to an exported symbol without
explicitly importing the namespace that it is defined in. This patch
consistently adds the namespace suffix to symbol names exported into
Module.symvers.
Example warning emitted by modpost in case of the above violation:
WARNING: module ums-usbat uses symbol usb_stor_resume from namespace
USB_STORAGE, but does not import it.
Co-developed-by: Martijn Coenen <[email protected]>
Signed-off-by: Martijn Coenen <[email protected]>
Reviewed-by: Joel Fernandes (Google) <[email protected]>
Reviewed-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Matthias Maennich <[email protected]>
---
Documentation/kbuild/modules.rst | 7 ++-
scripts/export_report.pl | 2 +-
scripts/mod/modpost.c | 104 ++++++++++++++++++++++++++-----
scripts/mod/modpost.h | 7 +++
4 files changed, 101 insertions(+), 19 deletions(-)
diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
index 24e763482650..d2ae799237fd 100644
--- a/Documentation/kbuild/modules.rst
+++ b/Documentation/kbuild/modules.rst
@@ -470,9 +470,12 @@ build.
The syntax of the Module.symvers file is::
- <CRC> <Symbol> <module>
+ <CRC> <Symbol> <Namespace> <Module> <Export Type>
- 0x2d036834 scsi_remove_host drivers/scsi/scsi_mod
+ 0xe1cc2a05 usb_stor_suspend USB_STORAGE drivers/usb/storage/usb-storage EXPORT_SYMBOL_GPL
+
+ The fields are separated by tabs and values may be empty (e.g.
+ if no namespace is defined for an exported symbol).
For a kernel build without CONFIG_MODVERSIONS enabled, the CRC
would read 0x00000000.
diff --git a/scripts/export_report.pl b/scripts/export_report.pl
index 7d3030d03a25..548330e8c4e7 100755
--- a/scripts/export_report.pl
+++ b/scripts/export_report.pl
@@ -94,7 +94,7 @@ if (defined $opt{'o'}) {
#
while ( <$module_symvers> ) {
chomp;
- my (undef, $symbol, $module, $gpl) = split;
+ my (undef, $symbol, $namespace, $module, $gpl) = split('\t');
$SYMBOL { $symbol } = [ $module , "0" , $symbol, $gpl];
}
close($module_symvers);
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f277e116e0eb..c2d49afaea1c 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -164,6 +164,7 @@ struct symbol {
struct module *module;
unsigned int crc;
int crc_valid;
+ const char *namespace;
unsigned int weak:1;
unsigned int vmlinux:1; /* 1 if symbol is defined in vmlinux */
unsigned int kernel:1; /* 1 if symbol is from kernel
@@ -233,6 +234,37 @@ static struct symbol *find_symbol(const char *name)
return NULL;
}
+static bool contains_namespace(struct namespace_list *list,
+ const char *namespace)
+{
+ struct namespace_list *ns_entry;
+
+ for (ns_entry = list; ns_entry != NULL; ns_entry = ns_entry->next)
+ if (strcmp(ns_entry->namespace, namespace) == 0)
+ return true;
+
+ return false;
+}
+
+static void add_namespace(struct namespace_list **list, const char *namespace)
+{
+ struct namespace_list *ns_entry;
+
+ if (!contains_namespace(*list, namespace)) {
+ ns_entry = NOFAIL(malloc(sizeof(struct namespace_list) +
+ strlen(namespace) + 1));
+ strcpy(ns_entry->namespace, namespace);
+ ns_entry->next = *list;
+ *list = ns_entry;
+ }
+}
+
+static bool module_imports_namespace(struct module *module,
+ const char *namespace)
+{
+ return contains_namespace(module->imported_namespaces, namespace);
+}
+
static const struct {
const char *str;
enum export export;
@@ -312,23 +344,39 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec)
return export_unknown;
}
+static const char *sym_extract_namespace(const char **symname)
+{
+ size_t n;
+ char *dupsymname;
+
+ n = strcspn(*symname, ".");
+ if (n < strlen(*symname) - 1) {
+ dupsymname = NOFAIL(strdup(*symname));
+ dupsymname[n] = '\0';
+ *symname = dupsymname;
+ return dupsymname + n + 1;
+ }
+
+ return NULL;
+}
+
/**
* Add an exported symbol - it may have already been added without a
* CRC, in this case just update the CRC
**/
-static struct symbol *sym_add_exported(const char *name, struct module *mod,
- enum export export)
+static struct symbol *sym_add_exported(const char *name, const char *namespace,
+ struct module *mod, enum export export)
{
struct symbol *s = find_symbol(name);
if (!s) {
s = new_symbol(name, mod, export);
+ s->namespace = namespace;
} else {
if (!s->preloaded) {
- warn("%s: '%s' exported twice. Previous export "
- "was in %s%s\n", mod->name, name,
- s->module->name,
- is_vmlinux(s->module->name) ?"":".ko");
+ warn("%s: '%s' exported twice. Previous export was in %s%s\n",
+ mod->name, name, s->module->name,
+ is_vmlinux(s->module->name) ? "" : ".ko");
} else {
/* In case Module.symvers was out of date */
s->module = mod;
@@ -620,6 +668,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
unsigned int crc;
enum export export;
bool is_crc = false;
+ const char *name, *namespace;
if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
strstarts(symname, "__ksymtab"))
@@ -691,8 +740,9 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
default:
/* All exported symbols */
if (strstarts(symname, "__ksymtab_")) {
- sym_add_exported(symname + strlen("__ksymtab_"), mod,
- export);
+ name = symname + strlen("__ksymtab_");
+ namespace = sym_extract_namespace(&name);
+ sym_add_exported(name, namespace, mod, export);
}
if (strcmp(symname, "init_module") == 0)
mod->has_init = 1;
@@ -1943,6 +1993,7 @@ static void read_symbols(const char *modname)
const char *symname;
char *version;
char *license;
+ char *namespace;
struct module *mod;
struct elf_info info = { };
Elf_Sym *sym;
@@ -1974,6 +2025,12 @@ static void read_symbols(const char *modname)
license = get_next_modinfo(&info, "license", license);
}
+ namespace = get_modinfo(&info, "import_ns");
+ while (namespace) {
+ add_namespace(&mod->imported_namespaces, namespace);
+ namespace = get_next_modinfo(&info, "import_ns", namespace);
+ }
+
for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
symname = remove_dot(info.strtab + sym->st_name);
@@ -2118,6 +2175,13 @@ static int check_exports(struct module *mod)
basename++;
else
basename = mod->name;
+
+ if (exp->namespace &&
+ !module_imports_namespace(mod, exp->namespace)) {
+ warn("module %s uses symbol %s from namespace %s, but does not import it.\n",
+ basename, exp->name, exp->namespace);
+ }
+
if (!mod->gpl_compatible)
check_for_gpl_usage(exp->export, basename, exp->name);
check_for_unused(exp->export, basename, exp->name);
@@ -2341,7 +2405,7 @@ static void read_dump(const char *fname, unsigned int kernel)
return;
while ((line = get_next_line(&pos, file, size))) {
- char *symname, *modname, *d, *export, *end;
+ char *symname, *namespace, *modname, *d, *export, *end;
unsigned int crc;
struct module *mod;
struct symbol *s;
@@ -2349,7 +2413,10 @@ static void read_dump(const char *fname, unsigned int kernel)
if (!(symname = strchr(line, '\t')))
goto fail;
*symname++ = '\0';
- if (!(modname = strchr(symname, '\t')))
+ if (!(namespace = strchr(symname, '\t')))
+ goto fail;
+ *namespace++ = '\0';
+ if (!(modname = strchr(namespace, '\t')))
goto fail;
*modname++ = '\0';
if ((export = strchr(modname, '\t')) != NULL)
@@ -2366,7 +2433,8 @@ static void read_dump(const char *fname, unsigned int kernel)
mod = new_module(modname);
mod->skip = 1;
}
- s = sym_add_exported(symname, mod, export_no(export));
+ s = sym_add_exported(symname, namespace, mod,
+ export_no(export));
s->kernel = kernel;
s->preloaded = 1;
sym_update_crc(symname, mod, crc, export_no(export));
@@ -2395,16 +2463,20 @@ static void write_dump(const char *fname)
{
struct buffer buf = { };
struct symbol *symbol;
+ const char *namespace;
int n;
for (n = 0; n < SYMBOL_HASH_SIZE ; n++) {
symbol = symbolhash[n];
while (symbol) {
- if (dump_sym(symbol))
- buf_printf(&buf, "0x%08x\t%s\t%s\t%s\n",
- symbol->crc, symbol->name,
- symbol->module->name,
- export_str(symbol->export));
+ if (dump_sym(symbol)) {
+ namespace = symbol->namespace;
+ buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
+ symbol->crc, symbol->name,
+ namespace ? namespace : "",
+ symbol->module->name,
+ export_str(symbol->export));
+ }
symbol = symbol->next;
}
}
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 8453d6ac2f77..9626bf3e7424 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -109,6 +109,11 @@ buf_printf(struct buffer *buf, const char *fmt, ...);
void
buf_write(struct buffer *buf, const char *s, int len);
+struct namespace_list {
+ struct namespace_list *next;
+ char namespace[0];
+};
+
struct module {
struct module *next;
const char *name;
@@ -121,6 +126,8 @@ struct module {
struct buffer dev_table_buf;
char srcversion[25];
int is_dot_o;
+ // Actual imported namespaces
+ struct namespace_list *imported_namespaces;
};
struct elf_info {
--
2.23.0.187.g17f5b7556c-goog
When building an out-of-tree module I was receiving many warnings from
modpost like:
WARNING: module dahdi_vpmadt032_loader uses symbol __kmalloc from namespace ts/dahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
WARNING: module dahdi_vpmadt032_loader uses symbol vpmadtreg_register from namespace linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
WARNING: module dahdi_vpmadt032_loader uses symbol param_ops_int from namespace ahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
WARNING: module dahdi_vpmadt032_loader uses symbol __init_waitqueue_head from namespace ux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
...
The fundamental issue appears to be that read_dump() is passing a
pointer to a statically allocated buffer for the namespace which is
reused as the file is parsed.
This change makes it so that 'struct symbol' holds a copy of the
namespace string in the same way that it holds a copy of the symbol
string. Because a copy is being made, handle_modversion can now free the
temporary copy
Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
Cc: Martijn Coenen <[email protected]>
Cc: Joel Fernandes (Google) <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Matthias Maennich <[email protected]>
Cc: Jessica Yu <[email protected]>
Signed-off-by: Shaun Ruffell <[email protected]>
---
Hi,
I didn't test that this change works with the namespaces, or investigate why
read_dump() is only called first while building out-of-tree modules, but it does
seem correct to me for the symbol to own the memory backing the namespace
string.
I also realize I'm jumping the gun a bit by testing against master before
5.4-rc1 is tagged.
Shaun
scripts/mod/modpost.c | 31 +++++++++++++++++++++++++++++--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 3961941e8e7a..349832ead200 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -364,6 +364,24 @@ static const char *sym_extract_namespace(const char **symname)
return NULL;
}
+static const char *dup_namespace(const char *namespace)
+{
+ if (!namespace || (namespace[0] == '\0'))
+ return NULL;
+ return NOFAIL(strdup(namespace));
+}
+
+static bool is_equal(const char *n1, const char *n2)
+{
+ if (n1 && !n2)
+ return false;
+ if (!n1 && n2)
+ return false;
+ if (!n1 && !n2)
+ return true;
+ return strcmp(n1, n2) == 0;
+}
+
/**
* Add an exported symbol - it may have already been added without a
* CRC, in this case just update the CRC
@@ -375,7 +393,7 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
if (!s) {
s = new_symbol(name, mod, export);
- s->namespace = namespace;
+ s->namespace = dup_namespace(namespace);
} else {
if (!s->preloaded) {
warn("%s: '%s' exported twice. Previous export was in %s%s\n",
@@ -384,6 +402,12 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
} else {
/* In case Module.symvers was out of date */
s->module = mod;
+
+ /* In case the namespace was out of date */
+ if (!is_equal(s->namespace, namespace)) {
+ free((char *)s->namespace);
+ s->namespace = dup_namespace(namespace);
+ }
}
}
s->preloaded = 0;
@@ -672,7 +696,6 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
unsigned int crc;
enum export export;
bool is_crc = false;
- const char *name, *namespace;
if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
strstarts(symname, "__ksymtab"))
@@ -744,9 +767,13 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
default:
/* All exported symbols */
if (strstarts(symname, "__ksymtab_")) {
+ const char *name, *namespace;
+
name = symname + strlen("__ksymtab_");
namespace = sym_extract_namespace(&name);
sym_add_exported(name, namespace, mod, export);
+ if (namespace)
+ free((char *)name);
}
if (strcmp(symname, "init_module") == 0)
mod->has_init = 1;
--
2.17.1
On Thu, Sep 26, 2019 at 05:24:46PM -0500, Shaun Ruffell wrote:
> When building an out-of-tree module I was receiving many warnings from
> modpost like:
>
> WARNING: module dahdi_vpmadt032_loader uses symbol __kmalloc from namespace ts/dahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> WARNING: module dahdi_vpmadt032_loader uses symbol vpmadtreg_register from namespace linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> WARNING: module dahdi_vpmadt032_loader uses symbol param_ops_int from namespace ahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> WARNING: module dahdi_vpmadt032_loader uses symbol __init_waitqueue_head from namespace ux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> ...
>
> The fundamental issue appears to be that read_dump() is passing a
> pointer to a statically allocated buffer for the namespace which is
> reused as the file is parsed.
>
> This change makes it so that 'struct symbol' holds a copy of the
> namespace string in the same way that it holds a copy of the symbol
> string. Because a copy is being made, handle_modversion can now free the
> temporary copy
>
> Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
> Cc: Martijn Coenen <[email protected]>
> Cc: Joel Fernandes (Google) <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Matthias Maennich <[email protected]>
> Cc: Jessica Yu <[email protected]>
> Signed-off-by: Shaun Ruffell <[email protected]>
> ---
>
> Hi,
>
> I didn't test that this change works with the namespaces, or investigate why
> read_dump() is only called first while building out-of-tree modules, but it does
> seem correct to me for the symbol to own the memory backing the namespace
> string.
>
> I also realize I'm jumping the gun a bit by testing against master before
> 5.4-rc1 is tagged.
Yes!!!
This fixes the issue that I reported to Mattias a few days ago on irc.
I am hitting this by just trying to build a single directory work of
modules:
make M=drivers/usb/
I just tested this patch and it works for me, thanks so much!
Tested-by: Greg Kroah-Hartman <[email protected]>
On Thu, Sep 26, 2019 at 05:24:46PM -0500, Shaun Ruffell wrote:
>When building an out-of-tree module I was receiving many warnings from
>modpost like:
>
> WARNING: module dahdi_vpmadt032_loader uses symbol __kmalloc from namespace ts/dahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> WARNING: module dahdi_vpmadt032_loader uses symbol vpmadtreg_register from namespace linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> WARNING: module dahdi_vpmadt032_loader uses symbol param_ops_int from namespace ahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> WARNING: module dahdi_vpmadt032_loader uses symbol __init_waitqueue_head from namespace ux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> ...
>
>The fundamental issue appears to be that read_dump() is passing a
>pointer to a statically allocated buffer for the namespace which is
>reused as the file is parsed.
Hi Shaun,
Thanks for working on this. I think you are right about the root cause
of this. I will have a closer look at your fix later today.
>This change makes it so that 'struct symbol' holds a copy of the
>namespace string in the same way that it holds a copy of the symbol
>string. Because a copy is being made, handle_modversion can now free the
>temporary copy
>
>Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
>Cc: Martijn Coenen <[email protected]>
>Cc: Joel Fernandes (Google) <[email protected]>
>Cc: Greg Kroah-Hartman <[email protected]>
>Cc: Matthias Maennich <[email protected]>
>Cc: Jessica Yu <[email protected]>
>Signed-off-by: Shaun Ruffell <[email protected]>
>---
>
>Hi,
>
>I didn't test that this change works with the namespaces, or investigate why
>read_dump() is only called first while building out-of-tree modules, but it does
>seem correct to me for the symbol to own the memory backing the namespace
>string.
>
>I also realize I'm jumping the gun a bit by testing against master before
>5.4-rc1 is tagged.
>
>Shaun
>
> scripts/mod/modpost.c | 31 +++++++++++++++++++++++++++++--
> 1 file changed, 29 insertions(+), 2 deletions(-)
>
>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>index 3961941e8e7a..349832ead200 100644
>--- a/scripts/mod/modpost.c
>+++ b/scripts/mod/modpost.c
>@@ -364,6 +364,24 @@ static const char *sym_extract_namespace(const char **symname)
> return NULL;
> }
>
>+static const char *dup_namespace(const char *namespace)
>+{
>+ if (!namespace || (namespace[0] == '\0'))
>+ return NULL;
>+ return NOFAIL(strdup(namespace));
>+}
>+
>+static bool is_equal(const char *n1, const char *n2)
>+{
>+ if (n1 && !n2)
>+ return false;
>+ if (!n1 && n2)
>+ return false;
>+ if (!n1 && !n2)
>+ return true;
>+ return strcmp(n1, n2) == 0;
>+}
>+
> /**
> * Add an exported symbol - it may have already been added without a
> * CRC, in this case just update the CRC
>@@ -375,7 +393,7 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
>
> if (!s) {
> s = new_symbol(name, mod, export);
>- s->namespace = namespace;
>+ s->namespace = dup_namespace(namespace);
> } else {
> if (!s->preloaded) {
> warn("%s: '%s' exported twice. Previous export was in %s%s\n",
>@@ -384,6 +402,12 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
> } else {
> /* In case Module.symvers was out of date */
> s->module = mod;
>+
>+ /* In case the namespace was out of date */
>+ if (!is_equal(s->namespace, namespace)) {
>+ free((char *)s->namespace);
>+ s->namespace = dup_namespace(namespace);
>+ }
> }
> }
> s->preloaded = 0;
>@@ -672,7 +696,6 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
> unsigned int crc;
> enum export export;
> bool is_crc = false;
>- const char *name, *namespace;
>
> if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
> strstarts(symname, "__ksymtab"))
>@@ -744,9 +767,13 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
> default:
> /* All exported symbols */
> if (strstarts(symname, "__ksymtab_")) {
>+ const char *name, *namespace;
>+
> name = symname + strlen("__ksymtab_");
> namespace = sym_extract_namespace(&name);
> sym_add_exported(name, namespace, mod, export);
>+ if (namespace)
>+ free((char *)name);
This probably should free namespace instead.
> }
> if (strcmp(symname, "init_module") == 0)
> mod->has_init = 1;
>--
>2.17.1
Cheers,
Matthias
On Fri, Sep 27, 2019 at 09:03:46AM +0100, Matthias Maennich wrote:
> On Thu, Sep 26, 2019 at 05:24:46PM -0500, Shaun Ruffell wrote:
> > When building an out-of-tree module I was receiving many warnings from
> > modpost like:
> >
> > WARNING: module dahdi_vpmadt032_loader uses symbol __kmalloc from namespace ts/dahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> > WARNING: module dahdi_vpmadt032_loader uses symbol vpmadtreg_register from namespace linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> > WARNING: module dahdi_vpmadt032_loader uses symbol param_ops_int from namespace ahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> > WARNING: module dahdi_vpmadt032_loader uses symbol __init_waitqueue_head from namespace ux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> > ...
> >
> > The fundamental issue appears to be that read_dump() is passing a
> > pointer to a statically allocated buffer for the namespace which is
> > reused as the file is parsed.
>
> Hi Shaun,
>
> Thanks for working on this. I think you are right about the root cause
> of this. I will have a closer look at your fix later today.
Thanks Matthias.
> > @@ -672,7 +696,6 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
> > unsigned int crc;
> > enum export export;
> > bool is_crc = false;
> > - const char *name, *namespace;
> >
> > if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
> > strstarts(symname, "__ksymtab"))
> > @@ -744,9 +767,13 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
> > default:
> > /* All exported symbols */
> > if (strstarts(symname, "__ksymtab_")) {
> > + const char *name, *namespace;
> > +
> > name = symname + strlen("__ksymtab_");
> > namespace = sym_extract_namespace(&name);
> > sym_add_exported(name, namespace, mod, export);
> > + if (namespace)
> > + free((char *)name);
>
> This probably should free namespace instead.
Given the implementation of sym_extract_namespace below, I believe
free((char *)name) is correct.
static const char *sym_extract_namespace(const char **symname)
{
size_t n;
char *dupsymname;
n = strcspn(*symname, ".");
if (n < strlen(*symname) - 1) {
dupsymname = NOFAIL(strdup(*symname));
dupsymname[n] = '\0';
*symname = dupsymname;
return dupsymname + n + 1;
}
return NULL;
}
I agree that freeing name instead of namespace is a little surprising
unless you know the implementation of sym_extract_namespace.
I thought about changing the the signature of sym_extract_namespace() to
make it clear when the symname is used to return a new allocation or
not, and given your comment, perhaps I should have.
On Mon, Sep 30, 2019 at 04:20:46PM -0500, Shaun Ruffell wrote:
>On Fri, Sep 27, 2019 at 09:03:46AM +0100, Matthias Maennich wrote:
>> On Thu, Sep 26, 2019 at 05:24:46PM -0500, Shaun Ruffell wrote:
>> > When building an out-of-tree module I was receiving many warnings from
>> > modpost like:
>> >
>> > WARNING: module dahdi_vpmadt032_loader uses symbol __kmalloc from namespace ts/dahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
>> > WARNING: module dahdi_vpmadt032_loader uses symbol vpmadtreg_register from namespace linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
>> > WARNING: module dahdi_vpmadt032_loader uses symbol param_ops_int from namespace ahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
>> > WARNING: module dahdi_vpmadt032_loader uses symbol __init_waitqueue_head from namespace ux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
>> > ...
>> >
>> > The fundamental issue appears to be that read_dump() is passing a
>> > pointer to a statically allocated buffer for the namespace which is
>> > reused as the file is parsed.
>>
>> Hi Shaun,
>>
>> Thanks for working on this. I think you are right about the root cause
>> of this. I will have a closer look at your fix later today.
>
>Thanks Matthias.
In the meantime, Masahiro came up with an alternative approach to
address this problem:
https://lore.kernel.org/lkml/[email protected]/
How do you think about it? It ignores the memory allocation problem that
you addressed as modpost is a host tool after all. As part of the patch
series, an alternative format for the namespace ksymtab entry is
suggested that also changes the way modpost has to deal with it.
>> > @@ -672,7 +696,6 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
>> > unsigned int crc;
>> > enum export export;
>> > bool is_crc = false;
>> > - const char *name, *namespace;
>> >
>> > if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
>> > strstarts(symname, "__ksymtab"))
>> > @@ -744,9 +767,13 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
>> > default:
>> > /* All exported symbols */
>> > if (strstarts(symname, "__ksymtab_")) {
>> > + const char *name, *namespace;
>> > +
>> > name = symname + strlen("__ksymtab_");
>> > namespace = sym_extract_namespace(&name);
>> > sym_add_exported(name, namespace, mod, export);
>> > + if (namespace)
>> > + free((char *)name);
>>
>> This probably should free namespace instead.
>
>Given the implementation of sym_extract_namespace below, I believe
>free((char *)name) is correct.
Yeah, you are right. I was just noticing the inconsistency and thought
it was obviously wrong. So, I was wrong. Sorry and thanks for the
explanation.
>
> static const char *sym_extract_namespace(const char **symname)
> {
> size_t n;
> char *dupsymname;
>
> n = strcspn(*symname, ".");
> if (n < strlen(*symname) - 1) {
> dupsymname = NOFAIL(strdup(*symname));
> dupsymname[n] = '\0';
> *symname = dupsymname;
> return dupsymname + n + 1;
> }
>
> return NULL;
> }
>
>I agree that freeing name instead of namespace is a little surprising
>unless you know the implementation of sym_extract_namespace.
>
>I thought about changing the the signature of sym_extract_namespace() to
>make it clear when the symname is used to return a new allocation or
>not, and given your comment, perhaps I should have.
I would rather follow-up with Masahiro's approach for now. What do you
think?
Cheers,
Matthias
On Tue, Oct 01, 2019 at 05:19:23PM +0100, Matthias Maennich wrote:
> On Mon, Sep 30, 2019 at 04:20:46PM -0500, Shaun Ruffell wrote:
> > On Fri, Sep 27, 2019 at 09:03:46AM +0100, Matthias Maennich wrote:
> > > On Thu, Sep 26, 2019 at 05:24:46PM -0500, Shaun Ruffell wrote:
> > > > When building an out-of-tree module I was receiving many warnings from
> > > > modpost like:
> > > >
> > > > WARNING: module dahdi_vpmadt032_loader uses symbol __kmalloc from namespace ts/dahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> > > > WARNING: module dahdi_vpmadt032_loader uses symbol vpmadtreg_register from namespace linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> > > > WARNING: module dahdi_vpmadt032_loader uses symbol param_ops_int from namespace ahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> > > > WARNING: module dahdi_vpmadt032_loader uses symbol __init_waitqueue_head from namespace ux/drivers/dahdi/dahdi-version.o: ..., but does not import it.
> > > > ...
> > > >
> > > > The fundamental issue appears to be that read_dump() is passing a
> > > > pointer to a statically allocated buffer for the namespace which is
> > > > reused as the file is parsed.
> > >
> > > Hi Shaun,
> > >
> > > Thanks for working on this. I think you are right about the root cause
> > > of this. I will have a closer look at your fix later today.
> >
> > Thanks Matthias.
>
> In the meantime, Masahiro came up with an alternative approach to
> address this problem:
> https://lore.kernel.org/lkml/[email protected]/
> How do you think about it? It ignores the memory allocation problem that
> you addressed as modpost is a host tool after all. As part of the patch
> series, an alternative format for the namespace ksymtab entry is
> suggested that also changes the way modpost has to deal with it.
Masahiro's patch set looks good to me.
My only comment would be that I felt it preferable for
sym_add_exported() to treat the two string arguments passed to it the
same way. I feel the way it is currently violates the princple of least
surprise. However I accept this is just my personal opinion.
> > > > @@ -672,7 +696,6 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
> > > > unsigned int crc;
> > > > enum export export;
> > > > bool is_crc = false;
> > > > - const char *name, *namespace;
> > > >
> > > > if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
> > > > strstarts(symname, "__ksymtab"))
> > > > @@ -744,9 +767,13 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
> > > > default:
> > > > /* All exported symbols */
> > > > if (strstarts(symname, "__ksymtab_")) {
> > > > + const char *name, *namespace;
> > > > +
> > > > name = symname + strlen("__ksymtab_");
> > > > namespace = sym_extract_namespace(&name);
> > > > sym_add_exported(name, namespace, mod, export);
> > > > + if (namespace)
> > > > + free((char *)name);
> > >
> > > This probably should free namespace instead.
> >
> > Given the implementation of sym_extract_namespace below, I believe
> > free((char *)name) is correct.
>
> Yeah, you are right. I was just noticing the inconsistency and thought
> it was obviously wrong. So, I was wrong. Sorry and thanks for the
> explanation.
>
> >
> > static const char *sym_extract_namespace(const char **symname)
> > {
> > size_t n;
> > char *dupsymname;
> >
> > n = strcspn(*symname, ".");
> > if (n < strlen(*symname) - 1) {
> > dupsymname = NOFAIL(strdup(*symname));
> > dupsymname[n] = '\0';
> > *symname = dupsymname;
> > return dupsymname + n + 1;
> > }
> >
> > return NULL;
> > }
> >
> > I agree that freeing name instead of namespace is a little surprising
> > unless you know the implementation of sym_extract_namespace.
> >
> > I thought about changing the the signature of sym_extract_namespace() to
> > make it clear when the symname is used to return a new allocation or
> > not, and given your comment, perhaps I should have.
>
> I would rather follow-up with Masahiro's approach for now. What do you
> think?
I agree that following-up with Masahiro's patch set is the better
option.
Cheers,
Shaun