[xyzzy@speakeasy.org: [PATCH] symbol_put_addr() locks kernel]

[Johannes Stezenbach]

Hi,

could you please have a look at the original message
and the attached patch?

Background:

Many people complain that one driver for a DVB PCI or
USB card depends on many frontend (== tuner + demodulator)
modules. Although one card usually has only one frontend,
we have these many dependencies because of the many existing
card variants.

As a way out, we try to replace a static dependency
in the frontend probe function with a combination of
symbol_request() and symbol_put_addr(). symbol_request()
would only be called for the frontends known to exists
for a given PCI or USB device id.

Thanks,
Johannes

----- Forwarded message from Trent Piepho <xyzzy@xxxxxxxxxxxxx> -----

Subject: [PATCH] symbol_put_addr() locks kernel
Date:	Fri, 28 Apr 2006 15:23:55 -0700 (PDT)
From: Trent Piepho <xyzzy@xxxxxxxxxxxxx>
To: linux-kernel@xxxxxxxxxxxxxxx
cc: xyzzy@xxxxxxxxxxxxx
X-Mailing-List:	linux-kernel@xxxxxxxxxxxxxxx

Please CC me on any replies, I'm not subscribed.

Even since a previous patch:

Fix race between CONFIG_DEBUG_SLABALLOC and modules
Sun, 27 Jun 2004 17:55:19 +0000 (17:55 +0000)
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commit;h=92b3db26d31cf21b70e3c1eadc56c179506d8fbe

The function symbol_put_addr() will deadlock the kernel.
symbol_put_addr() acquires the modlist_lock spinlock, then calls
kernel_text_address() and module_text_address() while it still holds
the lock.  That patch changed those two functions so they also try to
acquire the spinlock, which of course locks the kernel.

If you look at the original thread:  http://lkml.org/lkml/2004/6/23/20
The first patch corrected symbol_put_addr() to use the new
"double-underscore" functions that don't try to acquire the lock, but
the final patch missed symbol_put_addr().

Here is a patch which fixes this.  The locking inside
symbol_put_addr() is removed.  I changed symbol_put_addr() to call
core_kernel_text() instead of kernel_text_address(), the latter
includes an additional check if the addr is inside a module.  Since we
will call module_text_address() to get the module, this extra check
isn't needed.
# HG changeset patch
# User Trent Piepho <xyzzy@xxxxxxxxxxxxx>
# Node ID b66f3aa4bfe88f9ea2edb9c87419413ecc6caa8c
# Parent  4c3f241d7bc53fc25ddab54140f96cacd71ae1e1
From: Trent Piepho <xyzzy@xxxxxxxxxxxxx>

symbol_put_addr() would acquire modlist_lock, then while holding the lock call
two functions kernel_text_address() and module_text_address() which also try
to acquire the same lock.  This deadlocks the kernel of course.

This patch changes symbol_put_addr() to not acquire the modlist_lock, it
doesn't need it since it never looks at the module list directly.  Also, it
now uses core_kernel_text() instead of kernel_text_address().  The latter
has an additional check for addr inside a module, but we don't need to do that
since we call module_text_address() (the same function kernel_text_address
uses) ourselves.


Signed-off-by: Trent Piepho <xyzzy@xxxxxxxxxxxxx>

 
 include/linux/kernel.h |    1 +
 kernel/extable.c       |    2 +-
 kernel/module.c        |   14 +++++++-------
 3 files changed, 9 insertions(+), 8 deletions(-)

diff -r 4c3f241d7bc5 -r b66f3aa4bfe8 include/linux/kernel.h
--- a/include/linux/kernel.h	Fri Apr 28 23:00:35 2006 +0700
+++ b/include/linux/kernel.h	Fri Apr 28 14:49:34 2006 -0700
@@ -124,6 +124,7 @@ extern char *get_options(const char *str
 extern char *get_options(const char *str, int nints, int *ints);
 extern unsigned long long memparse(char *ptr, char **retptr);
 
+extern int core_kernel_text(unsigned long addr);
 extern int __kernel_text_address(unsigned long addr);
 extern int kernel_text_address(unsigned long addr);
 extern int session_of_pgrp(int pgrp);
diff -r 4c3f241d7bc5 -r b66f3aa4bfe8 kernel/extable.c
--- a/kernel/extable.c	Fri Apr 28 23:00:35 2006 +0700
+++ b/kernel/extable.c	Fri Apr 28 14:49:34 2006 -0700
@@ -40,7 +40,7 @@ const struct exception_table_entry *sear
 	return e;
 }
 
-static int core_kernel_text(unsigned long addr)
+int core_kernel_text(unsigned long addr)
 {
 	if (addr >= (unsigned long)_stext &&
 	    addr <= (unsigned long)_etext)
diff -r 4c3f241d7bc5 -r b66f3aa4bfe8 kernel/module.c
--- a/kernel/module.c	Fri Apr 28 23:00:35 2006 +0700
+++ b/kernel/module.c	Fri Apr 28 14:49:34 2006 -0700
@@ -705,14 +705,14 @@ EXPORT_SYMBOL(__symbol_put);
 
 void symbol_put_addr(void *addr)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&modlist_lock, flags);
-	if (!kernel_text_address((unsigned long)addr))
+	struct module *modaddr;
+
+	if (core_kernel_text((unsigned long)addr))
+		return;
+
+	if (!(modaddr = module_text_address((unsigned long)addr)))
 		BUG();
-
-	module_put(module_text_address((unsigned long)addr));
-	spin_unlock_irqrestore(&modlist_lock, flags);
+	module_put(modaddr);
 }
 EXPORT_SYMBOL_GPL(symbol_put_addr);
 


----- End forwarded message -----
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/