Re: [PATCH] modpost: Copy namespace string into 'struct symbol'
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/20190927093603.9140-2-yamada.masah...@socionext.com/
> 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();
> > > > 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
Re: [PATCH] modpost: Copy namespace string into 'struct symbol'
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/20190927093603.9140-2-yamada.masah...@socionext.com/
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();
>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
Re: [PATCH] modpost: Copy namespace string into 'struct symbol'
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();
> > 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.
Re: [PATCH] modpost: Copy namespace string into 'struct symbol'
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
Cc: Joel Fernandes (Google)
Cc: Greg Kroah-Hartman
Cc: Matthias Maennich
Cc: Jessica Yu
Signed-off-by: Shaun Ruffell
---
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();
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
Re: [PATCH] modpost: Copy namespace string into 'struct symbol'
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
> Cc: Joel Fernandes (Google)
> Cc: Greg Kroah-Hartman
> Cc: Matthias Maennich
> Cc: Jessica Yu
> Signed-off-by: Shaun Ruffell
> ---
>
> 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
[PATCH] modpost: Copy namespace string into 'struct symbol'
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
Cc: Joel Fernandes (Google)
Cc: Greg Kroah-Hartman
Cc: Matthias Maennich
Cc: Jessica Yu
Signed-off-by: Shaun Ruffell
---
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();
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
