Date:	Sat, 11 Jul 2009 12:00:14 +0200
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	Greg KH <gregkh@...e.de>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform_driver_register:  warn if probe is in
	.init.text

Hi Greg,

> > Are you conviced and took the patch or did you give up to convince me?
> 
> Heh, no, sorry, it got burried in my queue.
> 
> > I still think the patch is correct and I'd like to have it applied.
> 
> Ok, let's test it out in the linux-next tree for a while to make sure it
> works properly.  Care to send me an updated version?
I updated to latest Linus' master.  It applies to linux-next from
Fri Jul 10 14:44:30 2009 +1000 as is.

Back some time I sent a series that fixes many of these bugs.  I will
update it later today and resend.

Best regards and thanks,
Uwe

---->8----
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Subject: [PATCH] platform_driver_register:  warn if probe is in .init.text

with HOTPLUG=y it's wrong to register a platform_driver whose probe
function lives in .init.text because the probe function can be called
(e.g. via sysfs or by registering a device late) after the init sections
are already discarded.  This results in an oops.

So warn if such a driver is registered.

Without HOTPLUG the probe function can (and should) be discarded if the
driver is registered while the init sections are still available.
In this case warn if the probe function isn't discarded later.  (As
described in the comments there is a small chance for a wrong warning.
But as HOTPLUG=n is unusual today and the situation is strage enough to
be cleaned up anyhow, I think this is OK.)

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
---
 drivers/base/platform.c |   42 ++++++++++++++++++++++++++++++++++++------
 include/linux/kernel.h  |    2 ++
 include/linux/module.h  |   12 ++++++++++++
 kernel/extable.c        |   12 ++++++++++++
 kernel/module.c         |   36 ++++++++++++++++++++++++++++++++++++
 5 files changed, 98 insertions(+), 6 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 81cb01b..851ba84 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -470,11 +470,7 @@ static void platform_drv_shutdown(struct device *_dev)
 	drv->shutdown(dev);
 }
 
-/**
- * platform_driver_register
- * @drv: platform driver structure
- */
-int platform_driver_register(struct platform_driver *drv)
+static int __platform_driver_register(struct platform_driver *drv)
 {
 	drv->driver.bus = &platform_bus_type;
 	if (drv->probe)
@@ -489,6 +485,40 @@ int platform_driver_register(struct platform_driver *drv)
 
 	return driver_register(&drv->driver);
 }
+
+/**
+ * platform_driver_register
+ * @drv: platform driver structure
+ */
+int platform_driver_register(struct platform_driver *drv)
+{
+	int ret = __platform_driver_register(drv);
+
+#if defined(CONFIG_HOTPLUG)
+	/*
+	 * drivers that are registered by platform_driver_register
+	 * should not have their probe function in .init.text.  The
+	 * reason is that a probe can happen after .init.text is
+	 * discarded which then results in an oops.  The alternatives
+	 * are using .devinit.text for the probe function or "register"
+	 * with platform_driver_probe.
+	 */
+	if (drv->probe && kernel_init_text_address((unsigned long)drv->probe))
+		pr_warning("oops-warning: probe function of platform driver %s"
+			       " lives in .init.text\n", drv->driver.name);
+#else
+	/*
+	 * without HOTPLUG probe functions can be discarded after the driver is
+	 * loaded.
+	 * There is a little chance for false positives, namely if the driver is
+	 * registered after the .init sections are discarded.
+	 */
+	if (drv->probe && !kernel_init_text_address((unsigned long)drv->probe))
+		pr_info("probably the probe function of platform driver %s can"
+				" be moved to .init.text\n", drv->driver.name);
+#endif
+	return ret;
+}
 EXPORT_SYMBOL_GPL(platform_driver_register);
 
 /**
@@ -525,7 +555,7 @@ int __init_or_module platform_driver_probe(struct platform_driver *drv,
 
 	/* temporary section violation during probe() */
 	drv->probe = probe;
