[PATCH] kprobes: Print an error if probe is rejected

[PATCH] kprobes: Print an error if probe is rejected

[Naveen N. Rao]

When probing at different locations in the kernel, it is not always
evident if the location can be probed or not. As an example:

    $ perf probe __radix__flush_tlb_range:35
    Failed to write event: Invalid argument
      Error: Failed to add events.

The probed line above is:
     35         if (!mmu_has_feature(MMU_FTR_GTSE) && type == FLUSH_TYPE_GLOBAL) {

This ends up trying to probe on BUILD_BUG_ON(), which is rejected.
However, the user receives no indication at all as to why the probe
failed. Print an error in such cases so that it is clear that the probe
was rejected.

Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 kernel/kprobes.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index e41385afe79dc5..7c8929165924ed 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1542,8 +1542,10 @@ static int check_kprobe_address_safe(struct kprobe *p,
 	int ret;
 
 	ret = arch_check_ftrace_location(p);
-	if (ret)
+	if (ret) {
+		pr_err("kprobes: can't probe at the provided ftrace location\n");
 		return ret;
+	}
 	jump_label_lock();
 	preempt_disable();
 
@@ -1552,6 +1554,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
 	    within_kprobe_blacklist((unsigned long) p->addr) ||
 	    jump_label_text_reserved(p->addr, p->addr) ||
 	    find_bug((unsigned long)p->addr)) {
+		pr_err("kprobes: can't probe at address in reject list\n");
 		ret = -EINVAL;
 		goto out;
 	}
@@ -1976,8 +1979,10 @@ int register_kretprobe(struct kretprobe *rp)
 	void *addr;
 
 	ret = kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset);
-	if (ret)
+	if (ret) {
+		pr_err("kretprobes: can't probe at address outside function entry\n");
 		return ret;
+	}
 
 	/* If only rp->kp.addr is specified, check reregistering kprobes */
 	if (rp->kp.addr && warn_kprobe_rereg(&rp->kp))
@@ -1989,8 +1994,10 @@ int register_kretprobe(struct kretprobe *rp)
 			return PTR_ERR(addr);
 
 		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
-			if (kretprobe_blacklist[i].addr == addr)
+			if (kretprobe_blacklist[i].addr == addr) {
+				pr_err("kretprobes: can't probe at address in reject list\n");
 				return -EINVAL;
+			}
 		}
 	}
 

base-commit: 2e38eb04c95e5546b71bb86ee699a891c7d212b5
-- 
2.31.1

Re: [PATCH] kprobes: Print an error if probe is rejected

[Masami Hiramatsu]

Hi Naveen,

On Thu, 10 Jun 2021 14:26:17 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> When probing at different locations in the kernel, it is not always
> evident if the location can be probed or not. As an example:
> 
>     $ perf probe __radix__flush_tlb_range:35
>     Failed to write event: Invalid argument
>       Error: Failed to add events.
> 
> The probed line above is:
>      35         if (!mmu_has_feature(MMU_FTR_GTSE) && type == FLUSH_TYPE_GLOBAL) {
> 
> This ends up trying to probe on BUILD_BUG_ON(), which is rejected.
> However, the user receives no indication at all as to why the probe
> failed. Print an error in such cases so that it is clear that the probe
> was rejected.

Hmm, Nack for this way, but I understand that is a problem.
If you got the error in perf probe, which uses ftrace dynamic-event interface.
In that case, the errors should not be output in the dmesg, but are reported
via error_log in tracefs.
And kprobes itself should return different error code to the caller, instead
of printing error in dmesg. See below.

[...]
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1542,8 +1542,10 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  	int ret;
>  
>  	ret = arch_check_ftrace_location(p);
> -	if (ret)
> +	if (ret) {
> +		pr_err("kprobes: can't probe at the provided ftrace location\n");
>  		return ret;

This must be -EBUSY. (or arch depend return value)

> +	}
>  	jump_label_lock();
>  	preempt_disable();
>  
> @@ -1552,6 +1554,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  	    within_kprobe_blacklist((unsigned long) p->addr) ||
>  	    jump_label_text_reserved(p->addr, p->addr) ||
>  	    find_bug((unsigned long)p->addr)) {
> +		pr_err("kprobes: can't probe at address in reject list\n");
>  		ret = -EINVAL;

This maybe -EACCESS.

>  		goto out;
>  	}
> @@ -1976,8 +1979,10 @@ int register_kretprobe(struct kretprobe *rp)
>  	void *addr;
>  
>  	ret = kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset);
> -	if (ret)
> +	if (ret) {
> +		pr_err("kretprobes: can't probe at address outside function entry\n");
>  		return ret;

return -ERANGE.

> +	}
>  
>  	/* If only rp->kp.addr is specified, check reregistering kprobes */
>  	if (rp->kp.addr && warn_kprobe_rereg(&rp->kp))
> @@ -1989,8 +1994,10 @@ int register_kretprobe(struct kretprobe *rp)
>  			return PTR_ERR(addr);
>  
>  		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
> -			if (kretprobe_blacklist[i].addr == addr)
> +			if (kretprobe_blacklist[i].addr == addr) {
> +				pr_err("kretprobes: can't probe at address in reject list\n");
>  				return -EINVAL;

return -EACCESS too.

Thank you,

> +			}
>  		}
>  	}
>  
> 
> base-commit: 2e38eb04c95e5546b71bb86ee699a891c7d212b5
> -- 
> 2.31.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kprobes: Print an error if probe is rejected

