Skip to contentSkip to conversation

KGDB: This allows for KGDB to better deal with autoloaded modules.

PATCH In review Jason Wessel 2 participants 4 messages Oct 15, 2007 – Jan 25, 2008 1 message not in the archive Module support

lore
Share & tools
lore

Jason Wessel

author
#0 lore raw

PATCH 16/21 KGDB: This allows for KGDB to better deal with autoloaded modules.

Signed-off-by Jason Wessel <[email protected]>

module.patch

From: Jason Wessel <[email protected]>
CC: [email protected]
Subject: [PATCH] This allows for KGDB to better deal with autoloaded modules.

With more information in the kernel, gdb can be modified in such a way as to automatically read the kernel module section information and allow for easy kernel module debugging. This patch can be found at:

ftp://ftp.<countrycode>.kernel.org/pub/linux/kernel/people/trini/patches/gdb/6.3/gdb-6.3-kgdb-module-notification.patch

In gdb the solib-search-path must contain the location of any module to be debugged. When a module is loaded GDB acts like a shared library has been loaded and will collect the information about the memory location so the kernel module can be debugged or inspected.

Even when not using kgdb+gdb, it is quite useful for a debugger+ICE/jtag to have the module section information.

Signed-off-by Milind Dumbare <[email protected]>
Signed-off-by Tom Rini <[email protected]>
Signed-off-by Jason Wessel <[email protected]>
---
 include/linux/module.h |   16 +++++++++++++
 kernel/module.c        |   60 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)
include/linux/module.h +16 -0
Index: linux-2.6-kgdb/include/linux/module.h
===================================================================
@@ -226,8 +226,17 @@ enum module_state
 	MODULE_STATE_LIVE,
 	MODULE_STATE_COMING,
 	MODULE_STATE_GOING,
+	MODULE_STATE_GONE,
 };
 
+#ifdef CONFIG_KGDB
+#define MAX_SECTNAME 31
+struct mod_section {
+	void *address;
+	char name[MAX_SECTNAME + 1];
+};
+#endif
+
 /* Similar stuff for section attributes. */
 struct module_sect_attr
 {

@@ -255,6 +264,13 @@ struct module
 	/* Unique handle for this module */
 	char name[MODULE_NAME_LEN];
 
+#ifdef CONFIG_KGDB
+	/* keep kgdb info at the begining so that gdb doesn't have a chance to
+	 * miss out any fields */
+	unsigned long num_sections;
+	struct mod_section *mod_sections;
+#endif
+
 	/* Sysfs stuff. */
 	struct module_kobject mkobj;
 	struct module_param_attrs *param_attrs;
kernel/module.c +60 -0

114 lines not on this page · raw message

#1 lore raw
Quoted 2 lines · Jason Wessel
Signed-off-by Jason Wessel <[email protected]>

...

kernel/module.c extern int module_sysfs_initialized; · line 68

Quoted 7 lines
kernel/module.c +1 -0
@@ -65,6 +65,7 @@ extern int module_sysfs_initialized;
  * (add/delete uses stop_machine). */
 static DEFINE_MUTEX(module_mutex);
 static LIST_HEAD(modules);
+static DECLARE_MUTEX(notify_mutex);

These days, notify_mutex should better be a real mutex.

Signed-off-by Jan Kiszka <[email protected]>
---
 kernel/module.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
kernel/module.c +7 -7
Index: b/kernel/module.c
===================================================================
@@ -66,7 +66,7 @@ extern int module_sysfs_initialized;
  * (add/delete uses stop_machine). */
 static DEFINE_MUTEX(module_mutex);
 static LIST_HEAD(modules);
-static DECLARE_MUTEX(notify_mutex);
+static DEFINE_MUTEX(notify_mutex);
 
 static BLOCKING_NOTIFIER_HEAD(module_notify_list);
 

@@ -712,10 +712,10 @@ sys_delete_module(const char __user *nam
 	if (ret != 0)
 		goto out;
 
-	down(&notify_mutex);
+	mutex_lock(&notify_mutex);
 	blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING,
 								 mod);
-	up(&notify_mutex);
+	mutex_unlock(&notify_mutex);
 
 
 	/* Never wait if forced. */

@@ -730,10 +730,10 @@ sys_delete_module(const char __user *nam
 	}
 	free_module(mod);
 
-	down(&notify_mutex);
+	mutex_lock(&notify_mutex);
 	blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GONE,
 			NULL);
