On 06/13/2016 04:59 PM, Rasmus Villemoes wrote:
> On Mon, Jun 13 2016, Prarit Bhargava <pra...@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 <pra...@redhat.com>
>> Cc: Andrew Morton <a...@linux-foundation.org>
>> Cc: Thomas Gleixner <t...@linutronix.de>
>> Cc: Yang Shi <yang....@linaro.org>
>> Cc: Ingo Molnar <mi...@kernel.org>
>> Cc: Mel Gorman <mgor...@suse.de>
>> Cc: Rasmus Villemoes <li...@rasmusvillemoes.dk>
>> Cc: Kees Cook <keesc...@chromium.org>
>> Cc: Yaowei Bai <baiyao...@cmss.chinamobile.com>
>> Cc: Andrey Ryabinin <aryabi...@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
>
