Re: [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe()

[Masami Hiramatsu]

Hi James,

On Tue, 8 Jan 2019 17:13:36 +0000
James Morse <james.morse@xxxxxxx> wrote:

> Hi!
> 
> On 08/01/2019 02:39, Masami Hiramatsu wrote:
> > On Thu, 3 Jan 2019 17:05:18 +0000
> > James Morse <james.morse@xxxxxxx> wrote:
> >> On 17/12/2018 06:40, Masami Hiramatsu wrote:
> >>> Move extable address check into arch_prepare_kprobe() from
> >>> arch_within_kprobe_blacklist().
> >>
> >> I'm trying to work out the pattern for what should go in the blacklist, and what
> >> should be rejected by the arch code.
> >>
> >> It seems address-ranges should be blacklisted as the contents don't matter.
> >> easy-example: the idmap text.
> > 
> > Yes, more precisely, the code smaller than a function (symbol), it must be
> > rejected by arch_prepare_kprobe(), since blacklist is poplated based on
> > kallsyms.
> 
> Ah, okay, so the pattern is the blacklist should only be for whole symbols,
> (which explains why its usually based on sections).

Correct. Actually, the blacklist is generated based on the symbol info
from symbol address.

> I see kprobe_add_ksym_blacklist() would go wrong if you give it something like:
> platform_drv_probe+0x50/0xb0, as it will log platform_drv_probe+0x50 as the
> start_addr and platform_drv_probe+0x50+0xb0 as the end.

Yes, it expects given address is the entry of a symbol.

> 
> But how does anything from the arch code's blacklist get into the
> kprobe_blacklist list?

It should be done via arch_populate_kprobe_blacklist().

> 
> We don't have an arch_populate_kprobe_blacklist(), so rely on
> within_kprobe_blacklist() calling arch_within_kprobe_blacklist() with the
> address, as well as walking kprobe_blacklist.
> 
> Is this cleanup ahead of a series that does away with
> arch_within_kprobe_blacklist() so that debugfs list is always complete?

Right, after this cleanup, I will send arch_populate_kprobe_blacklist()
patch for arm64 and others. My plan is to move all arch_within_kprobe_blacklist()
to arch_populate_kprobe_blacklist() so that user can get more precise blacklist
via debugfs.


> > As I pointed, the exception_table contains some range of code which inside
> > functions, must be smaller than function.
> > Since those instructions are expected to cause exception (that is main reason
> > why it can not be probed on arm64), I thought such situation was similar to
> > the limitation of instruction.
> > 
> > So I think below will be better.
> > ----
> > Please do not blacklisting instructions on exception_table,
> > since those are smaller than one function.
> > ----
> 
> I keep tripping over this because the exception_table lists addresses that are
> allowed to fault. Nothing looks at the instruction, and we happily kprobe the
> same instruction elsewhere.

Thanks!

> 
> (based on my assumptions about where you are going next!,), How about:
> | The blacklist is exposed via debugfs as a list of symbols. extable entries are
> | smaller, so must be filtered out by arch_prepare_kprobe().

This looks much better for me too :)
Should I resend with the description?

Thank you!

> 
> (only we currently have more than one blacklist...)
> 
> 
> Thanks,
> 
> James


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>