Re: [PATCH] kprobes, x86/alternatives: use text_mutex to protect smp_alt_modules

[Masami Hiramatsu <mhiramat@kernel.org>]

On Fri, 27 Oct 2017 17:34:44 +0800
Zhou Chengming <zhouchengming1@huawei.com> wrote:

> Fixes: 2cfa197 "ftrace/alternatives: Introducing *_text_reserved
> functions"
> 
> We use alternatives_text_reserved() to check if the address is in
> the fixed pieces of alternative reserved, but the problem is that
> we don't hold the smp_alt mutex when call this function. So the list
> traversal may encounter a deleted list_head if another path is doing
> alternatives_smp_module_del().
> 
> One solution is that we can hold smp_alt mutex before call this
> function, but the difficult point is that the callers of this
> functions, arch_prepare_kprobe() and arch_prepare_optimized_kprobe(),
> are called inside the text_mutex. So we must hold smp_alt mutex
> before we go into these arch dependent code. But we can't now,
> the smp_alt mutex is the arch dependent part, only x86 has it.
> Maybe we can export another arch dependent callback to solve this.
> 
> But there is a simpler way to handle this problem. We can reuse the
> text_mutex to protect smp_alt_modules instead of using another mutex.
> And all the arch dependent checks of kprobes are inside the text_mutex,
> so it's safe now.

OK, I considered other ways but those may introduce unneeded 
complexity. So this simple solution is better to fix this bug.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> 
> Signed-off-by: Zhou Chengming <zhouchengming1@huawei.com>
> ---
>  arch/x86/kernel/alternative.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 3344d33..55abbaa 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -442,7 +442,6 @@ static void alternatives_smp_lock(const s32 *start, const s32 *end,
>  {
>  	const s32 *poff;
>  
> -	mutex_lock(&text_mutex);
>  	for (poff = start; poff < end; poff++) {
>  		u8 *ptr = (u8 *)poff + *poff;
>  
> @@ -452,7 +451,6 @@ static void alternatives_smp_lock(const s32 *start, const s32 *end,
>  		if (*ptr == 0x3e)
>  			text_poke(ptr, ((unsigned char []){0xf0}), 1);
>  	}
> -	mutex_unlock(&text_mutex);
>  }
>  
>  static void alternatives_smp_unlock(const s32 *start, const s32 *end,
> @@ -460,7 +458,6 @@ static void alternatives_smp_unlock(const s32 *start, const s32 *end,
>  {
>  	const s32 *poff;
>  
> -	mutex_lock(&text_mutex);
>  	for (poff = start; poff < end; poff++) {
>  		u8 *ptr = (u8 *)poff + *poff;
>  
> @@ -470,7 +467,6 @@ static void alternatives_smp_unlock(const s32 *start, const s32 *end,
>  		if (*ptr == 0xf0)
>  			text_poke(ptr, ((unsigned char []){0x3E}), 1);
>  	}
> -	mutex_unlock(&text_mutex);
>  }
>  
>  struct smp_alt_module {
> @@ -489,8 +485,7 @@ struct smp_alt_module {
>  	struct list_head next;
>  };
>  static LIST_HEAD(smp_alt_modules);
> -static DEFINE_MUTEX(smp_alt);
> -static bool uniproc_patched = false;	/* protected by smp_alt */
> +static bool uniproc_patched = false;	/* protected by text_mutex */
>  
>  void __init_or_module alternatives_smp_module_add(struct module *mod,
>  						  char *name,
> @@ -499,7 +494,7 @@ void __init_or_module alternatives_smp_module_add(struct module *mod,
>  {
>  	struct smp_alt_module *smp;
>  
> -	mutex_lock(&smp_alt);
> +	mutex_lock(&text_mutex);
>  	if (!uniproc_patched)
>  		goto unlock;
>  
> @@ -526,14 +521,14 @@ void __init_or_module alternatives_smp_module_add(struct module *mod,
>  smp_unlock:
>  	alternatives_smp_unlock(locks, locks_end, text, text_end);
>  unlock:
> -	mutex_unlock(&smp_alt);
> +	mutex_unlock(&text_mutex);
>  }
>  
>  void __init_or_module alternatives_smp_module_del(struct module *mod)
>  {
>  	struct smp_alt_module *item;
>  
> -	mutex_lock(&smp_alt);
> +	mutex_lock(&text_mutex);
>  	list_for_each_entry(item, &smp_alt_modules, next) {
>  		if (mod != item->mod)
>  			continue;
> @@ -541,7 +536,7 @@ void __init_or_module alternatives_smp_module_del(struct module *mod)
>  		kfree(item);
>  		break;
>  	}
> -	mutex_unlock(&smp_alt);
> +	mutex_unlock(&text_mutex);
>  }
>  
>  void alternatives_enable_smp(void)
> @@ -551,7 +546,7 @@ void alternatives_enable_smp(void)
>  	/* Why bother if there are no other CPUs? */
>  	BUG_ON(num_possible_cpus() == 1);
>  
> -	mutex_lock(&smp_alt);
> +	mutex_lock(&text_mutex);
>  
>  	if (uniproc_patched) {
>  		pr_info("switching to SMP code\n");
> @@ -563,10 +558,13 @@ void alternatives_enable_smp(void)
>  					      mod->text, mod->text_end);
>  		uniproc_patched = false;
>  	}
> -	mutex_unlock(&smp_alt);
> +	mutex_unlock(&text_mutex);
>  }
>  
> -/* Return 1 if the address range is reserved for smp-alternatives */
> +/*
> + * Return 1 if the address range is reserved for smp-alternatives.
> + * Must hold text_mutex.
> + */
>  int alternatives_text_reserved(void *start, void *end)
>  {
>  	struct smp_alt_module *mod;
> -- 
> 1.8.3.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>