Re: [PATCH 3/7] kprobes: Propagate error from disarm_kprobe_ftrace()

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(2015/02/27 1:13), Petr Mladek wrote:
> Also disarm_kprobe_ftrace() could fail, for example if there is an internal
> error in the Kprobe code and we try to unregister some Kprobe that is not
> registered.

No, no registered kprobes are rejected at the beginning of __disable_kprobe.

And I'm not sure unregister_ftrace_function() and ftrace_set_filter_ip() can
fail if correctly set. That seems a bug in the kernel (not a normal
situation).

> If we fail to unregister the ftrace function, we still could try to disarm
> the Kprobe by removing the filter. This is why the first error code is not
> fatal and we try to continue.
> 
> __disable_kprobe() has to return the error code via ERR_PTR. It allows to
> pass -EINVAL instead of NULL for invalid Kprobes. Then the NULL pointer does
> not need to be transformed into -EINVAL later.
> 
> In addition, __disable_kprobe() should disable the child probe only when
> the parent probe has been successfully disabled. Note that we could always
> clear KPROBE_FLAG_DISABLED in case of error. It does not harm even when
> p == orig_p. It keeps the code nesting on reasonable level.
> 
> The error handling is a bit complicated in __unregister_kprobe_top().
> It is used in unregister_*probes() that are used in module removal callbacks.
> Any error here could not stop the removal of the module and the related Kprobe
> handlers. Therefore such an error is fatal. There is already a similar check
> in the "disarmed:" goto target.

OK.

> 
> The only exception is when we try to unregister an invalid Kprobe. It does not
> exist and therefore should not harm the system. This error was weak even before.
> 
> disable_kprobe() just passes the error as it did before.
> 
> disarm_all_kprobes() uses similar approach like arm_all_kprobes(). It keeps
> the current behavior and does the best effort. It tries to disable as many
> Kprobes as possible. It always reports success. It returns the last error
> code. There is going to be separate patch that will improve this behavior.
> 

OK, this looks good to me except for the comment above.
But could you also update this for the latest -tip tree?

Thank you,

> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> ---
>  kernel/kprobes.c | 77 +++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 51 insertions(+), 26 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index a69d23add983..ba57147bd52c 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -962,23 +962,26 @@ err_filter:
>  }
>  
>  /* Caller must lock kprobe_mutex */
> -static void disarm_kprobe_ftrace(struct kprobe *p)
> +static int disarm_kprobe_ftrace(struct kprobe *p)
>  {
> -	int ret;
> +	int ret = 0;
>  
> -	kprobe_ftrace_enabled--;
> -	if (kprobe_ftrace_enabled == 0) {
> +	if (kprobe_ftrace_enabled == 1) {
>  		ret = unregister_ftrace_function(&kprobe_ftrace_ops);
> -		WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
> +		WARN(ret < 0, "Failed to remove kprobe-ftrace (%d)\n", ret);
>  	}
> +	if (!ret)
> +		kprobe_ftrace_enabled--;
> +
>  	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
> -			   (unsigned long)p->addr, 1, 0);
> +				   (unsigned long)p->addr, 1, 0);
>  	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
> +	return ret;
>  }
>  #else	/* !CONFIG_KPROBES_ON_FTRACE */
>  #define prepare_kprobe(p)	arch_prepare_kprobe(p)
>  #define arm_kprobe_ftrace(p)	(0)
> -#define disarm_kprobe_ftrace(p)	do {} while (0)
> +#define disarm_kprobe_ftrace(p)	(0)
>  #endif
>  
>  /* Arm a kprobe with text_mutex */
> @@ -998,16 +1001,15 @@ static int arm_kprobe(struct kprobe *kp)
>  }
>  
>  /* Disarm a kprobe with text_mutex */
> -static void disarm_kprobe(struct kprobe *kp, bool reopt)
> +static int disarm_kprobe(struct kprobe *kp, bool reopt)
>  {
> -	if (unlikely(kprobe_ftrace(kp))) {
> -		disarm_kprobe_ftrace(kp);
> -		return;
> -	}
> +	if (unlikely(kprobe_ftrace(kp)))
> +		return disarm_kprobe_ftrace(kp);
>  	/* Ditto */
>  	mutex_lock(&text_mutex);
>  	__disarm_kprobe(kp, reopt);
>  	mutex_unlock(&text_mutex);
> +	return 0;
>  }
>  
>  /*
> @@ -1585,11 +1587,12 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
>  static struct kprobe *__disable_kprobe(struct kprobe *p)
>  {
>  	struct kprobe *orig_p;
> +	int err;
>  
>  	/* Get an original kprobe for return */
>  	orig_p = __get_valid_kprobe(p);
>  	if (unlikely(orig_p == NULL))
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  
>  	if (!kprobe_disabled(p)) {
>  		/* Disable probe if it is a child probe */
> @@ -1598,7 +1601,11 @@ static struct kprobe *__disable_kprobe(struct kprobe *p)
>  
>  		/* Try to disarm and disable this/parent probe */
>  		if (p == orig_p || aggr_kprobe_disabled(orig_p)) {
> -			disarm_kprobe(orig_p, true);
> +			err = disarm_kprobe(orig_p, true);
> +			if (err) {
> +				p->flags &= ~KPROBE_FLAG_DISABLED;
> +				return ERR_PTR(err);
> +			}
>  			orig_p->flags |= KPROBE_FLAG_DISABLED;
>  		}
>  	}
> @@ -1615,8 +1622,17 @@ static int __unregister_kprobe_top(struct kprobe *p)
>  
>  	/* Disable kprobe. This will disarm it if needed. */
>  	ap = __disable_kprobe(p);
> -	if (ap == NULL)
> -		return -EINVAL;
> +	/*
> +	 * We could not prevent Kprobe handlers from going. Therefore
> +	 * we rather do not continue when the Kprobe is still enabled.
> +	 * The only exception is an invalid Kprobe. It does not exist
> +	 * and therefore could not harm the system.
> +	 */
> +	if (IS_ERR(ap)) {
> +		if (PTR_ERR(ap) == -EINVAL)
> +			return PTR_ERR(ap);
> +		BUG();

Yeah, this must be done.

> +	}
>  
>  	if (ap == p)
>  		/*
> @@ -2012,12 +2028,14 @@ static void kill_kprobe(struct kprobe *p)
>  int disable_kprobe(struct kprobe *kp)
>  {
>  	int ret = 0;
> +	struct kprobe *p;
>  
>  	mutex_lock(&kprobe_mutex);
>  
>  	/* Disable this kprobe */
> -	if (__disable_kprobe(kp) == NULL)
> -		ret = -EINVAL;
> +	p = __disable_kprobe(kp);
> +	if (IS_ERR(p))
> +		ret = PTR_ERR(p);
>  
>  	mutex_unlock(&kprobe_mutex);
>  	return ret;
> @@ -2367,34 +2385,41 @@ already_enabled:
>  	return ret;
>  }
>  
> -static void disarm_all_kprobes(void)
> +static int disarm_all_kprobes(void)
>  {
>  	struct hlist_head *head;
>  	struct kprobe *p;
>  	unsigned int i;
> +	int err, ret = 0;
>  
>  	mutex_lock(&kprobe_mutex);
>  
>  	/* If kprobes are already disarmed, just return */
>  	if (kprobes_all_disarmed) {
>  		mutex_unlock(&kprobe_mutex);
> -		return;
> +		return 0;
>  	}
>  
> -	kprobes_all_disarmed = true;
> -	printk(KERN_INFO "Kprobes globally disabled\n");
> -
>  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>  		head = &kprobe_table[i];
>  		hlist_for_each_entry_rcu(p, head, hlist) {
> -			if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p))
> -				disarm_kprobe(p, false);
> +			if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p)) {
> +				err = disarm_kprobe(p, false);
> +				if (err)
> +					ret = err;
> +			}
>  		}
>  	}
> +
> +	kprobes_all_disarmed = true;
> +	pr_info("Kprobes globally disabled\n");
> +
>  	mutex_unlock(&kprobe_mutex);
>  
>  	/* Wait for disarming all kprobes by optimizer */
>  	wait_for_kprobe_optimizer();
> +
> +	return ret;
>  }
>  
>  /*
> @@ -2437,7 +2462,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
>  	case 'n':
>  	case 'N':
>  	case '0':
> -		disarm_all_kprobes();
> +		err = disarm_all_kprobes();
>  		break;
>  	default:
>  		return -EINVAL;
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com