Re: exercising current aarch64 kprobe support with systemtap

[David Long <dave dot long at linaro dot org>]

On 06/10/2016 10:03 AM, William Cohen wrote:
> On 06/10/2016 09:42 AM, Pratyush Anand wrote:
> > On 10/06/2016:01:49:10 AM, David Long wrote:
> > > Attached are incremental diffs I hope will fix the latest systemtap
> > > failures, without abandoning atomic sequence checking.  I'm trying to avoid
> > > the hex constants but I don't think the insn.c functions help in this case.
> >
> > It will save us from current problem by checking "stp x29,x30,[sp,...]"
> > instruction and returning false if matches. However, we will have to find some
> > way to recognize .word instructions.
> >
> > * An assembly function may not start with "stp x29,x30,[sp,...]", e.g.
> >   __dma_map_area(), _cpu_resume etc. However, it could be least likely that a
> >   .word instruction exists before start of assembly function and that too
> >   contains a word value which could be misleading.
> >
> > * But major issue is, what if someone instruments a kprobe at an address which
> >   contains  .word values. Instruction will never hit, so probe function will not
> >   be called, but when real code reads that .word value, it reads a wrong value.
> >
> > Can GCC provide some compiler option where .word values are located into a
> > specific area?
> >
> > ~Pratyush
>
> Hi Dave and Pratyush,
>
> Expecting the instruction to the stp x29, x30, [sp,...] would be pretty fragile.  The compiler might not generate that for some very simple function or with certain types of optimization. If the compiler could generate a sentinel word before the start of each function that might be a more robust solution.  Maybe something like a breakpoint instruction or something that clearly would not be in an atomic region.

I think this is still a reasonable improvement.  The case of both 
heuristics failing together has to be pretty rare and the result is to 
make the safer choice.

I'm looking at what gcc might provide to help.  I made need to talk to a 
compiler expert though, I've always found the gcc option list a bit 
overwhelming.

> -Will
> > > -dl
> >
> > > diff --git a/arch/arm64/kernel/kprobes-arm64.c b/arch/arm64/kernel/kprobes-arm64.c
> > > index 28b9c5b..36b4ea5 100644
> > > --- a/arch/arm64/kernel/kprobes-arm64.c
> > > +++ b/arch/arm64/kernel/kprobes-arm64.c
> > > @@ -127,7 +127,9 @@ is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t *scan_end)
> > >   		 * atomic region starts from exclusive load and ends with
> > >   		 * exclusive store.
> > >   		 */
> > > -		if (aarch64_insn_is_store_ex(le32_to_cpu(*scan_start)))
> > > +		if ((le32_to_cpu(*scan_start) & 0xffc07fff) == 0xa9807bfd)
> > > +			return false;
> > > +		else if (aarch64_insn_is_store_ex(le32_to_cpu(*scan_start)))
> > >   			return false;
> > >   		else if (aarch64_insn_is_load_ex(le32_to_cpu(*scan_start)))
> > >   			return true;