Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490052 > unrolled thread

Re: [PATCH 5/5] arm64: Add uprobe support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 5/5] arm64: Add uprobe support Catalin Marinas <catalin.marinas@arm.com> - 2016-09-23 15:10 +0200
    Re: [PATCH 5/5] arm64: Add uprobe support Pratyush Anand <panand@redhat.com> - 2016-09-25 19:10 +0200
      Re: [PATCH 5/5] arm64: Add uprobe support Catalin Marinas <catalin.marinas@arm.com> - 2016-09-26 13:10 +0200
        Re: [PATCH 5/5] arm64: Add uprobe support Pratyush Anand <panand@redhat.com> - 2016-09-26 15:10 +0200
          Re: [PATCH 5/5] arm64: Add uprobe support Catalin Marinas <catalin.marinas@arm.com> - 2016-09-27 16:00 +0200
            Re: [PATCH 5/5] arm64: Add uprobe support Pratyush Anand <panand@redhat.com> - 2016-09-27 17:10 +0200
              Re: [PATCH 5/5] arm64: Add uprobe support Catalin Marinas <catalin.marinas@arm.com> - 2016-09-28 19:20 +0200

#1490052 — Re: [PATCH 5/5] arm64: Add uprobe support

On Fri, Sep 23, 2016 at 09:42:30AM +0530, Pratyush Anand wrote:
> On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
> > On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
> > > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> > > > On Wed, Sep 21, 2016 at 04:30:47PM +0530, Pratyush Anand wrote:
> > > > > On 20/09/2016:05:59:46 PM, Catalin Marinas wrote:
> > > > > > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> > > > > > > +		unsigned long addr)
> > > > > > > +{
> > > > > > > +	probe_opcode_t insn;
> > > > > > > +
> > > > > > > +	/* TODO: Currently we do not support AARCH32 instruction probing */
> > > > > > 
> > > > > > Is there a way to check (not necessarily in this file) that we don't
> > > > > > probe 32-bit tasks?
> > > > > 
> > > > > - Well, I do not have complete idea about it that, how it can be done. I think
> > > > >   we can not check that just by looking a single bit in an instruction.
> > > > >   My understanding is that, we can only know about it when we are executing the
> > > > >   instruction, by reading pstate, but that would not be useful for uprobe
> > > > >   instruction analysis.
> > > > >   
> > > > >   I hope, instruction encoding for aarch32 and aarch64 are different, and by
> > > > >   analyzing for all types of aarch32 instructions, we will be able to decide
> > > > >   that whether instruction is 32 bit trace-able or not.  Accordingly, we can use
> > > > >   either BRK or BKPT instruction for breakpoint generation.
> > > > 
> > > > We may have some unrelated instruction encoding overlapping but I
> > > > haven't checked. I was more thinking about whether we know which task is
> > > > being probed and check is_compat_task() or maybe using
> > > > compat_user_mode(regs).
> > > 
> > > I had thought of this, but problem is that we might not have task in existence
> > > when we enable uprobes.  For example: Lets say we are inserting a trace probe at
> > > offset 0x690 in a executable binary.
> > > 
> > > echo "p test:0x690" > /sys/kernel/debug/tracing/uprobe_events
> > > echo 1 > /sys/kernel/debug/tracing/events/uprobes/enable
> > > 
> > > In the 'enable' step, it is decided that whether instruction is traceable or
> > > not. 
> > > 
> > > (1) But at this point 'test' executable might not be running.
> 
> Let me correct myself first here. When executable is not running, then,
> arch_uprobe_analyze_insn() is not called while uprobes enabling (ie writing '1'
> to 'enable'). In that case, it is called when binary is executed and task is
> created.

I kind of figured this out. This function needs to read the actual
instruction from memory, so that won't be possible until at least the
executable file has an address_space in the kernel. What's not clear to
me is how early this is done, do we have the mm_struct fully populated?

> > > (2) Even if it is running, is_compat_task() or compat_user_mode() might not be
> > > usable, as they work with 'current' task.
> > 
> > What I find strange is that uprobes allows you to insert a breakpoint
> > instruction that's not even compatible with the task (so it would
> > SIGILL rather than generate a debug exception).
> > 
> > > What I was thinking that, let it go with 'TODO' as of now. 
> > 
> > Only that I don't have any guarantee that someone is going to fix it ;).
> > 
> > As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
> > the arch_uprobe_analyze_insn() function.
> 
> It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can
> return -EINVAL when mm->task_size < TASK_SIZE_64.

