From: Pratyush Anand <panand@redhat.com> To: Catalin Marinas <catalin.marinas@arm.com> Cc: Mark Rutland <mark.rutland@arm.com>, srikar@linux.vnet.ibm.com, Will Deacon <will.deacon@arm.com>, open list <linux-kernel@vger.kernel.org>, Vladimir Murzin <vladimir.murzin@arm.com>, Russell King - ARM Linux <linux@arm.linux.org.uk>, vijaya.kumar@caviumnetworks.com, Dave Long <dave.long@linaro.org>, Shi Yang <yang.shi@linaro.org>, Jungseok Lee <jungseoklee85@gmail.com>, Steve Capper <steve.capper@linaro.org>, "Suzuki K. Poulose" <suzuki.poulose@arm.com>, Andre Przywara <andre.przywara@arm.com>, Shaokun Zhang <zhangshaokun@hisilicon.com>, Ashok Kumar <ashoks@broadcom.com>, Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com>, Will Cohen <wcohen@redhat.com>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Oleg Nesterov <oleg@redhat.com>, James Morse <james.morse@arm.com>, Masami Hiramatsu <mhiramat@kernel.org>, Robin Murphy <robin.murphy@arm.com>, "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> Subject: Re: [PATCH 5/5] arm64: Add uprobe support Date: Tue, 27 Sep 2016 20:33:25 +0530 [thread overview] Message-ID: <c7675b74-982d-d8b6-935f-a74139820174@redhat.com> (raw) In-Reply-To: <20160927135133.GF17336@e104818-lin.cambridge.arm.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: panand@redhat.com (Pratyush Anand) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 5/5] arm64: Add uprobe support Date: Tue, 27 Sep 2016 20:33:25 +0530 [thread overview] Message-ID: <c7675b74-982d-d8b6-935f-a74139820174@redhat.com> (raw) In-Reply-To: <20160927135133.GF17336@e104818-lin.cambridge.arm.com> 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
next prev parent reply other threads:[~2016-09-27 15:03 UTC|newest] Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-08-02 5:30 [PATCH 0/5] ARM64: Uprobe support added Pratyush Anand 2016-08-02 5:30 ` Pratyush Anand 2016-08-02 5:30 ` [PATCH 1/5] arm64: kprobe: protect/rename few definitions to be reused by uprobe Pratyush Anand 2016-08-02 5:30 ` Pratyush Anand 2016-08-02 5:30 ` [PATCH 2/5] arm64: kgdb_step_brk_fn: ignore other's exception Pratyush Anand 2016-08-02 5:30 ` Pratyush Anand 2016-08-02 5:30 ` [PATCH 3/5] arm64: Handle TRAP_HWBRKPT for user mode as well Pratyush Anand 2016-08-02 5:30 ` Pratyush Anand 2016-09-06 16:11 ` Catalin Marinas 2016-09-06 16:11 ` Catalin Marinas 2016-09-06 21:36 ` David Long 2016-09-06 21:36 ` David Long 2016-09-07 4:47 ` Pratyush Anand 2016-09-07 4:47 ` Pratyush Anand 2016-09-07 13:41 ` Catalin Marinas 2016-09-07 13:41 ` Catalin Marinas 2016-08-02 5:30 ` [PATCH 4/5] arm64: Handle TRAP_BRKPT " Pratyush Anand 2016-08-02 5:30 ` Pratyush Anand 2016-09-06 16:34 ` Catalin Marinas 2016-09-06 16:34 ` Catalin Marinas 2016-08-02 5:30 ` [PATCH 5/5] arm64: Add uprobe support Pratyush Anand 2016-08-02 5:30 ` Pratyush Anand 2016-08-09 18:49 ` Oleg Nesterov 2016-08-09 18:49 ` Oleg Nesterov 2016-08-24 7:13 ` Pratyush Anand 2016-08-24 7:13 ` Pratyush Anand 2016-08-24 15:47 ` Oleg Nesterov 2016-08-24 15:47 ` Oleg Nesterov 2016-08-24 15:56 ` Will Deacon 2016-08-24 15:56 ` Will Deacon 2016-08-25 13:33 ` Oleg Nesterov 2016-08-25 13:33 ` Oleg Nesterov 2016-09-20 16:59 ` Catalin Marinas 2016-09-20 16:59 ` Catalin Marinas 2016-09-21 11:00 ` Pratyush Anand 2016-09-21 11:00 ` Pratyush Anand 2016-09-21 17:04 ` Catalin Marinas 2016-09-21 17:04 ` Catalin Marinas 2016-09-22 3:23 ` Pratyush Anand 2016-09-22 3:23 ` Pratyush Anand 2016-09-22 16:50 ` Catalin Marinas 2016-09-22 16:50 ` Catalin Marinas 2016-09-23 4:12 ` Pratyush Anand 2016-09-23 4:12 ` Pratyush Anand 2016-09-23 13:05 ` Catalin Marinas 2016-09-23 13:05 ` Catalin Marinas 2016-09-25 17:02 ` Pratyush Anand 2016-09-25 17:02 ` Pratyush Anand 2016-09-26 11:01 ` Catalin Marinas 2016-09-26 11:01 ` Catalin Marinas 2016-09-26 13:03 ` Pratyush Anand 2016-09-26 13:03 ` Pratyush Anand 2016-09-27 13:51 ` Catalin Marinas 2016-09-27 13:51 ` Catalin Marinas 2016-09-27 15:03 ` Pratyush Anand [this message] 2016-09-27 15:03 ` Pratyush Anand 2016-09-28 17:12 ` Catalin Marinas 2016-09-28 17:12 ` Catalin Marinas 2016-08-24 7:26 ` [PATCH 0/5] ARM64: Uprobe support added Pratyush Anand 2016-08-24 7:26 ` Pratyush Anand 2016-09-20 2:51 ` Pratyush Anand 2016-09-20 2:51 ` Pratyush Anand
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c7675b74-982d-d8b6-935f-a74139820174@redhat.com \ --to=panand@redhat.com \ --cc=andre.przywara@arm.com \ --cc=ard.biesheuvel@linaro.org \ --cc=ashoks@broadcom.com \ --cc=catalin.marinas@arm.com \ --cc=dave.long@linaro.org \ --cc=james.morse@arm.com \ --cc=jungseoklee85@gmail.com \ --cc=kirill.shutemov@linux.intel.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=mark.rutland@arm.com \ --cc=mhiramat@kernel.org \ --cc=oleg@redhat.com \ --cc=robin.murphy@arm.com \ --cc=sandeepa.s.prabhu@gmail.com \ --cc=srikar@linux.vnet.ibm.com \ --cc=steve.capper@linaro.org \ --cc=suzuki.poulose@arm.com \ --cc=vijaya.kumar@caviumnetworks.com \ --cc=vladimir.murzin@arm.com \ --cc=wcohen@redhat.com \ --cc=will.deacon@arm.com \ --cc=yang.shi@linaro.org \ --cc=zhangshaokun@hisilicon.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.