[v2,2/8] printk: Provide pi_<level> / pe_<level> macros for __init / __exit code
diff mbox series

Message ID 1405176212-12175-3-git-send-email-minipli@googlemail.com
State New, archived
Headers show
Series
  • Mark literal strings in __init / __exit code
Related show

Commit Message

Mathias Krause July 12, 2014, 2:43 p.m. UTC
The memory used for functions marked with __init will be released after
initialization, albeit static data referenced by such code will not, if
not explicitly marked this way, too. This is especially true for format
strings used in messages printed by such code. Those are not marked and
therefore will survive the __init cleanup -- dangling in memory without
ever being referenced again.

Ideally we would like the compiler to automatically recognise such
constructs but it does not and it's not as simple as it might sound, as
strings used in initialization code might latter still be used, e.g. as
a slab cache name. Therefore we need to explicitly mark the strings we
want to release together with the function itself.

For a seamless integration of the necessary __init_str() / __exit_str()
macros to mark the format strings, this patch provides new variants of
the well known pr_<level>() macros as pi_<level>() for __init code and
pe_<level>() for __exit code. Changing existing code should thereby be
as simple as changing a single letter.

For code that cannot be changed to use the pi_<level>() / pe_<level>()
macros printk_init() and printk_exit() macros are provided, too.

One remark, though: We cannot provide appropriate p[ie]_debug() macros
for the CONFIG_DYNAMIC_DEBUG case as there is (currently) no way to
remove entries from dyndbg after initialization. But supporting that
scenario would require more work (and code), therefore not necessarily
justifying the memory savings.

Signed-off-by: Mathias Krause <minipli@googlemail.com>

---
v2: introduce printk_init / printk_exit macros (suggested by Joe)
---
 include/linux/init.h   |    7 ++++--
 include/linux/printk.h |   59 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 2 deletions(-)

Comments

Andrew Morton July 15, 2014, 11:23 p.m. UTC | #1
On Sat, 12 Jul 2014 16:43:26 +0200 Mathias Krause <minipli@googlemail.com> wrote:

> The memory used for functions marked with __init will be released after
> initialization, albeit static data referenced by such code will not, if
> not explicitly marked this way, too. This is especially true for format
> strings used in messages printed by such code. Those are not marked and
> therefore will survive the __init cleanup -- dangling in memory without
> ever being referenced again.
> 
> Ideally we would like the compiler to automatically recognise such
> constructs but it does not and it's not as simple as it might sound, as
> strings used in initialization code might latter still be used, e.g. as
> a slab cache name. Therefore we need to explicitly mark the strings we
> want to release together with the function itself.
> 
> For a seamless integration of the necessary __init_str() / __exit_str()
> macros to mark the format strings, this patch provides new variants of
> the well known pr_<level>() macros as pi_<level>() for __init code and
> pe_<level>() for __exit code. Changing existing code should thereby be
> as simple as changing a single letter.
> 
> For code that cannot be changed to use the pi_<level>() / pe_<level>()
> macros printk_init() and printk_exit() macros are provided, too.
> 
> One remark, though: We cannot provide appropriate p[ie]_debug() macros
> for the CONFIG_DYNAMIC_DEBUG case as there is (currently) no way to
> remove entries from dyndbg after initialization. But supporting that
> scenario would require more work (and code), therefore not necessarily
> justifying the memory savings.

I assume that if a programmer gets this wrong,
CONFIG_DEBUG_SECTION_MISMATCH will detect and report the error?

Please thoroughly test this if you have not done so.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathias Krause July 16, 2014, 6:29 a.m. UTC | #2
On Tue, Jul 15, 2014 at 04:23:30PM -0700, Andrew Morton wrote:
> On Sat, 12 Jul 2014 16:43:26 +0200 Mathias Krause <minipli@googlemail.com> wrote:
> 
> > The memory used for functions marked with __init will be released after
> > initialization, albeit static data referenced by such code will not, if
> > not explicitly marked this way, too. This is especially true for format
> > strings used in messages printed by such code. Those are not marked and
> > therefore will survive the __init cleanup -- dangling in memory without
> > ever being referenced again.
> > 
> > Ideally we would like the compiler to automatically recognise such
> > constructs but it does not and it's not as simple as it might sound, as
> > strings used in initialization code might latter still be used, e.g. as
> > a slab cache name. Therefore we need to explicitly mark the strings we
> > want to release together with the function itself.
> > 
> > For a seamless integration of the necessary __init_str() / __exit_str()
> > macros to mark the format strings, this patch provides new variants of
> > the well known pr_<level>() macros as pi_<level>() for __init code and
> > pe_<level>() for __exit code. Changing existing code should thereby be
> > as simple as changing a single letter.
> > 
> > For code that cannot be changed to use the pi_<level>() / pe_<level>()
> > macros printk_init() and printk_exit() macros are provided, too.
> > 
> > One remark, though: We cannot provide appropriate p[ie]_debug() macros
> > for the CONFIG_DYNAMIC_DEBUG case as there is (currently) no way to
> > remove entries from dyndbg after initialization. But supporting that
> > scenario would require more work (and code), therefore not necessarily
> > justifying the memory savings.
> 
> I assume that if a programmer gets this wrong,
> CONFIG_DEBUG_SECTION_MISMATCH will detect and report the error?

Yes it does. Very much the same as it detects wrong __init / __exit code
annotations. For wrong uses of the pi_*() / pe_*() helpers or manually
__init_str / __exit_str annotated strings modpost will detect all of the
following cases (8 wrong uses in total: 4 wrong pi_info / pe_info and 4
wrong __init_str / __exit_str annotations):

  void __init init_fun(void)
  {
  	pe_info("%s: Wrong use of pe_*() and __exit_str() in __init code\n",
  		__exit_str("init test"));
  }

  void normal_fun(void)
  {
  	pi_info("%s: Wrong use of pi_*() and __init_str() in normal code\n",
  		__init_str("normal test"));
  	pe_info("%s: Wrong use of pe_*() and __exit_str() in normal code\n",
  		__exit_str("normal test"));
  }

  void __exit exit_fun(void)
  {
  	pi_info("%s: Wrong use of pi_*() and __init_str() in __exit code\n",
  		__init_str("exit test"));
  }

Those will be detected either rather silently with the following message:

WARNING: modpost: Found 8 section mismatch(es).
To see full details build your kernel with:
'make CONFIG_DEBUG_SECTION_MISMATCH=y'

Or, with CONFIG_DEBUG_SECTION_MISMATCH=y, rather verbose:

WARNING: lib/test_module.o(.text+0x4): Section mismatch in reference from the function normal_fun() to the variable .init.rodata:__UNIQUE_ID__init_str_3
The function normal_fun() references
the variable __initconst __UNIQUE_ID__init_str_3.
This is often because normal_fun lacks a __initconst
annotation or the annotation of __UNIQUE_ID__init_str_3 is wrong.

WARNING: lib/test_module.o(.text+0xb): Section mismatch in reference from the function normal_fun() to the variable .init.rodata:__UNIQUE_ID__init_str_2
The function normal_fun() references
the variable __initconst __UNIQUE_ID__init_str_2.
This is often because normal_fun lacks a __initconst
annotation or the annotation of __UNIQUE_ID__init_str_2 is wrong.

WARNING: lib/test_module.o(.text+0x1c): Section mismatch in reference from the function normal_fun() to the variable .exit.data:__UNIQUE_ID__exit_str_5
The function normal_fun() references a variable in an exit section.
Often the variable __UNIQUE_ID__exit_str_5 has valid usage outside the exit section
and the fix is to remove the __exitdata annotation of __UNIQUE_ID__exit_str_5.

WARNING: lib/test_module.o(.text+0x25): Section mismatch in reference from the function normal_fun() to the variable .exit.data:__UNIQUE_ID__exit_str_4
The function normal_fun() references a variable in an exit section.
Often the variable __UNIQUE_ID__exit_str_4 has valid usage outside the exit section
and the fix is to remove the __exitdata annotation of __UNIQUE_ID__exit_str_4.

WARNING: lib/test_module.o(.init.text+0x4): Section mismatch in reference from the function init_fun() to the variable .exit.data:__UNIQUE_ID__exit_str_1
The function __init init_fun() references
a variable __exitdata __UNIQUE_ID__exit_str_1.
This is often seen when error handling in the init function
uses functionality in the exit path.
The fix is often to remove the __exitdata annotation of
__UNIQUE_ID__exit_str_1 so it may be used outside an exit section.

WARNING: lib/test_module.o(.init.text+0xb): Section mismatch in reference from the function init_fun() to the variable .exit.data:__UNIQUE_ID__exit_str_0
The function __init init_fun() references
a variable __exitdata __UNIQUE_ID__exit_str_0.
This is often seen when error handling in the init function
uses functionality in the exit path.
The fix is often to remove the __exitdata annotation of
__UNIQUE_ID__exit_str_0 so it may be used outside an exit section.

WARNING: lib/test_module.o(.exit.text+0x4): Section mismatch in reference from the function exit_fun() to the variable .init.rodata:__UNIQUE_ID__init_str_7
The function __exit exit_fun() references
a variable __initconst __UNIQUE_ID__init_str_7.
This is often seen when error handling in the exit function
uses functionality in the init path.
The fix is often to remove the __initconst annotation of
__UNIQUE_ID__init_str_7 so it may be used outside an init section.

WARNING: lib/test_module.o(.exit.text+0xb): Section mismatch in reference from the function exit_fun() to the variable .init.rodata:__UNIQUE_ID__init_str_6
The function __exit exit_fun() references
a variable __initconst __UNIQUE_ID__init_str_6.
This is often seen when error handling in the exit function
uses functionality in the init path.
The fix is often to remove the __initconst annotation of
__UNIQUE_ID__init_str_6 so it may be used outside an init section.


I'll see if I can make modpost detect the __UNIQUE_ID__init_str_* /
__UNIQUE_ID__exit_str_* variables and emit a more fitting message in
this case.

> 
> Please thoroughly test this if you have not done so.

I'll add the above in a more condensed form to the patch description as
this question came up for the second time, by now.


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

Patch
diff mbox series

diff --git a/include/linux/init.h b/include/linux/init.h
index 2c1cf10bb7..dc5691b2f3 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -39,14 +39,17 @@ 
  * For strings used in __init / __exit functions the __init_str() /
  * __exit_str() macros will take care of marking the strings accordingly so
  * they can be freed, too. Otherwise the strings would resist in memory, even
- * though they are no longer referenced.
+ * though they are no longer referenced. They're also used to implement the
+ * printk_init() / printk_exit() macros.
  *
  * Use them like this:
  *
  * static int __init my_setup(char *arg)
  * {
- *    if (!strcmp(arg, __init_str("disable")))
+ *    if (!strcmp(arg, __init_str("disable"))) {
+ *       printk_init("Disabled by commandline\n");
  *       enabled = false;
+ *   }
  * }
  * __setup("mydev=", my_setup);
  */
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 319ff7e53e..342b9e9501 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -226,6 +226,10 @@  extern asmlinkage void dump_stack(void) __cold;
  * All of these will print unconditionally, although note that pr_debug()
  * and other debug macros are compiled out unless either DEBUG is defined
  * or CONFIG_DYNAMIC_DEBUG is set.
+ *
+ * The printk_init() and pi_<level>() macros shall be used in __init code in
+ * favour of printk() / pr_<level>(). The printk_exit() and pe_<level>()
+ * variants shall be used in __exit code.
  */
 #define pr_emerg(fmt, ...) \
 	printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
@@ -245,13 +249,60 @@  extern asmlinkage void dump_stack(void) __cold;
 #define pr_cont(fmt, ...) \
 	printk(KERN_CONT fmt, ##__VA_ARGS__)
 
+#define printk_init(fmt, ...) \
+	printk(__init_str(fmt), ##__VA_ARGS__)
+#define printk_exit(fmt, ...) \
+	printk(__exit_str(fmt), ##__VA_ARGS__)
+
+#define pi_emerg(fmt, ...) \
+	printk_init(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_alert(fmt, ...) \
+	printk_init(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_crit(fmt, ...) \
+	printk_init(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_err(fmt, ...) \
+	printk_init(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_warning(fmt, ...) \
+	printk_init(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_warn pi_warning
+#define pi_notice(fmt, ...) \
+	printk_init(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_info(fmt, ...) \
+	printk_init(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_cont(fmt, ...) \
+	printk_init(KERN_CONT fmt, ##__VA_ARGS__)
+
+#define pe_emerg(fmt, ...) \
+	printk_exit(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_alert(fmt, ...) \
+	printk_exit(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_crit(fmt, ...) \
+	printk_exit(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_err(fmt, ...) \
+	printk_exit(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_warning(fmt, ...) \
+	printk_exit(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_warn pe_warning
+#define pe_notice(fmt, ...) \
+	printk_exit(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_info(fmt, ...) \
+	printk_exit(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_cont(fmt, ...) \
+	printk_exit(KERN_CONT fmt, ##__VA_ARGS__)
+
 /* pr_devel() should produce zero code unless DEBUG is defined */
 #ifdef DEBUG
 #define pr_devel(fmt, ...) \
 	printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_devel(fmt, ...) \
+	printk_init(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_devel(fmt, ...) \
+	printk_exit(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
 #else
 #define pr_devel(fmt, ...) \
 	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_devel pr_devel
+#define pe_devel pr_devel
 #endif
 
 #include <linux/dynamic_debug.h>
@@ -261,12 +312,20 @@  extern asmlinkage void dump_stack(void) __cold;
 /* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
 #define pr_debug(fmt, ...) \
 	dynamic_pr_debug(fmt, ##__VA_ARGS__)
+#define pi_debug pr_debug
+#define pe_debug pr_debug
 #elif defined(DEBUG)
 #define pr_debug(fmt, ...) \
 	printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_debug(fmt, ...) \
+	printk_init(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pe_debug(fmt, ...) \
+	printk_exit(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
 #else
 #define pr_debug(fmt, ...) \
 	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pi_debug pr_debug
+#define pe_debug pr_debug
 #endif
 
 /*