[Steven Rostedt]

On Thu, 10 Jun 2021 19:16:43 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hmm, Nack for this way, but I understand that is a problem.
> If you got the error in perf probe, which uses ftrace dynamic-event interface.
> In that case, the errors should not be output in the dmesg, but are reported
> via error_log in tracefs.
> And kprobes itself should return different error code to the caller, instead
> of printing error in dmesg. See below.

We should update perf to use libtracefs that also has an interface to
read the error_log file.

-- Steve

Re: [PATCH] kprobes: Print an error if probe is rejected

[Masami Hiramatsu]

On Thu, 10 Jun 2021 13:33:46 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 10 Jun 2021 19:16:43 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hmm, Nack for this way, but I understand that is a problem.
> > If you got the error in perf probe, which uses ftrace dynamic-event interface.
> > In that case, the errors should not be output in the dmesg, but are reported
> > via error_log in tracefs.
> > And kprobes itself should return different error code to the caller, instead
> > of printing error in dmesg. See below.
> 
> We should update perf to use libtracefs that also has an interface to
> read the error_log file.

Hmm, it seems that libtracefs has no parser for the error_log file.

What we need is to parse error_log and find appropriate entry of
the error log. Thus, the interface will be;

int tracefs_add_dynamic_event(const char *command, char **error_log);

And the usage will be;

ret = tracefs_add_dynamic_event(command, &error_buf);
if (ret < 0) {
	pr_error("Failed to add dynamic event: %d\n", ret);
	pr_error("Error log:\n%s\n", error_buf);
	free(error_buf);
	return ret;
}
...
This is because error_log file keeps some previous error logs and
you need to find appropriate one from the actual command.

Maybe a part of perf probe (util/probe-file.c) should be shared with
libtracefs and finally it should moved on the libtracefs.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kprobes: Print an error if probe is rejected

[Naveen N. Rao]

Hi Masami,
Thanks for the review.