That's just a temporary workaround. If we ever merge ILP32, this test
would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).

Looking at prepare_uprobe(), we have a weak is_trap_insn() function.
This check is meaningless without knowing which instruction set we
target. A false positive here, however, is not that bad as we wouldn't
end up inserting the wrong breakpoint in the executable. But it looks to
me like the core uprobe code needs to pass some additional information
like the type of task or ELF format to the arch code to make a useful
choice of breakpoint type.

-- 
Catalin

[toc] | [next] | [standalone]

#1490936

On Fri, Sep 23, 2016 at 6:35 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Fri, Sep 23, 2016 at 09:42:30AM +0530, Pratyush Anand wrote:
>> On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
>> > On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
>> > > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:

>> > As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
>> > the arch_uprobe_analyze_insn() function.
>>
>> It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can
>> return -EINVAL when mm->task_size < TASK_SIZE_64.
>
> That's just a temporary workaround. If we ever merge ILP32, this test
> would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).

OK.. So what about doing something similar what x86 is doing.
We can have a flag for task Type in arch specific mm_context_t. We
also set this flag in COMPAT_SET_PERSONALITY() along with setting
thread_info flag, and we clear them in SET_PERSONALITY().

>
> Looking at prepare_uprobe(), we have a weak is_trap_insn() function.
> This check is meaningless without knowing which instruction set we
> target. A false positive here, however, is not that bad as we wouldn't
> end up inserting the wrong breakpoint in the executable. But it looks to
> me like the core uprobe code needs to pass some additional information
> like the type of task or ELF format to the arch code to make a useful
> choice of breakpoint type.

It seems that 'strtle r0, [r0], #160' would have the closest matching
aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
seems a bad instruction. So, may be we can use still weak
is_trap_insn().

~Pratyush

[toc] | [prev] | [next] | [standalone]

#1491216

On Sun, Sep 25, 2016 at 10:32:28PM +0530, Pratyush Anand wrote:
> On Fri, Sep 23, 2016 at 6:35 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Fri, Sep 23, 2016 at 09:42:30AM +0530, Pratyush Anand wrote:
> >> On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
> >> > On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
> >> > > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> 
> >> > As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
> >> > the arch_uprobe_analyze_insn() function.
> >>
> >> It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can
> >> return -EINVAL when mm->task_size < TASK_SIZE_64.
> >
> > That's just a temporary workaround. If we ever merge ILP32, this test
> > would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).
> 
> OK.. So what about doing something similar what x86 is doing.
> We can have a flag for task Type in arch specific mm_context_t. We
> also set this flag in COMPAT_SET_PERSONALITY() along with setting
> thread_info flag, and we clear them in SET_PERSONALITY().

This looks like a better approach.

> > Looking at prepare_uprobe(), we have a weak is_trap_insn() function.
> > This check is meaningless without knowing which instruction set we
> > target. A false positive here, however, is not that bad as we wouldn't
> > end up inserting the wrong breakpoint in the executable. But it looks to
> > me like the core uprobe code needs to pass some additional information
> > like the type of task or ELF format to the arch code to make a useful
> > choice of breakpoint type.
> 
> It seems that 'strtle r0, [r0], #160' would have the closest matching
> aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
> seems a bad instruction. So, may be we can use still weak
> is_trap_insn().

Even if the is_trap_insn() check passes, we would reject the probe in
arch_uprobe_analyze_insn() immediately after based on the mm type check,
so not too bad.

If we add support for probing 32-bit tasks, I would rather have
is_trap_insn() take the mm_struct as argument so that a non-weak
implementation can check for the correct encoding.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1491291

