* [PATCH] init, fix initcall blacklist for modules
@ 2016-06-13 12:27 Prarit Bhargava
0 siblings, 0 replies; 5+ messages in thread
From: Prarit Bhargava @ 2016-06-13 12:27 UTC (permalink / raw)
To: linux-kernel; +Cc: Prarit Bhargava
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 <prarit@redhat.com>
---
init/main.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/init/main.c b/init/main.c
index 4c17fda5c2ff..09a795e91efe 100644
--- a/init/main.c
+++ b/init/main.c
@@ -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;
}
--
1.7.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH] init, fix initcall blacklist for modules
@ 2016-06-13 12:29 Prarit Bhargava
2016-06-13 20:59 ` Rasmus Villemoes
0 siblings, 1 reply; 5+ messages in thread
From: Prarit Bhargava @ 2016-06-13 12:29 UTC (permalink / raw)
To: linux-kernel
Cc: Prarit Bhargava, Andrew Morton, Thomas Gleixner, Yang Shi,
Ingo Molnar, Mel Gorman, Rasmus Villemoes, Kees Cook, Yaowei Bai,
Andrey Ryabinin
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
comparing module_init() function names. This patch resolves the issue by
comparing to the length of the function_name.
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yang Shi <yang.shi@linaro.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
init/main.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/init/main.c b/init/main.c
index 4c17fda5c2ff..09a795e91efe 100644
--- a/init/main.c
+++ b/init/main.c
@@ -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;
}
--
1.7.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] init, fix initcall blacklist for modules
2016-06-13 12:29 Prarit Bhargava
@ 2016-06-13 20:59 ` Rasmus Villemoes
2016-06-14 9:59 ` Prarit Bhargava
0 siblings, 1 reply; 5+ messages in thread
From: Rasmus Villemoes @ 2016-06-13 20:59 UTC (permalink / raw)
To: Prarit Bhargava
Cc: linux-kernel, Andrew Morton, Thomas Gleixner, Yang Shi,
Ingo Molnar, Mel Gorman, Kees Cook, Yaowei Bai, Andrey Ryabinin
On Mon, Jun 13 2016, Prarit Bhargava <prarit@redhat.com> wrote:
> 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?
> comparing module_init() function names. This patch resolves the issue by
> comparing to the length of the function_name.
>
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Yang Shi <yang.shi@linaro.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
> init/main.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/init/main.c b/init/main.c
> index 4c17fda5c2ff..09a795e91efe 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -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;
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).
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] init, fix initcall blacklist for modules
2016-06-13 20:59 ` Rasmus Villemoes
@ 2016-06-14 9:59 ` Prarit Bhargava
2016-06-14 22:31 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Prarit Bhargava @ 2016-06-14 9:59 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: linux-kernel, Andrew Morton, Thomas Gleixner, Yang Shi,
Ingo Molnar, Mel Gorman, Kees Cook, Yaowei Bai, Andrey Ryabinin
On 06/13/2016 04:59 PM, Rasmus Villemoes wrote:
> On Mon, Jun 13 2016, Prarit Bhargava <prarit@redhat.com> wrote:
>
>> 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?
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.
P.
>
>> comparing module_init() function names. This patch resolves the issue by
>> comparing to the length of the function_name.
>>
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Yang Shi <yang.shi@linaro.org>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Mel Gorman <mgorman@suse.de>
>> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
>> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> ---
>> init/main.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/init/main.c b/init/main.c
>> index 4c17fda5c2ff..09a795e91efe 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -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;
>
> 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).
>
>> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] init, fix initcall blacklist for modules
2016-06-14 9:59 ` Prarit Bhargava
@ 2016-06-14 22:31 ` Andrew Morton
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Morton @ 2016-06-14 22:31 UTC (permalink / raw)
To: Prarit Bhargava
Cc: Rasmus Villemoes, linux-kernel, Thomas Gleixner, Yang Shi,
Ingo Molnar, Mel Gorman, Kees Cook, Yaowei Bai, Andrey Ryabinin
On Tue, 14 Jun 2016 05:59:46 -0400 Prarit Bhargava <prarit@redhat.com> wrote:
> On 06/13/2016 04:59 PM, Rasmus Villemoes wrote:
> > On Mon, Jun 13 2016, Prarit Bhargava <prarit@redhat.com> wrote:
> >
> >> 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?
>
> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-14 22:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-13 12:27 [PATCH] init, fix initcall blacklist for modules Prarit Bhargava
2016-06-13 12:29 Prarit Bhargava
2016-06-13 20:59 ` Rasmus Villemoes
2016-06-14 9:59 ` Prarit Bhargava
2016-06-14 22:31 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).