Re: [PATCH v2 1/2] kprobes: propagate error from arm_kprobe_ftrace()

[Masami Hiramatsu]

Hi Jessica,

Since this is under discussion about ftrace part, I just comment another one.

On Thu,  2 Nov 2017 17:33:33 +0100
Jessica Yu <jeyu@xxxxxxxxxx> wrote:

[...]
> @@ -1362,9 +1372,14 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p)
>  
>  	if (ret == 0 && kprobe_disabled(ap) && !kprobe_disabled(p)) {
>  		ap->flags &= ~KPROBE_FLAG_DISABLED;
> -		if (!kprobes_all_disarmed)
> +		if (!kprobes_all_disarmed) {
>  			/* Arm the breakpoint again. */
> -			arm_kprobe(ap);
> +			ret = arm_kprobe(ap);
> +			if (ret) {
> +				ap->flags |= KPROBE_FLAG_DISABLED;
> +				list_del_rcu(&p->list);

You also have to wait rcu here.

> +			}
> +		}
>  	}
>  	return ret;
>  }
[...]
> @@ -2428,16 +2452,26 @@ static void arm_all_kprobes(void)
>  	/* Arming kprobes doesn't optimize kprobe itself */
>  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>  		head = &kprobe_table[i];
> -		hlist_for_each_entry_rcu(p, head, hlist)
> -			if (!kprobe_disabled(p))
> -				arm_kprobe(p);
> +		/* Arm all kprobes on a best-effort basis */
> +		hlist_for_each_entry_rcu(p, head, hlist) {
> +			if (!kprobe_disabled(p)) {
> +				err = arm_kprobe(p);
> +				if (err)  {
> +					errors++;
> +					ret = err;
> +				}
> +			}
> +		}
>  	}
>  
> -	printk(KERN_INFO "Kprobes globally enabled\n");
> +	if (errors)
> +		pr_warn("Kprobes globally enabled, but failed to arm %d kprobes\n", errors);

The last "kprobes" should be "probes" because it can include kretprobes :P

Thank you,

-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>