-	retval = code = platform_driver_register(drv);
+	retval = code = __platform_driver_register(drv);
 
 	/* Fixup that section violation, being paranoid about code scanning
 	 * the list of drivers in order to probe new devices.  Check to see
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d6320a3..298e9bc 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -203,8 +203,10 @@ extern char *get_options(const char *str, int nints, int *ints);
 extern unsigned long long memparse(const char *ptr, char **retptr);
 
 extern int core_kernel_text(unsigned long addr);
+extern int core_kernel_init_text(unsigned long addr)
 extern int __kernel_text_address(unsigned long addr);
 extern int kernel_text_address(unsigned long addr);
+extern int kernel_init_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
 
 struct pid;
diff --git a/include/linux/module.h b/include/linux/module.h
index 098bdb7..93f47c4 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -385,9 +385,11 @@ static inline int module_is_live(struct module *mod)
 }
 
 struct module *__module_text_address(unsigned long addr);
+struct module *__module_init_text_address(unsigned long addr);
 struct module *__module_address(unsigned long addr);
 bool is_module_address(unsigned long addr);
 bool is_module_text_address(unsigned long addr);
+bool is_module_init_text_address(unsigned long addr);
 
 static inline int within_module_core(unsigned long addr, struct module *mod)
 {
@@ -556,6 +558,11 @@ static inline struct module *__module_text_address(unsigned long addr)
 	return NULL;
 }
 
+static inline struct module *__module_init_text_address(unsigned long addr)
+{
+	return NULL;
+}
+
 static inline bool is_module_address(unsigned long addr)
 {
 	return false;
@@ -566,6 +573,11 @@ static inline bool is_module_text_address(unsigned long addr)
 	return false;
 }
 
+static inline bool is_module_init_text_address(unsigned long addr)
+{
+	return false;
+}
+
 /* Get/put a kernel symbol (calls should be symmetric) */
 #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak)); &(x); })
 #define symbol_put(x) do { } while(0)
diff --git a/kernel/extable.c b/kernel/extable.c
index 7f8f263..bfd7bda 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -66,6 +66,11 @@ int core_kernel_text(unsigned long addr)
 	    addr <= (unsigned long)_etext)
 		return 1;
 
+	return core_kernel_init_text;
+}
+
+int core_kernel_init_text(unsigned long addr)
+{
 	if (system_state == SYSTEM_BOOTING &&
 	    init_kernel_text(addr))
 		return 1;
@@ -98,6 +103,13 @@ int kernel_text_address(unsigned long addr)
 	return is_module_text_address(addr);
 }
 
+int kernel_init_text_address(unsigned long addr)
+{
+	if (core_kernel_init_text(addr))
+		return 1;
+	return is_module_init_text_address(addr);
+}
+
 /*
  * On some architectures (PPC64, IA64) function pointers
  * are actually only tokens to some data that then holds the
diff --git a/kernel/module.c b/kernel/module.c
index 0a04983..f1fbeb0 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2890,6 +2890,22 @@ bool is_module_text_address(unsigned long addr)
 }
 
 /*
+ * is_module_init_text_address - is this address inside a module's .init.text
+ * section?
+ * @addr: the address to check.
+ */
+bool is_module_init_text_address(unsigned long addr)
+{
+	bool ret;
+
+	preempt_disable();
+	ret = __module_init_text_address(addr) != NULL;
+	preempt_enable();
+
+	return ret;
+}
+
+/*
  * __module_text_address - get the module whose code contains an address.
  * @addr: the address.
  *
@@ -2909,6 +2925,26 @@ struct module *__module_text_address(unsigned long addr)
 }
 EXPORT_SYMBOL_GPL(__module_text_address);
 
+/*
+ * __module_init_text_address - get the module whose .init.text contains an
+ * address.
+ * @addr: the address.
+ *
+ * Must be called with preempt disabled or module mutex held so that
+ * module doesn't get freed during this.
+ */
+struct module *__module_init_text_address(unsigned long addr)
+{
+	struct module *mod = __module_address(addr);
+	if (mod) {
+		/* Make sure it's within the .init.text section. */
+		if (!within(addr, mod->module_init, mod->init_text_size))
+			mod = NULL;
+	}
+	return mod;
+}
+EXPORT_SYMBOL_GPL(__module_init_text_address);
+
 /* Don't grab lock, we're oopsing. */
 void print_modules(void)
 {
-- 
tg: (f00caa7..) t/platsection/warn_in_probe (depends on: linus/master)

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists