init, fix initcall blacklist for modules

PATCH In review Prarit Bhargava 3 participants 4 messages Jun 13, 2016 – Jun 14, 2016

lore

Share & tools

lore

Prarit Bhargava

author

2016-06-13 12:29 UTC #0 lore raw

PATCH 1/1 init, fix initcall blacklist for modules

Sorry ... forgot to cc everyone on the last email.

----8<----

sprint_symbol_no_offset() returns the string "function_name [module_name]" where [module_name] is not printed for built in kernel functions. This means that the initcall blacklisting code will now always fail when comparing module_init() function names. This patch resolves the issue by comparing to the length of the function_name.

Signed-off-by Prarit Bhargava <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Yang Shi <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Rasmus Villemoes <[email protected]>

Cc Kees Cook <[email protected]>

Cc Yaowei Bai <[email protected]>

Cc Andrey Ryabinin <[email protected]>

---
 init/main.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

init/main.c +13 -1

index 4c17fda5c2ff..09a795e91efe 100644
@@ -708,14 +708,26 @@ static bool __init_or_module initcall_blacklisted(initcall_t fn)
 {
 	struct blacklist_entry *entry;
 	char fn_name[KSYM_SYMBOL_LEN];
+	char *space;
+	int length;
 
 	if (list_empty(&blacklisted_initcalls))
 		return false;
 
 	sprint_symbol_no_offset(fn_name, (unsigned long)fn);
+	/*
+	 * fn will be "function_name [module_name]" where [module_name] is not
+	 * displayed for built-in initcall functions.  Strip off the
+	 * [module_name].
+	 */
+	space = strchrnul(fn_name, ' ');
+	if (!space)
+		length = strlen(fn_name);
+	else
+		length = space - fn_name;
 
 	list_for_each_entry(entry, &blacklisted_initcalls, next) {
-		if (!strcmp(fn_name, entry->buf)) {
+		if (!strncmp(fn_name, entry->buf, length)) {
 			pr_debug("initcall %s blacklisted\n", fn_name);
 			return true;
 		}

Signature · 1 line

1.7.9.3

Rasmus Villemoes

2016-06-13 20:59 UTC #1 lore raw

Quoted 9 lines · Prarit Bhargava

Sorry ... forgot to cc everyone on the last email.
P.
----8<----
sprint_symbol_no_offset() returns the string "function_name [module_name]" where [module_name] is not printed for built in kernel functions. This means that the initcall blacklisting code will now always fail when

I was and am pretty sure that %pf ends up using sprint_symbol_no_offset(), so I don't see how this is new. But maybe "now" doesn't refer to c8cdd2be21?

Quoted 32 lines

comparing module_init() function names. This patch resolves the issue by comparing to the length of the function_name.
Signed-off-by Prarit Bhargava <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Yang Shi <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rasmus Villemoes <[email protected]>
Cc Kees Cook <[email protected]>
Cc Yaowei Bai <[email protected]>
Cc Andrey Ryabinin <[email protected]>
---
 init/main.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
init/main.c +2 -0
index 4c17fda5c2ff..09a795e91efe 100644
@@ -708,14 +708,26 @@ static bool __init_or_module initcall_blacklisted(initcall_t fn)
 {
 	struct blacklist_entry *entry;
 	char fn_name[KSYM_SYMBOL_LEN];
+	char *space;
+	int length;
 
 	if (list_empty(&blacklisted_initcalls))
 		return false;
 
 	sprint_symbol_no_offset(fn_name, (unsigned long)fn);

init/main.c static bool __init_or_module initcall_blacklisted(initcall_t fn) · line 727

Quoted 10 lines

init/main.c +10 -0

+	/*
+	 * fn will be "function_name [module_name]" where [module_name] is not
+	 * displayed for built-in initcall functions.  Strip off the
+	 * [module_name].
+	 */
+	space = strchrnul(fn_name, ' ');
+	if (!space)
+		length = strlen(fn_name);
+	else
+		length = space - fn_name;

strchrnul never returns NULL, so this could just be 'length = strchrnul(fn_name, ' ') - fn_name;'. But I don't think that's what you want anyway: Suppose one has blacklisted "init_foobar", and the function pointer resolves to a completely unrelated "init_foo", we'll end up falsely also blacklisting that since we're just comparing prefixes.

May I suggest

  strreplace(fn_name, ' ', '\0');

which also seems to match the comment a little better (and eliminates the extra variables and the hunk below).

init/main.c static bool __init_or_module initcall_blacklisted(initcall_t fn) · line 733

Quoted 6 lines

init/main.c +1 -1

 	list_for_each_entry(entry, &blacklisted_initcalls, next) {
-		if (!strcmp(fn_name, entry->buf)) {
+		if (!strncmp(fn_name, entry->buf, length)) {
 			pr_debug("initcall %s blacklisted\n", fn_name);
 			return true;
 		}

Rasmus

Prarit Bhargava

author

2016-06-14 09:59 UTC #2 lore raw

Quoted 8 lines · Rasmus Villemoes

Quoted 6 lines · Prarit Bhargava
Sorry ... forgot to cc everyone on the last email.
P.
----8<----

Quoted 7 lines

Quoted 3 lines
sprint_symbol_no_offset() returns the string "function_name [module_name]" where [module_name] is not printed for built in kernel functions. This means that the initcall blacklisting code will now always fail when
I was and am pretty sure that %pf ends up using sprint_symbol_no_offset(), so I don't see how this is new. But maybe "now" doesn't refer to c8cdd2be21?

Oops. I can see how you read that that way. No, this isn't caused by or "Fixes:" c8cdd2be21. At some point "%pF" changed its behavior and blacklisting module_init() functions stopped working.

Quoted 66 lines

Quoted 33 lines
comparing module_init() function names. This patch resolves the issue by comparing to the length of the function_name.
Signed-off-by Prarit Bhargava <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Yang Shi <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rasmus Villemoes <[email protected]>
Cc Kees Cook <[email protected]>
Cc Yaowei Bai <[email protected]>
Cc Andrey Ryabinin <[email protected]>
---
 init/main.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
init/main.c +2 -0
index 4c17fda5c2ff..09a795e91efe 100644
@@ -708,14 +708,26 @@ static bool __init_or_module initcall_blacklisted(initcall_t fn)
 {
 	struct blacklist_entry *entry;
 	char fn_name[KSYM_SYMBOL_LEN];
+	char *space;
+	int length;
 
 	if (list_empty(&blacklisted_initcalls))
 		return false;
 
 	sprint_symbol_no_offset(fn_name, (unsigned long)fn);
init/main.c static bool __init_or_module initcall_blacklisted(initcall_t fn) · line 727
Quoted 10 lines
init/main.c +10 -0
+	/*
+	 * fn will be "function_name [module_name]" where [module_name] is not
+	 * displayed for built-in initcall functions.  Strip off the
+	 * [module_name].
+	 */
+	space = strchrnul(fn_name, ' ');
+	if (!space)
+		length = strlen(fn_name);
+	else
+		length = space - fn_name;
strchrnul never returns NULL, so this could just be 'length = strchrnul(fn_name, ' ') - fn_name;'. But I don't think that's what you want anyway: Suppose one has blacklisted "init_foobar", and the function pointer resolves to a completely unrelated "init_foo", we'll end up falsely also blacklisting that since we're just comparing prefixes.
May I suggest
  strreplace(fn_name, ' ', '\0');
which also seems to match the comment a little better (and eliminates the extra variables and the hunk below).
init/main.c static bool __init_or_module initcall_blacklisted(initcall_t fn) · line 733
Quoted 6 lines
init/main.c +1 -1
 	list_for_each_entry(entry, &blacklisted_initcalls, next) {
-		if (!strcmp(fn_name, entry->buf)) {
+		if (!strncmp(fn_name, entry->buf, length)) {
 			pr_debug("initcall %s blacklisted\n", fn_name);
 			return true;
 		}
Rasmus

Andrew Morton

2016-06-14 22:31 UTC #3 lore raw

Quoted 13 lines · On Tue, 14 Jun 2016 05:59:46 -0400 Prarit Bhargava <[email protected]> wrote:

Quoted 12 lines · Rasmus Villemoes
Quoted 9 lines · Prarit Bhargava
Sorry ... forgot to cc everyone on the last email.
P.
----8<----
sprint_symbol_no_offset() returns the string "function_name [module_name]" where [module_name] is not printed for built in kernel functions. This means that the initcall blacklisting code will now always fail when

Quoted 7 lines

Quoted 3 lines
I was and am pretty sure that %pf ends up using sprint_symbol_no_offset(), so I don't see how this is new. But maybe "now" doesn't refer to c8cdd2be21?
Oops. I can see how you read that that way. No, this isn't caused by or "Fixes:" c8cdd2be21. At some point "%pF" changed its behavior and blacklisting module_init() functions stopped working.

Well can we please have a v2 with a complete changelog which identifies when the regression occurred? That will help in deciding which kernel versions need fixing.