-	up(&notify_mutex);
+	mutex_unlock(&notify_mutex);
 
  out:
 	mutex_unlock(&module_mutex);

@@ -2182,11 +2182,11 @@ sys_init_module(void __user *umod,
 		/* Init routine failed: abort.  Try to protect us from
                    buggy refcounters. */
 		mod->state = MODULE_STATE_GOING;
-		down(&notify_mutex);
+		mutex_lock(&notify_mutex);
 		blocking_notifier_call_chain(&module_notify_list,
 				MODULE_STATE_GOING,
 				mod);
-		up(&notify_mutex);
+		mutex_unlock(&notify_mutex);
 		synchronize_sched();
 		module_put(mod);
 		mutex_lock(&module_mutex);
author
#2 lore raw
Quoted 52 lines · Jan Kiszka

These days, notify_mutex should better be a real mutex.

Signed-off-by Jan Kiszka <[email protected]>
---
 kernel/module.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: b/kernel/module.c
===================================================================

kernel/module.c +5 -5
@@ -66,7 +66,7 @@ extern int module_sysfs_initialized;
  * (add/delete uses stop_machine). */
 static DEFINE_MUTEX(module_mutex);
 static LIST_HEAD(modules);
-static DECLARE_MUTEX(notify_mutex);
+static DEFINE_MUTEX(notify_mutex);
 
 static BLOCKING_NOTIFIER_HEAD(module_notify_list);
 

@@ -712,10 +712,10 @@ sys_delete_module(const char __user *nam
 	if (ret != 0)
 		goto out;
 
-	down(&notify_mutex);
+	mutex_lock(&notify_mutex);
 	blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING,
 								 mod);
-	up(&notify_mutex);
+	mutex_unlock(&notify_mutex);
 
 
 	/* Never wait if forced. */

@@ -730,10 +730,10 @@ sys_delete_module(const char __user *nam
 	}
 	free_module(mod);
 
-	down(&notify_mutex);
+	mutex_lock(&notify_mutex);
 	blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GONE,
 			NULL);
-	up(&notify_mutex);
+	mutex_unlock(&notify_mutex);
 
  out:
 	mutex_unlock(&module_mutex);

@@ -2182,11 +2182,11 @@ sys_init_module(void __user *umod,
 		/* Init routine failed: abort.  Try to protect us from
                    buggy refcounters. */
 		mod->state = MODULE_STATE_GOING;

kernel/module.c sys_init_module(void __user *umod, · line 2192

Quoted 10 lines
kernel/module.c +2 -2
-		down(&notify_mutex);
+		mutex_lock(&notify_mutex);
 		blocking_notifier_call_chain(&module_notify_list,
 				MODULE_STATE_GOING,
 				mod);
-		up(&notify_mutex);
+		mutex_unlock(&notify_mutex);
 		synchronize_sched();
 		module_put(mod);
 		mutex_lock(&module_mutex);

Jan,

This looks fine.

I committed this to the latest development branch as well as the 2.6.24 branch. It will show up in the kernel.org & cvs repository later today.

Thanks,
Jason.

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#4 lore raw
Quoted 87 lines · Rusty Russell
Quoted 3 lines · On Tuesday 16 October 2007 04:33:45 Jason Wessel wrote:

With more information in the kernel, gdb can be modified in such a way as to automatically read the kernel module section information and allow for easy kernel module debugging. This patch can be found at:

Hi Jason,

Some comments on your patch:

Review enum module_state · line 230

Quoted 6 lines
(quoted diff) +1 -0
@@ -226,8 +226,17 @@ enum module_state
        MODULE_STATE_LIVE,
        MODULE_STATE_COMING,
        MODULE_STATE_GOING,
+       MODULE_STATE_GONE,
 };

A new state is added, but it's not actually ever set in a module (it's used for notifiers). That's fine, but a comment is nice, and you don't need to add the printing code :)

Review enum module_state · line 238

Quoted 8 lines
(quoted diff) +8 -0
+#ifdef CONFIG_KGDB
+#define MAX_SECTNAME 31
+struct mod_section {
+       void *address;
+       char name[MAX_SECTNAME + 1];
+};
+#endif
+

We already have this information in "sect_attrs". It would be better to combine with that somehow, as far as I can tell.

Review enum module_state

Quoted 8 lines
(quoted diff) +5 -0
        }
        free_module(mod);
 
+       down(&notify_mutex);
+       blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GONE,
+                       NULL); 
+       up(&notify_mutex);
+

Err, that's horribly inconsistent (handing NULL). It would be far more useful before the free_module(), (or at least before the actual free), and handing the module arg.

