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

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

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