Masami Hiramatsu wrote:
> Hi Naveen,
> 
> On Thu, 10 Jun 2021 14:26:17 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> When probing at different locations in the kernel, it is not always
>> evident if the location can be probed or not. As an example:
>> 
>>     $ perf probe __radix__flush_tlb_range:35
>>     Failed to write event: Invalid argument
>>       Error: Failed to add events.
>> 
>> The probed line above is:
>>      35         if (!mmu_has_feature(MMU_FTR_GTSE) && type == FLUSH_TYPE_GLOBAL) {
>> 
>> This ends up trying to probe on BUILD_BUG_ON(), which is rejected.
>> However, the user receives no indication at all as to why the probe
>> failed. Print an error in such cases so that it is clear that the probe
>> was rejected.
> 
> Hmm, Nack for this way, but I understand that is a problem.
> If you got the error in perf probe, which uses ftrace dynamic-event interface.
> In that case, the errors should not be output in the dmesg, but are reported
> via error_log in tracefs.

That would be a nice thing to add to perf, but I don't see why this 
should be a either/or. I still think it is good to have the core kprobe 
infrastructure print such errors in the kernel log. It is easier to look 
up such error strings in the kernel source to understand why a probe was 
rejected.

We also have perf_event_open() as an interface to add probes, and I 
don't think it would be helpful to require all tools to utilize the 
error log from tracefs for this purpose.


- Naveen

Re: [PATCH] kprobes: Print an error if probe is rejected

[Steven Rostedt]

On Fri, 11 Jun 2021 19:25:38 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> We also have perf_event_open() as an interface to add probes, and I 
> don't think it would be helpful to require all tools to utilize the 
> error log from tracefs for this purpose.

The there should be a perf interface to read the errors. I agree with
Masami. Let's not have console logs for probe errors.

-- Steve

Re: [PATCH] kprobes: Print an error if probe is rejected

[Naveen N. Rao]

Steven Rostedt wrote:
> On Fri, 11 Jun 2021 19:25:38 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> We also have perf_event_open() as an interface to add probes, and I 
>> don't think it would be helpful to require all tools to utilize the 
>> error log from tracefs for this purpose.
> 
> The there should be a perf interface to read the errors. I agree with
> Masami. Let's not have console logs for probe errors.

Ok, understood.


Thanks,
Naveen

Re: [PATCH] kprobes: Print an error if probe is rejected

[Masami Hiramatsu]

Hi Naveen,

On Mon, 14 Jun 2021 21:00:52 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Steven Rostedt wrote:
> > On Fri, 11 Jun 2021 19:25:38 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> >> We also have perf_event_open() as an interface to add probes, and I 
> >> don't think it would be helpful to require all tools to utilize the 
> >> error log from tracefs for this purpose.
> > 
> > The there should be a perf interface to read the errors. I agree with
> > Masami. Let's not have console logs for probe errors.
> 
> Ok, understood.

Will you update this?

Thank you,

> 
> 
> Thanks,
> Naveen
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kprobes: Print an error if probe is rejected

[Naveen N. Rao]

Hi Masami,

Masami Hiramatsu wrote:
> Hi Naveen,
> 
> On Mon, 14 Jun 2021 21:00:52 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> Steven Rostedt wrote:
>> > On Fri, 11 Jun 2021 19:25:38 +0530
>> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>> > 
>> >> We also have perf_event_open() as an interface to add probes, and I 
>> >> don't think it would be helpful to require all tools to utilize the 
>> >> error log from tracefs for this purpose.
>> > 
>> > The there should be a perf interface to read the errors. I agree with
>> > Masami. Let's not have console logs for probe errors.
>> 
>> Ok, understood.
> 
> Will you update this?

Not sure if you are asking about error logging with perf_event_open(), 
or about reading error_log from tracefs.

For perf_event_open(), I think we will just need to return the error 
code, and interpret the return value in userspace. I don't think there 
is any other way to pass error log or strings back from the kernel.

I will update this series to return the correct return value as 
suggested.


Thanks,
Naveen

Re: [PATCH] kprobes: Print an error if probe is rejected

[Masami Hiramatsu]

On Mon, 21 Jun 2021 15:06:24 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Hi Masami,
> 
> Masami Hiramatsu wrote:
> > Hi Naveen,
> > 
> > On Mon, 14 Jun 2021 21:00:52 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> >> Steven Rostedt wrote:
> >> > On Fri, 11 Jun 2021 19:25:38 +0530
> >> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> >> > 
> >> >> We also have perf_event_open() as an interface to add probes, and I 
> >> >> don't think it would be helpful to require all tools to utilize the 
> >> >> error log from tracefs for this purpose.
> >> > 
> >> > The there should be a perf interface to read the errors. I agree with
> >> > Masami. Let's not have console logs for probe errors.
> >> 
> >> Ok, understood.
> > 
> > Will you update this?
> 
> Not sure if you are asking about error logging with perf_event_open(), 
> or about reading error_log from tracefs.
> 
> For perf_event_open(), I think we will just need to return the error 
> code, and interpret the return value in userspace. I don't think there 
> is any other way to pass error log or strings back from the kernel.

Yeah, I think that's good idea.

> 
> I will update this series to return the correct return value as 
> suggested.

OK, then I will update tracefs to interpret the code and put appropriate
error message on error_log.

Thank you!

> 
> 
> Thanks,
> Naveen
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kprobes: Print an error if probe is rejected

[Masami Hiramatsu]

Hi Naveen,

On Fri, 11 Jun 2021 19:25:38 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Hi Masami,
> Thanks for the review.
> 
> 
> Masami Hiramatsu wrote:
> > Hi Naveen,
> > 
> > On Thu, 10 Jun 2021 14:26:17 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> >> When probing at different locations in the kernel, it is not always
> >> evident if the location can be probed or not. As an example:
> >> 
> >>     $ perf probe __radix__flush_tlb_range:35
> >>     Failed to write event: Invalid argument
> >>       Error: Failed to add events.
> >> 
> >> The probed line above is:
> >>      35         if (!mmu_has_feature(MMU_FTR_GTSE) && type == FLUSH_TYPE_GLOBAL) {
> >> 
> >> This ends up trying to probe on BUILD_BUG_ON(), which is rejected.
> >> However, the user receives no indication at all as to why the probe
> >> failed. Print an error in such cases so that it is clear that the probe
> >> was rejected.
> > 
> > Hmm, Nack for this way, but I understand that is a problem.
> > If you got the error in perf probe, which uses ftrace dynamic-event interface.
> > In that case, the errors should not be output in the dmesg, but are reported
> > via error_log in tracefs.
> 
> That would be a nice thing to add to perf, but I don't see why this 
> should be a either/or. I still think it is good to have the core kprobe 
> infrastructure print such errors in the kernel log.

Yes, but that is only when if there is any unexpected errors.

For the expected error (e.g. rejecting user input), the design policy is
- kprobes API should return correct error code.
- kprobe tracefs I/F should return correct error code and put a human
  readable error mesage in the error_log.
Thus, the perf probe should decode the error code or reuse the error_log.

> It is easier to look 
> up such error strings in the kernel source to understand why a probe was 
> rejected.

I don't like to put a log message for rejecting user input on dmesg anymore.


> We also have perf_event_open() as an interface to add probes, and I 
> don't think it would be helpful to require all tools to utilize the 
> error log from tracefs for this purpose.

No, perf probe doesn't use perf-event interface to add probes. It uses
the tracefs for adding probes.

Thank you,


> 
> 
> - Naveen
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kprobes: Print an error if probe is rejected

[Naveen N. Rao]

Masami Hiramatsu wrote:
> Hi Naveen,
> 
> On Fri, 11 Jun 2021 19:25:38 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> Hi Masami,
>> Thanks for the review.
>> 
>> 
>> Masami Hiramatsu wrote:
>> > Hi Naveen,
>> > 
>> > On Thu, 10 Jun 2021 14:26:17 +0530
>> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>> > 
>> >> When probing at different locations in the kernel, it is not always
>> >> evident if the location can be probed or not. As an example:
>> >> 
>> >>     $ perf probe __radix__flush_tlb_range:35
>> >>     Failed to write event: Invalid argument
>> >>       Error: Failed to add events.
>> >> 
>> >> The probed line above is:
>> >>      35         if (!mmu_has_feature(MMU_FTR_GTSE) && type == FLUSH_TYPE_GLOBAL) {
>> >> 
>> >> This ends up trying to probe on BUILD_BUG_ON(), which is rejected.
>> >> However, the user receives no indication at all as to why the probe
>> >> failed. Print an error in such cases so that it is clear that the probe
>> >> was rejected.
>> > 
>> > Hmm, Nack for this way, but I understand that is a problem.
>> > If you got the error in perf probe, which uses ftrace dynamic-event interface.
>> > In that case, the errors should not be output in the dmesg, but are reported
>> > via error_log in tracefs.
>> 
>> That would be a nice thing to add to perf, but I don't see why this 
>> should be a either/or. I still think it is good to have the core kprobe 
>> infrastructure print such errors in the kernel log.
> 
> Yes, but that is only when if there is any unexpected errors.
> 
> For the expected error (e.g. rejecting user input), the design policy is
> - kprobes API should return correct error code.
> - kprobe tracefs I/F should return correct error code and put a human
>   readable error mesage in the error_log.
> Thus, the perf probe should decode the error code or reuse the error_log.
> 
>> It is easier to look 
>> up such error strings in the kernel source to understand why a probe was 
>> rejected.
> 
> I don't like to put a log message for rejecting user input on dmesg anymore.

Understood.

> 
> 
>> We also have perf_event_open() as an interface to add probes, and I 
>> don't think it would be helpful to require all tools to utilize the 
>> error log from tracefs for this purpose.
> 
> No, perf probe doesn't use perf-event interface to add probes. It uses
> the tracefs for adding probes.

Yes, but I was referring to some of the bpf tools (bcc) that now use 
perf_event_open() interface.


Thanks,
Naveen

Re: [PATCH] kprobes: Print an error if probe is rejected

[Masami Hiramatsu]

Hi Naveen,

On Mon, 14 Jun 2021 21:07:40 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> >> We also have perf_event_open() as an interface to add probes, and I 
> >> don't think it would be helpful to require all tools to utilize the 
> >> error log from tracefs for this purpose.
> > 
> > No, perf probe doesn't use perf-event interface to add probes. It uses
> > the tracefs for adding probes.
> 
> Yes, but I was referring to some of the bpf tools (bcc) that now use 
> perf_event_open() interface.

Yes, bpf chooses to use a special temporary perf_event, something like a
performance counter event. Those are hidden from tracefs and perf-tool.
The perf probe is the frontend of the trace event and perf event, which
will add dynamic events in tracefs, and perf will use it as same as
the other static events.

Thank you,




-- 
Masami Hiramatsu <mhiramat@kernel.org>