On 26/09/2016:12:01:59 PM, Catalin Marinas wrote:
> On Sun, Sep 25, 2016 at 10:32:28PM +0530, Pratyush Anand wrote:
> > On Fri, Sep 23, 2016 at 6:35 PM, Catalin Marinas
> > <catalin.marinas@arm.com> wrote:
> > > On Fri, Sep 23, 2016 at 09:42:30AM +0530, Pratyush Anand wrote:
> > >> On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
> > >> > On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
> > >> > > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> > 
> > >> > As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
> > >> > the arch_uprobe_analyze_insn() function.
> > >>
> > >> It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can
> > >> return -EINVAL when mm->task_size < TASK_SIZE_64.
> > >
> > > That's just a temporary workaround. If we ever merge ILP32, this test
> > > would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).
> > 
> > OK.. So what about doing something similar what x86 is doing.
> > We can have a flag for task Type in arch specific mm_context_t. We
> > also set this flag in COMPAT_SET_PERSONALITY() along with setting
> > thread_info flag, and we clear them in SET_PERSONALITY().
> 
> This looks like a better approach.
> 
> > > Looking at prepare_uprobe(), we have a weak is_trap_insn() function.
> > > This check is meaningless without knowing which instruction set we
> > > target. A false positive here, however, is not that bad as we wouldn't
> > > end up inserting the wrong breakpoint in the executable. But it looks to
> > > me like the core uprobe code needs to pass some additional information
> > > like the type of task or ELF format to the arch code to make a useful
> > > choice of breakpoint type.
> > 
> > It seems that 'strtle r0, [r0], #160' would have the closest matching
> > aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
> > seems a bad instruction. So, may be we can use still weak
> > is_trap_insn().
> 
> Even if the is_trap_insn() check passes, we would reject the probe in
> arch_uprobe_analyze_insn() immediately after based on the mm type check,
> so not too bad.

OK..I will have an always returning false from arm64 is_trap_insn() in v2.

> 
> If we add support for probing 32-bit tasks, I would rather have
> is_trap_insn() take the mm_struct as argument so that a non-weak
> implementation can check for the correct encoding.

Yes, for 32 bit task we would need mm_struct as arg in is_trap_insn() as well as
in is_swbp_insn(). We would also need to have arm64 specific set_swbp().

Thanks for all your input. It was helpful. I will send V2 soon.

~Pratyush

[toc] | [prev] | [next] | [standalone]

#1491919

On Mon, Sep 26, 2016 at 06:33:59PM +0530, Pratyush Anand wrote:
> On 26/09/2016:12:01:59 PM, Catalin Marinas wrote:
> > On Sun, Sep 25, 2016 at 10:32:28PM +0530, Pratyush Anand wrote:
> > > On Fri, Sep 23, 2016 at 6:35 PM, Catalin Marinas
> > > <catalin.marinas@arm.com> wrote:
> > > > On Fri, Sep 23, 2016 at 09:42:30AM +0530, Pratyush Anand wrote:
> > > >> On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
> > > >> > On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
> > > >> > > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> > > 
> > > >> > As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
> > > >> > the arch_uprobe_analyze_insn() function.
> > > >>
> > > >> It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can
> > > >> return -EINVAL when mm->task_size < TASK_SIZE_64.
> > > >
> > > > That's just a temporary workaround. If we ever merge ILP32, this test
> > > > would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).
> > > 
> > > OK.. So what about doing something similar what x86 is doing.
> > > We can have a flag for task Type in arch specific mm_context_t. We
> > > also set this flag in COMPAT_SET_PERSONALITY() along with setting
> > > thread_info flag, and we clear them in SET_PERSONALITY().
> > 
> > This looks like a better approach.
> > 
> > > > Looking at prepare_uprobe(), we have a weak is_trap_insn() function.
> > > > This check is meaningless without knowing which instruction set we
> > > > target. A false positive here, however, is not that bad as we wouldn't
> > > > end up inserting the wrong breakpoint in the executable. But it looks to
> > > > me like the core uprobe code needs to pass some additional information
> > > > like the type of task or ELF format to the arch code to make a useful
> > > > choice of breakpoint type.
> > > 
> > > It seems that 'strtle r0, [r0], #160' would have the closest matching
> > > aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
> > > seems a bad instruction. So, may be we can use still weak
> > > is_trap_insn().
> > 
> > Even if the is_trap_insn() check passes, we would reject the probe in
> > arch_uprobe_analyze_insn() immediately after based on the mm type check,
> > so not too bad.
> 
> OK..I will have an always returning false from arm64 is_trap_insn() in v2.

