Re: [PATCH 4/4] read string sections such as .modinfo with load_strings()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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

[Home]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Video Projectors]     [PDAs]     [Free Online Dating]     [Hacking TiVo]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Devices]     [Big List of Linux Books]     [16.7MP]

Add to Google Powered by Linux