Re: [PATCH V2 6/6] arm64: Add uprobe support

[Pratyush Anand]

Hi Catalin,

On Sun, Oct 30, 2016 at 7:39 PM, Catalin Marinas
<catalin.marinas@xxxxxxx> wrote:
> On Tue, Sep 27, 2016 at 01:18:00PM +0530, Pratyush Anand wrote:
>> --- /dev/null
>> +++ b/arch/arm64/kernel/probes/uprobes.c
>> @@ -0,0 +1,221 @@
>> +/*
>> + * Copyright (C) 2014-2016 Pratyush Anand <panand@xxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include <linux/highmem.h>
>> +#include <linux/ptrace.h>
>> +#include <linux/uprobes.h>
>> +#include <asm/cacheflush.h>
>> +
>> +#include "decode-insn.h"
>> +
>> +#define UPROBE_INV_FAULT_CODE        UINT_MAX
>> +
>> +bool is_trap_insn(uprobe_opcode_t *insn)
>> +{
>> +     return false;
>> +}
>
> On the previous series, I had a comment left unanswered with regards to
> always returning false in is_trap_insn():
>
> 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.

A agreed on this that the older implementation i.e. the default one of
is_trap_insn() is better for the time being.

probably 'strtle r0, [r0], #160' would have the closest matching
aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
seems a bad instruction. So, there might not be any aarch32
instruction which will match to uprobe BRK instruction.


Therefore, if I send a V3 by removing aacrh64
Hi Catalin,

On Sun, Oct 30, 2016 at 7:39 PM, Catalin Marinas
<catalin.marinas@xxxxxxx> wrote:
> On Tue, Sep 27, 2016 at 01:18:00PM +0530, Pratyush Anand wrote:
>> --- /dev/null
>> +++ b/arch/arm64/kernel/probes/uprobes.c
>> @@ -0,0 +1,221 @@
>> +/*
>> + * Copyright (C) 2014-2016 Pratyush Anand <panand@xxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include <linux/highmem.h>
>> +#include <linux/ptrace.h>
>> +#include <linux/uprobes.h>
>> +#include <asm/cacheflush.h>
>> +
>> +#include "decode-insn.h"
>> +
>> +#define UPROBE_INV_FAULT_CODE        UINT_MAX
>> +
>> +bool is_trap_insn(uprobe_opcode_t *insn)
>> +{
>> +     return false;
>> +}
>
> On the previous series, I had a comment left unanswered with regards to
> always returning false in is_trap_insn():
>
> 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.

A agreed on this that the older implementation i.e. the default one of
is_trap_insn() is better for the time being.

probably 'strtle r0, [r0], #160' would have the closest matching
aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
seems a bad instruction. So, there might not be any aarch32
instruction which will match to uprobe BRK instruction.


Therefore, if I send a V3 by removing aacrh64
Hi Catalin,

On Sun, Oct 30, 2016 at 7:39 PM, Catalin Marinas
<catalin.marinas@xxxxxxx> wrote:
> On Tue, Sep 27, 2016 at 01:18:00PM +0530, Pratyush Anand wrote:
>> --- /dev/null
>> +++ b/arch/arm64/kernel/probes/uprobes.c
>> @@ -0,0 +1,221 @@
>> +/*
>> + * Copyright (C) 2014-2016 Pratyush Anand <panand@xxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include <linux/highmem.h>
>> +#include <linux/ptrace.h>
>> +#include <linux/uprobes.h>
>> +#include <asm/cacheflush.h>
>> +
>> +#include "decode-insn.h"
>> +
>> +#define UPROBE_INV_FAULT_CODE        UINT_MAX
>> +
>> +bool is_trap_insn(uprobe_opcode_t *insn)
>> +{
>> +     return false;
>> +}
>
> On the previous series, I had a comment left unanswered with regards to
> always returning false in is_trap_insn():
>
> 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.

A agreed on this that the older implementation i.e. the default one of
is_trap_insn() is better for the time being. I sent out V2 before your
last comment on it in V1 :(.

probably 'strtle r0, [r0], #160' would have the closest matching
aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
seems a bad aarch32 instruction. So, there might not be any aarch32
instruction which will match to uprobe BRK instruction.


Therefore, if I send a V3 by removing aacrh64 is_trap_insn(), would
that be acceptable, or do you see any other issue with this patch
series? If there is anything else, I would address that in V3 as well.

Thanks for your review.

~Pratyush