Review static ssize_t show_initstate(struct mod · line 862

Quoted 10 lines
(quoted diff) +3 -0
@@ -842,6 +854,9 @@ static ssize_t show_initstate(struct mod
        case MODULE_STATE_GOING:
                state = "going";
                break;
+       case MODULE_STATE_GONE:
+               state = "gone";
+               break;
        }
        return sprintf(buffer, "%s\n", state);
 }

This is the printing code that can't happen...

Quoted 4 lines
(quoted diff) +4 -0
+#ifdef CONFIG_KGDB
+int add_modsects(struct module *mod, Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+               const char *secstrings)
+{

...

Review

Quoted 3 lines
(quoted diff) +3 -0
+       return 0;
+}
+#endif

This should be static, and should have a noop version under #else...

Quoted 5 lines
(quoted diff) +1 -0
+
 #ifdef CONFIG_KALLSYMS
 static int is_exported(const char *name, const struct module *mod)
 {

Review static struct module *load_module(void _ · line 1932

Quoted 9 lines
(quoted diff) +6 -0
@@ -1880,6 +1924,12 @@ static struct module *load_module(void _
 
        add_kallsyms(mod, sechdrs, symindex, strindex, secstrings);
 
+#ifdef CONFIG_KGDB
+       err = add_modsects(mod, hdr, sechdrs, secstrings);
+        if (err < 0)
+                goto nomodsectinfo;
+#endif
+

...so this that code doesn't need an ifdef.

Quoted 2 lines

Thanks!
Rusty.

Rusty thanks for all your comments.

I restructured the gdb kernel module awareness based on the consolidation of the KGDB section headers into the KALLSYMS requirement for section headers. The new patch to a non patched module.c and module.h is as follows. Below that patch, you will find the delta to the original patch included in the x86.git tree which will clean up the kgdb module code as desired.

Please feel free to further comment.

Thanks,
Jason.

-----------Patch against Clean kernel-------------

From: Jason Wessel <[email protected]>
CC: [email protected]
Subject: [PATCH] This allows for KGDB to better deal with autoloaded modules.

With more information in the kernel, gdb can be modified in such a way as to automatically read the kernel module section information and allow for easy kernel module debugging.

In gdb the solib-search-path must contain the location of any module to be debugged. When a module is loaded GDB acts like a shared library has been loaded and will collect the information about the memory location so the kernel module can be debugged or inspected.

Even when not using kgdb+gdb, it is quite useful for a debugger+ICE/jtag to have the module section information, which can be obtained by configure KGDB into the kernel or using CONFIG_KALLSYMS.

Signed-off-by Jason Wessel <[email protected]>
---
 include/linux/module.h |    9 ++++++---
 kernel/module.c        |   34 ++++++++++++++++++++++------------
 2 files changed, 28 insertions(+), 15 deletions(-)
kernel/module.c +22 -12
@@ -580,6 +580,9 @@ static void module_unload_free(struct mo
 			}
 		}
 	}
+	blocking_notifier_call_chain(&module_notify_list,
+				MODULE_STATE_GOING,
+				mod);
 }
 
 #ifdef CONFIG_MODULE_FORCE_UNLOAD

