Re: [PATCH] init, fix initcall blacklist for modules

[Prarit Bhargava <prarit@redhat.com>]

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
>