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

[Rasmus Villemoes]

On Mon, Jun 13 2016, Prarit Bhargava <prarit@xxxxxxxxxx> 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@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Yang Shi <yang.shi@xxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Mel Gorman <mgorman@xxxxxxx>
> Cc: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Yaowei Bai <baiyaowei@xxxxxxxxxxxxxxxxxxxx>
> Cc: Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx>
> ---
>  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