For the time being, I think the default is_trap_insn() check is still
useful on arm64. The problem gets trickier when we add AArch32 support
as it may return 'true' on an AArch32 instruction that matches the
AArch64 BRK (or vice-versa). That's when we need to either pass the mm
to is_trap_insn() or simply return false and always perform the check in
the arch_uprobe_analyze_insn() (which should, in addition, check for the
trap instruction).

There is also the is_trap_at_addr() function which uses is_trap_insn().
I haven't checked the call paths here, are there any implications if
is_trap_insn() always returns false?

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1491975

On Tuesday 27 September 2016 07:21 PM, Catalin Marinas wrote:
>>>>> Looking at prepare_uprobe(), we have a weak is_trap_insn() function.
>>>>> > > > > This check is meaningless without knowing which instruction set we
>>>>> > > > > target. A false positive here, however, is not that bad as we wouldn't
>>>>> > > > > end up inserting the wrong breakpoint in the executable. But it looks to
>>>>> > > > > me like the core uprobe code needs to pass some additional information
>>>>> > > > > like the type of task or ELF format to the arch code to make a useful
>>>>> > > > > choice of breakpoint type.
>>>> > > >
>>>> > > > It seems that 'strtle r0, [r0], #160' would have the closest matching
>>>> > > > aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
>>>> > > > seems a bad instruction. So, may be we can use still weak
>>>> > > > is_trap_insn().
>>> > >
>>> > > Even if the is_trap_insn() check passes, we would reject the probe in
>>> > > arch_uprobe_analyze_insn() immediately after based on the mm type check,
>>> > > so not too bad.
>> >
>> > OK..I will have an always returning false from arm64 is_trap_insn() in v2.
> For the time being, I think the default is_trap_insn() check is still
> useful on arm64.

I have already sent V2 with arm64 is_trap_insn() :(

> The problem gets trickier when we add AArch32 support
> as it may return 'true' on an AArch32 instruction that matches the
> AArch64 BRK (or vice-versa). That's when we need to either pass the mm
> to is_trap_insn() or simply return false and always perform the check in
> the arch_uprobe_analyze_insn() (which should, in addition, check for the
> trap instruction).

Yes, I agree that we will have to modify is_trap_insn() for supporting 
aarch32 task tracing.

>
> There is also the is_trap_at_addr() function which uses is_trap_insn().
> I haven't checked the call paths here, are there any implications if
> is_trap_insn() always returns false?

I had looked into it and also tested that a tracepoint at an application 
having a same instruction as that of "uprobe break instruction" ie "BRK 
#0x5" is rejected. So, I think a false positive return from 
is_tarp_insn() is still OK.

~Pratyush

[toc] | [prev] | [next] | [standalone]

#1492785

On Tue, Sep 27, 2016 at 08:33:25PM +0530, Pratyush Anand wrote:
> On Tuesday 27 September 2016 07:21 PM, Catalin Marinas wrote:
> >There is also the is_trap_at_addr() function which uses is_trap_insn().
> >I haven't checked the call paths here, are there any implications if
> >is_trap_insn() always returns false?
> 
> I had looked into it and also tested that a tracepoint at an application
> having a same instruction as that of "uprobe break instruction" ie "BRK
> #0x5" is rejected. So, I think a false positive return from is_tarp_insn()
> is still OK.

Looking at handle_swbp(), if we hit a breakpoint for which we don't have
a valid uprobe, this function currently sends a SIGTRAP. But if
is_trap_insn() returns false always, is_trap_at_addr() would return 0 in
this case so the SIGTRAP is never issued.

-- 
Catalin

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web