@@ -967,6 +970,7 @@ static unsigned long resolve_symbol(Elf_
  * /sys/module/foo/sections stuff
  * J. Corbet <[email protected]>
  */
+#if defined(CONFIG_KALLSYMS) || defined(CONFIG_KGDB)
 #ifdef CONFIG_KALLSYMS
 static ssize_t module_sect_show(struct module_attribute *mattr,
 				struct module *mod, char *buf)

@@ -975,6 +979,7 @@ static ssize_t module_sect_show(struct m
 		container_of(mattr, struct module_sect_attr, mattr);
 	return sprintf(buf, "0x%lx\n", sattr->address);
 }
+#endif /* CONFIG_KALLSYMS */
 
 static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
 {

@@ -1021,7 +1026,9 @@ static void add_sect_attrs(struct module
 		if (sattr->name == NULL)
 			goto out;
 		sect_attrs->nsections++;
+#ifdef CONFIG_KALLSYMS
 		sattr->mattr.show = module_sect_show;
+#endif
 		sattr->mattr.store = NULL;
 		sattr->mattr.attr.name = sattr->name;
 		sattr->mattr.attr.mode = S_IRUGO;

@@ -1049,7 +1056,20 @@ static void remove_sect_attrs(struct mod
 		mod->sect_attrs = NULL;
 	}
 }
+#else /* ! (CONFIG_KALLSYMS || CONFIG_KGDB) */
 
+static inline void add_sect_attrs(struct module *mod, unsigned int nsect,
+		char *sectstrings, Elf_Shdr *sechdrs)
+{
+}
+
+static inline void remove_sect_attrs(struct module *mod)
+{
+}
+
+#endif /* CONFIG_KALLSYMS || CONFIG_KGDB */
+
+#ifdef CONFIG_KALLSYMS
 /*
  * /sys/module/foo/notes/.section.name gives contents of SHT_NOTE sections.
  */

@@ -1143,18 +1163,7 @@ static void remove_notes_attrs(struct mo
 	if (mod->notes_attrs)
 		free_notes_attrs(mod->notes_attrs, mod->notes_attrs->notes);
 }
-
 #else
-
-static inline void add_sect_attrs(struct module *mod, unsigned int nsect,
-		char *sectstrings, Elf_Shdr *sechdrs)
-{
-}
-
-static inline void remove_sect_attrs(struct module *mod)
-{
-}
-
 static inline void add_notes_attrs(struct module *mod, unsigned int nsect,
 				   char *sectstrings, Elf_Shdr *sechdrs)
 {

@@ -1163,7 +1172,8 @@ static inline void add_notes_attrs(struc
 static inline void remove_notes_attrs(struct module *mod)
 {
 }
-#endif /* CONFIG_KALLSYMS */
+#endif
+
 
 #ifdef CONFIG_SYSFS
 int module_add_modinfo_attrs(struct module *mod)
include/linux/module.h +6 -3
@@ -342,13 +342,16 @@ struct module
 	unsigned long num_symtab;
 	char *strtab;
 
-	/* Section attributes */
-	struct module_sect_attrs *sect_attrs;
-
 	/* Notes attributes */
 	struct module_notes_attrs *notes_attrs;
 #endif
 
+#if defined(CONFIG_KALLSYMS) || defined(CONFIG_KGDB)
+	/* Section attributes */
+	struct module_sect_attrs *sect_attrs;
+#endif
+
+
 	/* Per-cpu data. */
 	void *percpu;

-----------Delta patch against x86.git tree------------- kgdb-module_cleanups.patch

From: Jason Wessel <[email protected]>
Subject: [PATCH][KGDB] debugger module interface cleanups

This patch removes all the duplicated module section information that KGDB added. This means that the debugger "kernel awareness" has to look at a different structure, but the kernel remains clean.

The only event required for a module that is being unloaded is to notify the debugger when the module has been removed from the module list.

Signed-off-by Jason Wessel <[email protected]>
---
 include/linux/module.h |   25 +++----------
 kernel/module.c        |   92 +++++++++++--------------------------------------
 2 files changed, 28 insertions(+), 89 deletions(-)
include/linux/module.h +6 -19
@@ -227,17 +227,8 @@ enum module_state
 	MODULE_STATE_LIVE,
 	MODULE_STATE_COMING,
 	MODULE_STATE_GOING,
-	MODULE_STATE_GONE,
 };
 
-#ifdef CONFIG_KGDB
-#define MAX_SECTNAME 31
-struct mod_section {
-	void *address;
-	char name[MAX_SECTNAME + 1];
-};
-#endif
-
 /* Similar stuff for section attributes. */
 struct module_sect_attr
 {

@@ -265,13 +256,6 @@ struct module
 	/* Unique handle for this module */
 	char name[MODULE_NAME_LEN];
 
-#ifdef CONFIG_KGDB
-	/* keep kgdb info at the begining so that gdb doesn't have a chance to
-	 * miss out any fields */
-	unsigned long num_sections;
-	struct mod_section *mod_sections;
-#endif
-
 	/* Sysfs stuff. */
 	struct module_kobject mkobj;
 	struct module_param_attrs *param_attrs;

@@ -358,13 +342,16 @@ struct module
 	unsigned long num_symtab;
 	char *strtab;
 
-	/* Section attributes */
-	struct module_sect_attrs *sect_attrs;
-
 	/* Notes attributes */
 	struct module_notes_attrs *notes_attrs;
 #endif
 
+#if defined(CONFIG_KALLSYMS) || defined(CONFIG_KGDB)
+	/* Section attributes */
+	struct module_sect_attrs *sect_attrs;
+#endif
+
+
 	/* Per-cpu data. */
 	void *percpu;
kernel/module.c +22 -70

191 lines not on this page · raw message