On Thu, 2009-05-21 at 17:50 +0100, Alan Jenkins wrote: > I don't mind what modinfo does; more lenient behaviour may be useful > for debugging. > > depmod I disagree with though. As a minimum, if the old depmod could > crash on a module then I want the new one to exit with an error code. > That should let e.g. RPM scripts pick it up in a similar way as > before. I don't think you're doing that at the moment. No you're right. The way I did it could for example have silently lost some module aliases ... > > I guess the scenario where it matters is a non-expert user installing > extra modules. If you're simply following instructions, you may not > know how to uninstall invalid modules. If the error is FATAL, you'll > still have a working system, but you won't be able add new modules in > future. > > Maybe I've lurked on too many forums, but I admit this sounds like a > realistic scenario :-). If depmod can return an error code as well, > then I'll be happy with a non-fatal ERROR. Hmm, it seems depmod is designed to just die. We'll have to save error codes for another day. I've reworked the patch (now it's two patches). load_strings() in depmod and modprobe call fatal() and modinfo calls error(). http://github.com/andr345/module-init-tools/commits/elf_cleanup3 is rebased too. >From 32971d375158804b34113d1359c9b726e1807f03 Mon Sep 17 00:00:00 2001 From: Andreas Robinson <andr345@xxxxxxxxx> Date: Thu, 21 May 2009 21:04:25 +0200 Subject: [PATCH] logging, modprobe: move errfn_t to logging.h Signed-off-by: Andreas Robinson <andr345@xxxxxxxxx> --- logging.h | 2 ++ modprobe.c | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/logging.h b/logging.h index b7227bc..efa682f 100644 --- a/logging.h +++ b/logging.h @@ -18,6 +18,8 @@ extern void error(const char *fmt, ...); extern void warn(const char *fmt, ...); extern void info(const char *fmt, ...); +typedef void (*errfn_t)(const char *fmt, ...); + static inline void grammar(const char *cmd, const char *filename, unsigned int line) { diff --git a/modprobe.c b/modprobe.c index f5a8437..9f60e71 100644 --- a/modprobe.c +++ b/modprobe.c @@ -64,8 +64,6 @@ struct module { #define MODULE_DIR "/lib/modules" #endif -typedef void (*errfn_t)(const char *fmt, ...); - static void print_usage(const char *progname) { fprintf(stderr, -- 1.6.0.4 >From 528db92ab1dd0d75dba415b9f3dc81f5a34773ce Mon Sep 17 00:00:00 2001 From: Andreas Robinson <andr345@xxxxxxxxx> Date: Thu, 21 May 2009 22:10:38 +0200 Subject: [PATCH] read string sections such as .modinfo with load_strings() Changes to load_strings(): * export in struct module_ops * avoid buffer overruns by looking for a string terminator at the end of a section. * new errfn_t parameter selects error message type. If load_strings fails on a missing string terminator, the module is probably broken. Therefore, modprobe and depmod sets errfn_t error = fatal. Modinfo on the other hand sets errfn_t error = error. Signed-off-by: Andreas Robinson <andr345@xxxxxxxxx> --- depmod.c | 42 ++++++++++++++++++++++-------------------- elfops.h | 3 +++ elfops_core.c | 8 +++++--- modinfo.c | 25 +++++++++++++++---------- modprobe.c | 9 +++++---- 5 files changed, 50 insertions(+), 37 deletions(-) diff --git a/depmod.c b/depmod.c index a8c3a47..68fcaee 100644 --- a/depmod.c +++ b/depmod.c @@ -770,8 +770,8 @@ static void output_aliases(struct module *modules, FILE *out, char *dirname) { struct module *i; struct elf_file *file; - const char *p; - unsigned long size; + struct string_table *tbl; + int j; fprintf(out, "# Aliases extracted from modules themselves.\n"); for (i = modules; i; i = i->next) { @@ -781,19 +781,20 @@ static void output_aliases(struct module *modules, FILE *out, char *dirname) filename2modname(modname, i->pathname); /* Grab from old-style .modalias section. */ - for (p = file->ops->get_aliases(file, &size); - p; - p = next_string(p, &size)) - fprintf(out, "alias %s %s\n", p, modname); - - /* Grab form new-style .modinfo section. */ - for (p = file->ops->get_modinfo(file, &size); - p; - p = next_string(p, &size)) { + tbl = file->ops->load_strings(file, ".modalias", NULL, fatal); + for (j = 0; tbl && j < tbl->cnt; j++) + fprintf(out, "alias %s %s\n", tbl->str[j], modname); + strtbl_free(tbl); + + /* Grab from new-style .modinfo section. */ + tbl = file->ops->load_strings(file, ".modinfo", NULL, fatal); + for (j = 0; tbl && j < tbl->cnt; j++) { + const char *p = tbl->str[j]; if (strstarts(p, "alias=")) fprintf(out, "alias %s %s\n", p + strlen("alias="), modname); } + strtbl_free(tbl); } } @@ -801,9 +802,9 @@ static void output_aliases_bin(struct module *modules, FILE *out, char *dirname) { struct module *i; struct elf_file *file; - const char *p; + struct string_table *tbl; + int j; char *alias; - unsigned long size; struct index_node *index; int duplicate; @@ -816,10 +817,9 @@ static void output_aliases_bin(struct module *modules, FILE *out, char *dirname) filename2modname(modname, i->pathname); /* Grab from old-style .modalias section. */ - for (p = file->ops->get_aliases(file, &size); - p; - p = next_string(p, &size)) { - alias = NOFAIL(strdup(p)); + tbl = file->ops->load_strings(file, ".modalias", NULL, fatal); + for (j = 0; tbl && j < tbl->cnt; j++) { + alias = NOFAIL(strdup(tbl->str[j])); underscores(alias); duplicate = index_insert(index, alias, modname, i->order); if (duplicate && warn_dups) @@ -827,11 +827,12 @@ static void output_aliases_bin(struct module *modules, FILE *out, char *dirname) alias, modname); free(alias); } + strtbl_free(tbl); /* Grab from new-style .modinfo section. */ - for (p = file->ops->get_modinfo(file, &size); - p; - p = next_string(p, &size)) { + tbl = file->ops->load_strings(file, ".modinfo", NULL, fatal); + for (j = 0; tbl && j < tbl->cnt; j++) { + const char *p = tbl->str[j]; if (strstarts(p, "alias=")) { alias = NOFAIL(strdup(p + strlen("alias="))); underscores(alias); @@ -842,6 +843,7 @@ static void output_aliases_bin(struct module *modules, FILE *out, char *dirname) free(alias); } } + strtbl_free(tbl); } index_write(index, out); diff --git a/elfops.h b/elfops.h index 7069d92..c3d2e33 100644 --- a/elfops.h +++ b/elfops.h @@ -2,6 +2,7 @@ #define MODINITTOOLS_MODULEOPS_H #include <stdio.h> #include <stdint.h> +#include "logging.h" /* All the icky stuff to do with manipulating 64 and 32-bit modules belongs here. */ @@ -71,6 +72,8 @@ struct module_ops { void *(*load_section)(struct elf_file *module, const char *secname, unsigned long *secsize); + struct string_table *(*load_strings)(struct elf_file *module, + const char *secname, struct string_table *tbl, errfn_t error); struct string_table *(*load_symbols)(struct elf_file *module); struct string_table *(*load_dep_syms)(const char *pathname, struct elf_file *module, struct string_table **types); diff --git a/elfops_core.c b/elfops_core.c index 4283d65..c5e84ba 100644 --- a/elfops_core.c +++ b/elfops_core.c @@ -80,7 +80,8 @@ static void *PERBIT(load_section)(struct elf_file *module, static struct string_table *PERBIT(load_strings)(struct elf_file *module, const char *secname, - struct string_table *tbl) + struct string_table *tbl, + errfn_t error) { unsigned long size; const char *strings; @@ -108,11 +109,11 @@ static struct string_table *PERBIT(load_symbols)(struct elf_file *module) symtbl = NULL; /* New-style: strings are in this section. */ - symtbl = PERBIT(load_strings)(module, "__ksymtab_strings", symtbl); + symtbl = PERBIT(load_strings)(module, "__ksymtab_strings", symtbl, fatal); if (symtbl) { /* GPL symbols too */ return PERBIT(load_strings)(module, "__ksymtab_strings_gpl", - symtbl); + symtbl, fatal); } /* Old-style. */ @@ -338,6 +339,7 @@ static int PERBIT(dump_modversions)(struct elf_file *module) struct module_ops PERBIT(mod_ops) = { .load_section = PERBIT(load_section), + .load_strings = PERBIT(load_strings), .load_symbols = PERBIT(load_symbols), .load_dep_syms = PERBIT(load_dep_syms), .fetch_tables = PERBIT(fetch_tables), diff --git a/modinfo.c b/modinfo.c index d8412db..1c1b57e 100644 --- a/modinfo.c +++ b/modinfo.c @@ -50,9 +50,10 @@ static struct param *add_param(const char *name, struct param **list) return i; } -static void print_tag(const char *tag, const char *info, unsigned long size, +static void print_tag(const char *tag, struct string_table *tags, const char *filename, char sep) { + int j; unsigned int taglen = strlen(tag); if (streq(tag, "filename")) { @@ -60,18 +61,22 @@ static void print_tag(const char *tag, const char *info, unsigned long size, return; } - for (; info; info = next_string(info, &size)) + for (j = 0; j < tags->cnt; j++) { + const char *info = tags->str[j]; if (strncmp(info, tag, taglen) == 0 && info[taglen] == '=') printf("%s%c", info + taglen + 1, sep); + } } -static void print_all(const char *info, unsigned long size, +static void print_all(struct string_table *tags, const char *filename, char sep) { + int j; struct param *i, *params = NULL; printf("%-16s%s%c", "filename:", filename, sep); - for (; info; info = next_string(info, &size)) { + for (j = 0; j < tags->cnt; j++) { + const char *info = tags->str[j]; char *eq, *colon; /* We expect this in parm and parmtype. */ @@ -294,8 +299,7 @@ int main(int argc, char *argv[]) } for (opt = optind; opt < argc; opt++) { - void *info; - unsigned long infosize = 0; + struct string_table *tags; struct elf_file *mod; mod = grab_module(argv[opt], kernel, basedir); @@ -303,15 +307,16 @@ int main(int argc, char *argv[]) ret = 1; continue; } - info = mod->ops->get_modinfo(mod, &infosize); - if (!info) { + tags = mod->ops->load_strings(mod, ".modinfo", NULL, error); + if (!tags) { release_elf_file(mod); continue; } if (field) - print_tag(field, info, infosize, mod->pathname, sep); + print_tag(field, tags, mod->pathname, sep); else - print_all(info, infosize, mod->pathname, sep); + print_all(tags, mod->pathname, sep); + strtbl_free(tags); release_elf_file(mod); } return ret; diff --git a/modprobe.c b/modprobe.c index 9f60e71..a3cdbf6 100644 --- a/modprobe.c +++ b/modprobe.c @@ -324,15 +324,16 @@ static void rename_module(struct elf_file *module, static void clear_magic(struct elf_file *module) { - const char *p; - unsigned long len; + struct string_table *tbl; + int j; /* Old-style: __vermagic section */ module->ops->strip_section(module, "__vermagic"); /* New-style: in .modinfo section */ - p = module->ops->get_modinfo(module, &len); - for (; p; p = next_string(p, &len)) { + tbl = module->ops->load_strings(module, ".modinfo", NULL, fatal); + for (j = 0; tbl && j < tbl->cnt; j++) { + const char *p = tbl->str[j]; if (strstarts(p, "vermagic=")) { memset((char *)p, 0, strlen(p)); return; -- 1.6.0.4 -- To unsubscribe from this list: send the line "unsubscribe linux-modules" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
