Re: [PATCH v3 1/5] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

On 19 November 2014 20:25, David Long  wrote:
>> I was thinking of the magic hex numbers in the kprobes decode tables,
>> which
>> seem to correspond directly to the instruction classes described in insn.c
>>
>> Keeping the actual emulation code separate makes sense.
>>
>> Will
>
>
> Of course that follows the model of the much more complex arm32
> kprobes/uprobes decoding.  I can have a go at replacing it with insn.c
> calls.
well, the magic hex numbers were derived directly from ARMv8 ARM
Tabled C4.1 thru C4.6 and bit-masking would be faster search, but
surely you can give a try with insn.c (this would consume lot of
function calls to arrive at same decision).
~Sandeepa
>
> -dl
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 1/5] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

On 18 November 2014 20:26, Will Deacon  wrote:

> One thing I noticed looking through this patch is that we're effectively
> reinventing a bunch of the instruction decoding logic that we already have
> in the kernel (introduced since Sandeepa last sent his patch).
>
> Could you take a look at include/asm/insn.h and kernel/insn.c please, and
> see if you can at least consolidate some of this? Some of it should be easy
> (i.e. reusing masks, using existing #defines to construct BRK encodings),
> but I appreciate there may be places where kprobes needs to add extra bits,
> in which case I'd really like to keep this all together if at all possible.
>
> We're currently in a position where the module loader, BPF jit, ftrace and
> the proposed alternative patching scheme are all using the same instruction
> manipulation functions, so we should try to continue that trend if we can.
Will,

kernel/insn.c support generating instruction encodings(forming opcodes
with given specifications), so for kprobes, only BRK encoding can use
this mechanism.
For instruction simulation, the instruction behavior should be
simulated on saved pt_regs, which is not supported on insn.c routines,
so still need probes-simulate-insn.c. Please point me if I am missing
something here.

>
> Thanks,
>
> Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Sandeepa Prabhu]

On 24 April 2014 02:02, Tanmay Inamdar  wrote:
> Hello Sandeepa,
>
> On Tue, Apr 22, 2014 at 4:50 AM, Sandeepa Prabhu
>  wrote:
>> On 22 April 2014 15:41, Liviu Dudau  wrote:
>>> On Tue, Apr 22, 2014 at 09:58:28AM +0100, Sandeepa Prabhu wrote:
>>>> On 14 March 2014 21:04, Liviu Dudau  wrote:
>>>> > Hi,
>>>> >
>>>> > This patch adds support for PCI to AArch64. It is based on my v7 patch
>>>> > that adds support for creating generic host bridge structure from
>>>> > device tree. With that in place, I was able to boot a platform that
>>>> > has PCIe host bridge support and use a PCIe network card.
>>>> Hi Liviu,
>>>>
>>>> Are these patches (including your other patchset for device tree host
>>>> bridge support ) available on a public git repo I can checkout from?
>>>
>>> Yes, I have pushed them into git://linux-arm.org/linux-ld.git
>> Thanks Liviu,
>> Just to understand, is the X-gene PCIe
>> (https://lwn.net/Articles/589733/) verified using the same patchset?
>
> Yes. V5 version of X-Gene PCIe patches have verified Liviu's patchset
> on X-Gene platform.
Thanks Tanmay!
>
>>
>> ~Sandeepa
>>>
>>> Best regards,
>>> Liviu
>>>
>>>>
>>>> Thanks,
>>>> ~Sandeepa
>>>>
>>>> >
>>>> > I have dropped the RFC tag from the subject as I now have the ambitious 
>>>> > goal
>>>> > of trying to get it mainlined.
>>>> >
>>>> > Changes from v6:
>>>> >   - Guard the pci_domain_nr() inline implementation with #ifdef 
>>>> > CONFIG_PCI as
>>>> > to avoid conflict with default empty version present in 
>>>> > include/linux/pci.h.
>>>> > Thanks to Jingoo Han for catching this.
>>>> >
>>>> > Changes from v5:
>>>> >   - Removed pcibios_fixup_bridge_ranges() as the week default version is 
>>>> > fine.
>>>> >   - Removed the ALIGN() call in pcibios_align_resource()
>>>> >   - Stopped exporting pcibios_align_resource()
>>>> >
>>>> > Changes from v4:
>>>> >   - Fixed the pci_domain_nr() implementation for arm64. Now we use
>>>> > find_pci_host_bride() to find the host bridge before we retrieve
>>>> > the domain number.
>>>> >
>>>> > Changes from v3:
>>>> >   - Added Acks accumulated so far ;)
>>>> >   - Still carrying Catalin's patch for moving the PCI_IO_BASE until it
>>>> > lands in linux-next or mainline, in order to ease applying the series
>>>> >
>>>> > Changes from v2:
>>>> >   - Implement an arch specific version of pci_register_io_range() and
>>>> > pci_address_to_pio().
>>>> >   - Return 1 from pci_proc_domain().
>>>> >
>>>> > Changes from v1:
>>>> >   - Added Catalin's patch for moving the PCI_IO_BASE location and extend
>>>> > its size to 16MB
>>>> >   - Integrated Arnd's version of pci_ioremap_io that uses a bitmap for
>>>> > keeping track of assigned IO space and returns an io_offset. At the
>>>> > moment the code is added in arch/arm64 but it can be moved in 
>>>> > drivers/pci.
>>>> >   - Added a fix for the generic ioport_map() function when 
>>>> > !CONFIG_GENERIC_IOMAP
>>>> > as suggested by Arnd.
>>>> >
>>>> > v6 thread here: https://lkml.org/lkml/2014/3/5/41
>>>> > v5 thread here: https://lkml.org/lkml/2014/3/4/307
>>>> > v4 thread here: https://lkml.org/lkml/2014/3/3/298
>>>> > v3 thread here: https://lkml.org/lkml/2014/2/28/211
>>>> > v2 thread here: https://lkml.org/lkml/2014/2/27/255
>>>> > v1 thread here: https://lkml.org/lkml/2014/2/3/389
>>>> >
>>>> >
>>>> > The API used is different from the one used by ARM architecture. There is
>>>> > no pci_common_init_dev() function and no hw_pci structure, as that is no
>>>> > longer needed. Once the last signature is added to the legal agreement, I
>>>> > will post the host bridge driver code that I am using. Meanwhile, here
>>>> > is an example of what the probe function looks like, posted as an 
>>>> > example:
>>>> >
>>

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Sandeepa Prabhu]

On 22 April 2014 15:41, Liviu Dudau  wrote:
> On Tue, Apr 22, 2014 at 09:58:28AM +0100, Sandeepa Prabhu wrote:
>> On 14 March 2014 21:04, Liviu Dudau  wrote:
>> > Hi,
>> >
>> > This patch adds support for PCI to AArch64. It is based on my v7 patch
>> > that adds support for creating generic host bridge structure from
>> > device tree. With that in place, I was able to boot a platform that
>> > has PCIe host bridge support and use a PCIe network card.
>> Hi Liviu,
>>
>> Are these patches (including your other patchset for device tree host
>> bridge support ) available on a public git repo I can checkout from?
>
> Yes, I have pushed them into git://linux-arm.org/linux-ld.git
Thanks Liviu,
Just to understand, is the X-gene PCIe
(https://lwn.net/Articles/589733/) verified using the same patchset?

~Sandeepa
>
> Best regards,
> Liviu
>
>>
>> Thanks,
>> ~Sandeepa
>>
>> >
>> > I have dropped the RFC tag from the subject as I now have the ambitious 
>> > goal
>> > of trying to get it mainlined.
>> >
>> > Changes from v6:
>> >   - Guard the pci_domain_nr() inline implementation with #ifdef CONFIG_PCI 
>> > as
>> > to avoid conflict with default empty version present in 
>> > include/linux/pci.h.
>> > Thanks to Jingoo Han for catching this.
>> >
>> > Changes from v5:
>> >   - Removed pcibios_fixup_bridge_ranges() as the week default version is 
>> > fine.
>> >   - Removed the ALIGN() call in pcibios_align_resource()
>> >   - Stopped exporting pcibios_align_resource()
>> >
>> > Changes from v4:
>> >   - Fixed the pci_domain_nr() implementation for arm64. Now we use
>> > find_pci_host_bride() to find the host bridge before we retrieve
>> > the domain number.
>> >
>> > Changes from v3:
>> >   - Added Acks accumulated so far ;)
>> >   - Still carrying Catalin's patch for moving the PCI_IO_BASE until it
>> > lands in linux-next or mainline, in order to ease applying the series
>> >
>> > Changes from v2:
>> >   - Implement an arch specific version of pci_register_io_range() and
>> > pci_address_to_pio().
>> >   - Return 1 from pci_proc_domain().
>> >
>> > Changes from v1:
>> >   - Added Catalin's patch for moving the PCI_IO_BASE location and extend
>> > its size to 16MB
>> >   - Integrated Arnd's version of pci_ioremap_io that uses a bitmap for
>> > keeping track of assigned IO space and returns an io_offset. At the
>> > moment the code is added in arch/arm64 but it can be moved in 
>> > drivers/pci.
>> >   - Added a fix for the generic ioport_map() function when 
>> > !CONFIG_GENERIC_IOMAP
>> > as suggested by Arnd.
>> >
>> > v6 thread here: https://lkml.org/lkml/2014/3/5/41
>> > v5 thread here: https://lkml.org/lkml/2014/3/4/307
>> > v4 thread here: https://lkml.org/lkml/2014/3/3/298
>> > v3 thread here: https://lkml.org/lkml/2014/2/28/211
>> > v2 thread here: https://lkml.org/lkml/2014/2/27/255
>> > v1 thread here: https://lkml.org/lkml/2014/2/3/389
>> >
>> >
>> > The API used is different from the one used by ARM architecture. There is
>> > no pci_common_init_dev() function and no hw_pci structure, as that is no
>> > longer needed. Once the last signature is added to the legal agreement, I
>> > will post the host bridge driver code that I am using. Meanwhile, here
>> > is an example of what the probe function looks like, posted as an example:
>> >
>> > static int myhostbridge_probe(struct platform_device *pdev)
>> > {
>> > int err;
>> > struct device_node *dev;
>> > struct pci_host_bridge *bridge;
>> > struct myhostbridge_port *pp;
>> > resource_size_t lastbus;
>> >
>> > dev = pdev->dev.of_node;
>> >
>> > if (!of_device_is_available(dev)) {
>> > pr_warn("%s: disabled\n", dev->full_name);
>> > return -ENODEV;
>> > }
>> >
>> > pp = kzalloc(sizeof(struct myhostbridge_port), GFP_KERNEL);
>> > if (!pp)
>> > return -ENOMEM;
>> >
>> > bridge = of_create_pci_host_bridge(&pdev->dev, &myhostbridge_ops, 
>> > pp);
>> > if (IS_ERR(bridge)) {
>> >   

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Sandeepa Prabhu]

On 14 March 2014 21:04, Liviu Dudau  wrote:
> Hi,
>
> This patch adds support for PCI to AArch64. It is based on my v7 patch
> that adds support for creating generic host bridge structure from
> device tree. With that in place, I was able to boot a platform that
> has PCIe host bridge support and use a PCIe network card.
Hi Liviu,

Are these patches (including your other patchset for device tree host
bridge support ) available on a public git repo I can checkout from?

Thanks,
~Sandeepa

>
> I have dropped the RFC tag from the subject as I now have the ambitious goal
> of trying to get it mainlined.
>
> Changes from v6:
>   - Guard the pci_domain_nr() inline implementation with #ifdef CONFIG_PCI as
> to avoid conflict with default empty version present in 
> include/linux/pci.h.
> Thanks to Jingoo Han for catching this.
>
> Changes from v5:
>   - Removed pcibios_fixup_bridge_ranges() as the week default version is fine.
>   - Removed the ALIGN() call in pcibios_align_resource()
>   - Stopped exporting pcibios_align_resource()
>
> Changes from v4:
>   - Fixed the pci_domain_nr() implementation for arm64. Now we use
> find_pci_host_bride() to find the host bridge before we retrieve
> the domain number.
>
> Changes from v3:
>   - Added Acks accumulated so far ;)
>   - Still carrying Catalin's patch for moving the PCI_IO_BASE until it
> lands in linux-next or mainline, in order to ease applying the series
>
> Changes from v2:
>   - Implement an arch specific version of pci_register_io_range() and
> pci_address_to_pio().
>   - Return 1 from pci_proc_domain().
>
> Changes from v1:
>   - Added Catalin's patch for moving the PCI_IO_BASE location and extend
> its size to 16MB
>   - Integrated Arnd's version of pci_ioremap_io that uses a bitmap for
> keeping track of assigned IO space and returns an io_offset. At the
> moment the code is added in arch/arm64 but it can be moved in drivers/pci.
>   - Added a fix for the generic ioport_map() function when 
> !CONFIG_GENERIC_IOMAP
> as suggested by Arnd.
>
> v6 thread here: https://lkml.org/lkml/2014/3/5/41
> v5 thread here: https://lkml.org/lkml/2014/3/4/307
> v4 thread here: https://lkml.org/lkml/2014/3/3/298
> v3 thread here: https://lkml.org/lkml/2014/2/28/211
> v2 thread here: https://lkml.org/lkml/2014/2/27/255
> v1 thread here: https://lkml.org/lkml/2014/2/3/389
>
>
> The API used is different from the one used by ARM architecture. There is
> no pci_common_init_dev() function and no hw_pci structure, as that is no
> longer needed. Once the last signature is added to the legal agreement, I
> will post the host bridge driver code that I am using. Meanwhile, here
> is an example of what the probe function looks like, posted as an example:
>
> static int myhostbridge_probe(struct platform_device *pdev)
> {
> int err;
> struct device_node *dev;
> struct pci_host_bridge *bridge;
> struct myhostbridge_port *pp;
> resource_size_t lastbus;
>
> dev = pdev->dev.of_node;
>
> if (!of_device_is_available(dev)) {
> pr_warn("%s: disabled\n", dev->full_name);
> return -ENODEV;
> }
>
> pp = kzalloc(sizeof(struct myhostbridge_port), GFP_KERNEL);
> if (!pp)
> return -ENOMEM;
>
> bridge = of_create_pci_host_bridge(&pdev->dev, &myhostbridge_ops, pp);
> if (IS_ERR(bridge)) {
> err = PTR_ERR(bridge);
> goto bridge_create_fail;
> }
>
> err = myhostbridge_setup(bridge->bus);
> if (err)
> goto bridge_setup_fail;
>
> /* We always enable PCI domains and we keep domain 0 backward
>  * compatible in /proc for video cards
>  */
> pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
> pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
>
> lastbus = pci_scan_child_bus(bridge->bus);
> pci_bus_update_busn_res_end(bridge->bus, lastbus);
>
> pci_assign_unassigned_bus_resources(bridge->bus);
>
> pci_bus_add_devices(bridge->bus);
>
> return 0;
>
> bridge_setup_fail:
> put_device(&bridge->dev);
> device_unregister(&bridge->dev);
> bridge_create_fail:
> kfree(pp);
> return err;
> }
>
> Best regards,
> Liviu
>
>
> Catalin Marinas (1):
>   arm64: Extend the PCI I/O space to 16MB
>
> Liviu Dudau (2):
>   Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
>   arm64: Add architecture support for PCI
>
>  Documentation/arm64/memory.txt |  16 +--
>  arch/arm64/Kconfig |  19 +++-
>  arch/arm64/include/asm/Kbuild  |   1 +
>  arch/arm64/include/asm/io.h|   5 +-
>  arch/arm64/include/asm/pci.h   |  51 +
>  arch/arm64/kernel/Makefile |   1 +
>  arch/arm64/kernel/pci.c| 173 +++
>  include/asm-generic/io.h   |   2 +-
>  8 files changed, 258 insertions(+), 10 deletions(-)
>  create mode 100644 a

Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs

[Sandeepa Prabhu]

>> I am not sure if this question is related, uprobes or ftrace code does
>> not  define __kprobes, so is it safe to place kprobe on uprobes or
>> ftrace code?
>
> Yes, it is "safe" in qualitative meaning. But for ftrace code, it could
> give a performance impact by miss-hitting. Since uprobe is independent
> from kprobe, it should work.
>
>> Is it expected from arch code to support such cases?
>
> Yes, the arch dependent implementation is the key. If it shares some
> code which can be called from miss-hit path, it should be blacklisted.
well, isn't the blacklist only for those routines that can not be
handled or may crash kernel, like the code sections called from
exception kprobes exception handlers etc?
suppose if the probe on routine can miss-hit (probes re-cursing) but
can be handled, it's only a quantitative issue (i.e. performance
impact) so it should be *user's* problem right? I mean, as you said
earlier about having white-list or a performance gatekeeper
(systemtap), one can avoid such cases by white list or removing
miss-hit probes dynamically.  But a blacklisting a symbol means
placing a probe on that *can not be handled* and can crash the system,
is it correct?

Thanks,
Sandeepa

>
> Thank you,
>
> --
> Masami HIRAMATSU
> IT Management Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu...@hitachi.com
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs

[Sandeepa Prabhu]

> OK, I think the kprobe is like a strong medicine, not a toy,
> since it can intercept most of the kernel functions which
> may process a sensitive user private data. Thus even if we
> fix all bugs and make it safe, I don't think we can open
> it for all users (of course, there should be a knob to open
> for any or restricted users.)
>
>> So we need both a maintainable and a sane/safe solution, and I'd like
>> to apply the whole thing at once and be at ease that the solution is
>> round. We should have done this years ago.
>
> For the safeness of kprobes, I have an idea; introduce a whitelist
> for dynamic events. AFAICS, the biggest unstable issue of kprobes
> comes from putting *many* probes on the functions called from tracers.
>
> It doesn't crash the kernel but slows down so much, because every
> probes hit many other nested miss-hit probes. This gives us a big
> performance impact. However, on the other side, this kind of feature
> can be used *for debugging* static trace events by dynamic one if we
> carefully use a small number of probes on such functions. :)
>
> Thus, I think we can restrict users from probing such functions by
> using a whitelist which ftrace does already have;
>  available_filter_functions :)
I am not sure if this question is related, uprobes or ftrace code does
not  define __kprobes, so is it safe to place kprobe on uprobes or
ftrace code? Is it expected from arch code to support such cases?

Thanks,
Sandeepa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs

[Sandeepa Prabhu]

On 4 December 2013 13:09, Masami Hiramatsu
 wrote:
> (2013/12/04 11:54), Sandeepa Prabhu wrote:
>> On 4 December 2013 06:58, Masami Hiramatsu
>>  wrote:
>>> Hi,
>>> Here is the version 4 of NOKPORBE_SYMBOL series.
>>>
>>> In this version, I removed the cleanup patches and
>>> add bugfixes I've found, since those bugs will be
>>> critical.
>>> Rest of the cleanup and visible blacklists will be
>>> proposed later in another series.
>>>
>>> Oh, just one new thing, I added a new RFC patch which
>>> removes the dependency of notify_die() from kprobes
>>> miss-hit/recovery path. Since the notify_die() involves
>>> locking and lockdep code which invokes a lot of heavy
>>> printk functions etc. This helped me to minimize the
>>> blacklist and provides more stability for kprobes.
>>> Actually, most of int3 handlers are already called
>>> from do_int3 directly, I think this change is acceptable
>>> too.
>>>
>>> Here is the updates about NOKPROBE_SYMBOL().
>>>  - Now _ASM_NOKPROBE() macro is introduced for assembly
>>>symbols on x86.
>>>  - Rename kprobe_blackpoint to kprobe_blacklist_entry
>>>and simplify it. Also NOKPROBE_SYMBOL() macro just
>>>saves the address of non-probe-able symbols.
>>>
>>> ---
>>>
>>> Masami Hiramatsu (6):
>>
>>>   kprobes: Prohibit probing on .entry.text code
>>>   kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist
>> Hi Masami,
>> Is it good idea to split  "arch/x86" code from generic kernel changes?
>> Then we just need to take above two patches for verifying it on arm64
>> or other platforms.
>
> Yeah, it can be.
> However I think you can apply it without any problem on arm64 tree too,
> since it "just adds" an asm macro in arch/x86/include/asm/asm.h.
> It should not have any effect for other arch. Could you try it? :)
Hmm, for the second patch, git am failed with: "error: patch failed:
kernel/sched/core.c:2662",
manually patched to resolve it.  aarch64 tree is right now at Linux 3.13-rc2.

Anyways, no conflicts for x86 arch files.

Thanks,
Sandeepa

> Thank you,
>
>
> --
> Masami HIRAMATSU
> IT Management Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu...@hitachi.com
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4] arm64: support single-step and breakpoint handler hooks

[Sandeepa Prabhu]

AArch64 Single Steping and Breakpoint debug exceptions will be
used by multiple debug framworks like kprobes & kgdb.

This patch implements the hooks for those frameworks to register
their own handlers for handling breakpoint and single step events.

Reworked the debug exception handler in entry.S: do_dbg to route
software breakpoint (BRK64) exception to do_debug_exception()

Signed-off-by: Sandeepa Prabhu 
Signed-off-by: Deepak Saxena 
Acked-by: Will Deacon 
---
 arch/arm64/include/asm/debug-monitors.h | 21 
 arch/arm64/kernel/debug-monitors.c  | 88 -
 arch/arm64/kernel/entry.S   |  2 +
 3 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h 
b/arch/arm64/include/asm/debug-monitors.h
index a2232d0..6231479 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -62,6 +62,27 @@ struct task_struct;
 
 #define DBG_ARCH_ID_RESERVED   0   /* In case of ptrace ABI updates. */
 
+#define DBG_HOOK_HANDLED   0
+#define DBG_HOOK_ERROR 1
+
+struct step_hook {
+   struct list_head node;
+   int (*fn)(struct pt_regs *regs, unsigned int esr);
+};
+
+void register_step_hook(struct step_hook *hook);
+void unregister_step_hook(struct step_hook *hook);
+
+struct break_hook {
+   struct list_head node;
+   u32 esr_val;
+   u32 esr_mask;
+   int (*fn)(struct pt_regs *regs, unsigned int esr);
+};
+
+void register_break_hook(struct break_hook *hook);
+void unregister_break_hook(struct break_hook *hook);
+
 u8 debug_monitors_arch(void);
 
 void enable_debug_monitors(enum debug_el el);
diff --git a/arch/arm64/kernel/debug-monitors.c 
b/arch/arm64/kernel/debug-monitors.c
index 4ae6857..636ba8b 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -187,6 +187,48 @@ static void clear_regs_spsr_ss(struct pt_regs *regs)
regs->pstate = spsr;
 }
 
+/* EL1 Single Step Handler hooks */
+static LIST_HEAD(step_hook);
+DEFINE_RWLOCK(step_hook_lock);
+
+void register_step_hook(struct step_hook *hook)
+{
+   write_lock(&step_hook_lock);
+   list_add(&hook->node, &step_hook);
+   write_unlock(&step_hook_lock);
+}
+
+void unregister_step_hook(struct step_hook *hook)
+{
+   write_lock(&step_hook_lock);
+   list_del(&hook->node);
+   write_unlock(&step_hook_lock);
+}
+
+/*
+ * Call registered single step handers
+ * There is no Syndrome info to check for determining the handler.
+ * So we call all the registered handlers, until the right handler is
+ * found which returns zero.
+ */
+static int call_step_hook(struct pt_regs *regs, unsigned int esr)
+{
+   struct step_hook *hook;
+   int retval = DBG_HOOK_ERROR;
+
+   read_lock(&step_hook_lock);
+
+   list_for_each_entry(hook, &step_hook, node) {
+   retval = hook->fn(regs, esr);
+   if (retval == DBG_HOOK_HANDLED)
+   break;
+   }
+
+   read_unlock(&step_hook_lock);
+
+   return retval;
+}
+
 static int single_step_handler(unsigned long addr, unsigned int esr,
   struct pt_regs *regs)
 {
@@ -214,7 +256,9 @@ static int single_step_handler(unsigned long addr, unsigned 
int esr,
 */
user_rewind_single_step(current);
} else {
-   /* TODO: route to KGDB */
+   if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
+   return 0;
+
pr_warning("Unexpected kernel single-step exception at EL1\n");
/*
 * Re-enable stepping since we know that we will be
@@ -226,11 +270,53 @@ static int single_step_handler(unsigned long addr, 
unsigned int esr,
return 0;
 }
 
+/*
+ * Breakpoint handler is re-entrant as another breakpoint can
+ * hit within breakpoint handler, especically in kprobes.
+ * Use reader/writer locks instead of plain spinlock.
+ */
+static LIST_HEAD(break_hook);
+DEFINE_RWLOCK(break_hook_lock);
+
+void register_break_hook(struct break_hook *hook)
+{
+   write_lock(&break_hook_lock);
+   list_add(&hook->node, &break_hook);
+   write_unlock(&break_hook_lock);
+}
+
+void unregister_break_hook(struct break_hook *hook)
+{
+   write_lock(&break_hook_lock);
+   list_del(&hook->node);
+   write_unlock(&break_hook_lock);
+}
+
+static int call_break_hook(struct pt_regs *regs, unsigned int esr)
+{
+   struct break_hook *hook;
+   int (*fn)(struct pt_regs *regs, unsigned int esr) = NULL;
+
+   read_lock(&break_hook_lock);
+   list_for_each_entry(hook, &break_hook, node)
+   if ((esr & hook->esr_mask) == hook->esr_val)
+   fn = hook->fn;
+   read_unlock(&break_hook_lock);
+
+   return fn ? fn(regs, esr) : D

[PATCH v4] ARM64 breakpoint and single step exception hooks

[Sandeepa Prabhu]

This patch adds support for breakpoint and single-step exception hooks.

v3 version of this patch is published and reviewed with arm64 kdgb and kprobes 
patch series [1] and [2]

[1] http://lwn.net/Articles/570648/
[2] https://lwn.net/Articles/571063/

Changes v3 -> v4:
 -Incorporated review comments: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/207372.html
-Removed unnecessary comments
-Added comments for breakpoint re-entrancy & rw locks
 - Rebased on top of 
git://git.kernel.org/pub/scm/linux/kernel/git/cmarinas/linux-aarch64.git 
Branch:upstream
commit ID: dc1ccc48159d63eca5089e507c82c7d22ef60839  (Linux 3.13-rc2)
 - CCing Jason Wessel, since arm64 kgdb patchset is dependant on this.

Sandeepa Prabhu (1):
  arm64: support single-step and breakpoint handler hooks

 arch/arm64/include/asm/debug-monitors.h | 21 
 arch/arm64/kernel/debug-monitors.c  | 88 -
 arch/arm64/kernel/entry.S   |  2 +
 3 files changed, 110 insertions(+), 1 deletion(-)

-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs

[Sandeepa Prabhu]

On 4 December 2013 06:58, Masami Hiramatsu
 wrote:
> Hi,
> Here is the version 4 of NOKPORBE_SYMBOL series.
>
> In this version, I removed the cleanup patches and
> add bugfixes I've found, since those bugs will be
> critical.
> Rest of the cleanup and visible blacklists will be
> proposed later in another series.
>
> Oh, just one new thing, I added a new RFC patch which
> removes the dependency of notify_die() from kprobes
> miss-hit/recovery path. Since the notify_die() involves
> locking and lockdep code which invokes a lot of heavy
> printk functions etc. This helped me to minimize the
> blacklist and provides more stability for kprobes.
> Actually, most of int3 handlers are already called
> from do_int3 directly, I think this change is acceptable
> too.
>
> Here is the updates about NOKPROBE_SYMBOL().
>  - Now _ASM_NOKPROBE() macro is introduced for assembly
>symbols on x86.
>  - Rename kprobe_blackpoint to kprobe_blacklist_entry
>and simplify it. Also NOKPROBE_SYMBOL() macro just
>saves the address of non-probe-able symbols.
>
> ---
>
> Masami Hiramatsu (6):

>   kprobes: Prohibit probing on .entry.text code
>   kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist
Hi Masami,
Is it good idea to split  "arch/x86" code from generic kernel changes?
Then we just need to take above two patches for verifying it on arm64
or other platforms.

Thanks,
Sandeepa
>   [BUGFIX] kprobes/x86: Prohibit probing on debug_stack_*
>   [BUGFIX] x86: Prohibit probing on native_set_debugreg
>   [BUGFIX] x86: Prohibit probing on thunk functions and restore
>   [RFC] kprobes/x86: Call exception handlers directly from 
> do_int3/do_debug
>
>
>  Documentation/kprobes.txt |   16 +
>  arch/x86/include/asm/asm.h|7 ++
>  arch/x86/include/asm/kprobes.h|2 +
>  arch/x86/kernel/cpu/common.c  |4 +
>  arch/x86/kernel/entry_32.S|   33 ---
>  arch/x86/kernel/entry_64.S|   20 ---
>  arch/x86/kernel/kprobes/core.c|   32 --
>  arch/x86/kernel/paravirt.c|5 ++
>  arch/x86/kernel/traps.c   |   10 +++
>  arch/x86/lib/thunk_32.S   |3 +
>  arch/x86/lib/thunk_64.S   |3 +
>  include/asm-generic/vmlinux.lds.h |9 +++
>  include/linux/kprobes.h   |   21 ++-
>  kernel/kprobes.c  |  113 
> -
>  kernel/sched/core.c   |1
>  15 files changed, 147 insertions(+), 132 deletions(-)
>
> --
> Masami HIRAMATSU
> IT Management Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu...@hitachi.com
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v4 1/6] arm64: support single-step and breakpoint handler hooks

[Sandeepa Prabhu]

Hi Will,

Sorry for responding to this after long-time, I missed this review
during Linaro connect travels.

On 25 October 2013 20:52, Will Deacon  wrote:
> Hi Sandeepa,
>
> This is getting there, thanks for persevering with it. I still have a few
> minor comments though.
>
> On Thu, Oct 17, 2013 at 12:17:46PM +0100, Sandeepa Prabhu wrote:
>> AArch64 Single Steping and Breakpoint debug exceptions will be
>> used by multiple debug framworks like kprobes & kgdb.
>>
>> This patch implements the hooks for those frameworks to register
>> their own handlers for handling breakpoint and single step events.
>>
>> Reworked the debug exception handler in entry.S: do_dbg to route
>> software breakpoint (BRK64) exception to do_debug_exception()
>>
>> Signed-off-by: Sandeepa Prabhu 
>> Signed-off-by: Deepak Saxena 
>> ---
>>  arch/arm64/include/asm/debug-monitors.h | 21 
>>  arch/arm64/kernel/debug-monitors.c  | 86 
>> -
>>  arch/arm64/kernel/entry.S   |  2 +
>>  3 files changed, 108 insertions(+), 1 deletion(-)
>
> [...]
>
>> @@ -215,7 +257,10 @@ static int single_step_handler(unsigned long addr, 
>> unsigned int esr,
>>*/
>>   user_rewind_single_step(current);
>>   } else {
>> - /* TODO: route to KGDB */
>> + /* call registered single step handlers */
>
> Don't bother with this comment (it's crystal clear from the code).
OK, I will remove this unnecessary print.
>
>> + if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
>> + return 0;
>> +
>>   pr_warning("Unexpected kernel single-step exception at EL1\n");
>>   /*
>>* Re-enable stepping since we know that we will be
>> @@ -227,11 +272,50 @@ static int single_step_handler(unsigned long addr, 
>> unsigned int esr,
>>   return 0;
>>  }
>>
>> +
>> +static LIST_HEAD(break_hook);
>> +DEFINE_RWLOCK(break_hook_lock);
>
> This guy can be a plain old spinlock. That way, the readers have less
> overhead but things still work because we only call a single hook function.
well, kprobes need to support recursive breakpoints (i.e. breakpoint
handler executing BRK once again)
so I converted this lock to rw_lock.  I should put this info in commit
description to be more clearer.
Let me know if you find any issue with re-cursing in breakpoint exception?

Thanks,
Sandeepa
>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Re: Re: [PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

On 18 November 2013 12:25, Sandeepa Prabhu  wrote:
>>> This is generic test module (samples/hw_breakpoint/data_breakpoint.ko)
>>> which places watchpoint for bothe read/write.
>>> Atleast watchpt should have triggered for Read right? I also tried
>>> with othe functions like do_fork, vfs_read etc but no hit.
>>
>> You'd need to place something for exec if you want to see anything on the
>> instruction side. A read by the instruction fetcher does not trigger a read
>> watchpoint on ARM.
> Hmm, then watchpoint cannot not be placed on text address (instruction
> fetch path) right?
> (Sorry I did not check debug spec fully for watchpoint/hw breakpoints,
> I should do that)
>
>>
>>> >> 2.  Placing text breakpoint (modified sample module with attr.bp_type
>>> >> = HW_BREAKPOINT_X) upon vfs_symlink, and run "ln -s /dev/null
>>> >> /tmp/foo".  This time, breakpoint hit but exception is re-cursing
>>> >> infinitely!
>>> >
>>> > The problem here is that we expect the overflow handler to deal with the
>>> > stepping (like GDB does via ptrace). If you don't register a handler, the
>>> > kernel will do the step (like you would get if you used perf stat -e
>>> > mem:0x:x).
>>> [This test was done on upstream branch, without kprobes patches.]
>>> Hmm, then this is expected with test breakpoint right? is this
>>> handling to be done by perf and ptrace?
>>
>> perf stat doesn't register an overflow handler, so the hw_breakpoint
>> backend will handle the step. ptrace registers a handler which sends a
>> SIGTRAP to the debugger (e.g. gdb), which handles the step manually
>> (probably using a PTRACE_SINGLESTEP request).
>>
>>> I did not see arm64 support in linux/tools/perf/,  there are multiple
>>> patches in mailing list though. Are you aware of any version of perf
>>> that work with arm64?
>>
>> The perf tool should work fine on arm64 using mainline. Are you seeing
>> problems?
> Hmm basically perf is working and I can run tests now.
Hi Will,
Okay. I can see what you meant with respect to D-flag unmasking, on
original patchset(v2). :)

a. Placed a kprobe on vfs_read - works fine independently.
b. Placed hw_breakpoint on vfs_read using perf (perf record -e
mem:0xffc000134ba4:x -a -- sleep 10) and it works fine
independently.

c. Now, a+b, first placed kprobe on vfs_read and then ran perf record
event on vfs_read (hw breakpoint). Now, seeing that kprobe single step
is never complete/never disabled!, so debug exception would generate
forever! (Continuously printing "Unexpected kernel single-step
exception at EL1")

 kprobe pre_handler: p->addr = 0xffc000134ba4
 reenter_dbg: test API invoked
 kprobe post_handler: p->addr = 0xffc000134ba4
 fmp/fo^[[5Dreenter_dbg: test API invoked
 kprobe pre_handler: p->addr = 0xffc000134ba4
 Unexpected kernel single-step exception at EL1
 Unexpected kernel single-step exception at EL1


Once I change the location of D-flag manipulation (your suggestion)
and run enough tests, I would come back with more details and inputs.

Thanks,
Sandeepa

>>
>> Will
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Re: Re: [PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

>> This is generic test module (samples/hw_breakpoint/data_breakpoint.ko)
>> which places watchpoint for bothe read/write.
>> Atleast watchpt should have triggered for Read right? I also tried
>> with othe functions like do_fork, vfs_read etc but no hit.
>
> You'd need to place something for exec if you want to see anything on the
> instruction side. A read by the instruction fetcher does not trigger a read
> watchpoint on ARM.
Hmm, then watchpoint cannot not be placed on text address (instruction
fetch path) right?
(Sorry I did not check debug spec fully for watchpoint/hw breakpoints,
I should do that)

>
>> >> 2.  Placing text breakpoint (modified sample module with attr.bp_type
>> >> = HW_BREAKPOINT_X) upon vfs_symlink, and run "ln -s /dev/null
>> >> /tmp/foo".  This time, breakpoint hit but exception is re-cursing
>> >> infinitely!
>> >
>> > The problem here is that we expect the overflow handler to deal with the
>> > stepping (like GDB does via ptrace). If you don't register a handler, the
>> > kernel will do the step (like you would get if you used perf stat -e
>> > mem:0x:x).
>> [This test was done on upstream branch, without kprobes patches.]
>> Hmm, then this is expected with test breakpoint right? is this
>> handling to be done by perf and ptrace?
>
> perf stat doesn't register an overflow handler, so the hw_breakpoint
> backend will handle the step. ptrace registers a handler which sends a
> SIGTRAP to the debugger (e.g. gdb), which handles the step manually
> (probably using a PTRACE_SINGLESTEP request).
>
>> I did not see arm64 support in linux/tools/perf/,  there are multiple
>> patches in mailing list though. Are you aware of any version of perf
>> that work with arm64?
>
> The perf tool should work fine on arm64 using mainline. Are you seeing
> problems?
Hmm basically perf is working and I can run tests now.
>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

On 15 November 2013 22:07, Will Deacon  wrote:

>> well, kprobes does not step from kernel address, but it prepares a
>> allocated memory(executable),  copies the instruction and update the
>> single step address (ELR) to enable stepping while ERET.
>> So, don't we need NOP at next location after the instruction because
>> next instruction will be in decode stage and might throw "undefined
>> instruction" error?
>
> You can't take speculative prefetch aborts like that, so unless you actually
> go and *execute* garbage, you don't need that NOP. From the sounds of it, it's
> not required, as long as you handle the step exception correctly.
Ok, the stepping is for exactly one instruction, after trapping again
stepping is disabled.
so clear that NOP is not necessary. I will remove NOP in next version.

>> > Is this recursion only to support setting kprobes on the kprobe
>> > implementation? The problem is that the rest of the debug infrastructure is
>> > not set up to deal with recursive exceptions, so allowing them can break
>> > state machines maintained by code like hw_breakpoint.
>> No, upon one kprobe hit for an address, the subsystem can call the
>> user-defined handlers (pre- and -post) which can call same function
>> again. Example, if we place kprobe on "printk" entry, and registered
>> handler can invoke printk to print more info.
>
> Hang on, I think I'm missing something here. If you run into a recursive
> probe, you'll simply hit another BRK instruction, right? That should be
> fine, since PSTATE.D doesn't mask software breakpoint exceptions. The
> tricky part comes when you try to step over that guy, but you might be ok
> if you clear PSTATE.D *only* while you step your single instruction that you
> copied out to the buffer.
Yes exactly, D-flag was enabled so that single step can run on BRK
handler code! exactly once. So I was unmasking D-flag before
taking(expecting) a BRK so that saved spsr_el1 in pt_regs have it
stored and restore when return with single stepping!
I agree with your suggestion to unmask D-flag just before ERET for
stepping, makes more sense, is it as simple as updating pt_regs
(regs->pstate)?

>
> What do you think? I'd really like you to try testing something like:
>
>   1. Place a hardware breakpoint in the kernel
>   2. Place a kprobe on the same address
>   3. Place a kprobe somewhere in the pre- hook for the kprobe placed in (2)
>
> then check that (a) we manage to get through that lot without locking up and
> (b) each probe/breakpoint is hit exactly once.
Ok, will do, this can be easily done with kprobing "printk" with pre-
handler invoking printk, and then placing a hw_breakpoint.  (also we
need to check for kprobe-watchpoint concurrency right?)
I would update with results.

>
>> This will make kprobe to trigger again and re-enter, so the kprobe
>> subsystem need to handle the 2nd instance first, and then return back
>> to previous execution. D-flag is enabled only the duration when the
>> pre- and post- handler are called, so they they can recurse and handle
>> single stepping, after that, D-flag is kept disabled.   I am yet to
>> test the concurrency with hw_breakpoint, would update once I run these
>> tests.
>
> If you really want to support this, you need to do more than just clear the
> D flag. Not only do you need to deal with hardware breakpoints, but also
> things like scheduling... Assuming that the user-defined handlers can block,
> then you run the risk of context-switching with the D-flag set, which
> introduces a significant black-out period to kernel debugging. There are
> also issues like returning to userspace with MDSCR_EL1.SS set because of a
> context switch triggered by the pre- handler, resulting in a single-step
> exception from userspace.
kprobe user-defined handlers should not block no blocking calls) and
this is one of the requirements of kprobes, so we cannot manage
context-switching or interrupts until the kprobe trap is completely
handled. So, we basically disable local interrupts throughout
exception handler.

Masami,
kprobe framework cannot check if the  supplied user-handlers can block
right? and blocking call would cause undesired sequences, how to
handle this? (recommendations/policies?)

>
> I reckon what I suggested above might work, but I'd like your input.
Sure, would come back with more clear picture after running all tests
and making the changes as per your suggestion.

Thanks for reviewing this in detail,
-Sandeepa
>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] ARM64: perf: add support for the perf registers and dwarf unwinding

[Sandeepa Prabhu]

On 15 November 2013 22:12, Will Deacon  wrote:
> On Thu, Nov 14, 2013 at 11:04:04AM +0000, Sandeepa Prabhu wrote:
>> Hi Jean,
>>
>> I have applied this patchset on aarch64 upstream branch,cross-compiled
>> for arm64 and try running some tests for hardware breakpoints.
>>
>> I cross-compiled perf using linaro toolchain
>> "gcc-linaro-aarch64-linux-gnu-4.7-2013.04-20130415_linux" as
>>  $ cd tools/perf/
>>  $ make LDFLAGS=-static ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-
>>
>> Copied the static binary to my initrd image and tried running tests to
>> place hw_breakpoint on an kernel symbol as:
>>
>>  # perf record -e "mem:0xffc00013f640:x"
>
> Right, but that's not a valid invocation of perf. You have to give it
> something to record. Something like:
>
> # perf record -e mem:0xffc00013f640:x -a -- sleep 10
Hi Will,

Thanks for correcting me, now I see perf record working :)

~Sandeepa
>
> for example.
>
> Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] ARM64: perf: add support for the perf registers and dwarf unwinding

[Sandeepa Prabhu]

On 14 November 2013 16:45, Jean Pihet  wrote:
> Hi Sandeepa,
>
> On Thu, Nov 14, 2013 at 12:04 PM, Sandeepa Prabhu
>  wrote:
>> Hi Jean,
>>
>> I have applied this patchset on aarch64 upstream branch,cross-compiled
>> for arm64 and try running some tests for hardware breakpoints.
>>
>> I cross-compiled perf using linaro toolchain
>> "gcc-linaro-aarch64-linux-gnu-4.7-2013.04-20130415_linux" as
>>  $ cd tools/perf/
>>  $ make LDFLAGS=-static ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-
>>
>> Copied the static binary to my initrd image and tried running tests to
>> place hw_breakpoint on an kernel symbol as:
>>
>>  # perf record -e "mem:0xffc00013f640:x"
>> 737
>> 738  usage: perf record [] []
>> 739 or: perf record [] --  []
>> 740
>> 741 -e, --eventevent selector. use 'perf list' to list
>> available events
>> 742 --filter 
>> 743   event filter
>> 744 -p, --pidrecord events on existing process id
>> .
>>
>> Somehow perf is not able run record, seems not accepting any
>> arguments. Do you have any idea what could be going wrong?
> Is the behavior caused by this very patch set? This patch set is about
> callchain unwinding, which is unrelated to the hardward breakpoint
> feature.
> Is perf list reporting something? Can you check the '-e mem' command
> correctness?
>
>> How did you
>> verify/cross-compile perf? Is the tool-chain version wrong?
> Since perf depends on external libraries it is not straight forward to
> cross compile it. You can compile it as part of a build system (for
> example an OE based build system), or compile it natively on the
> target.
> I am compiling perf and the libraries (libunwind) natively in my case.
Hmm, looks like cross-compiling is not good option.
>
>>
>> Thanks,
>> Sandeepa
>
> Please let me know how it goes.
>
> Jean
I tried this and workig:
 # ./perf record -- /bin/ls

214 ^[[1;34mbin^[[0m   ^[[0;0mkprobe_dbg_addr.ko^[[0m
  ^[[1;34mroot^[[0m
215 ^[[1;34mdata^[[0m  ^[[0;0mkprobe_dbg_sym.ko^[[0m
  ^[[1;34msdcard^[[0m
216 ^[[0;0mdata_breakpoint.ko^[[0m^[[0;0mkprobe_example.ko^[[0m
 ^[[1;34msys^[[0m
217 ^[[1;36mdebug^[[0m
^[[0;0mkretprobe_example.ko^[[0m  ^[[1;34msystem^[[0m
218 ^[[1;34mdev^[[0m   ^[[1;32mperf^[[0m
   ^[[0;0mtext_breakpoint.ko^[[0m
219 ^[[1;34mhome^[[0m  ^[[0;0mperf.data^[[0m
  ^[[1;34mtmp^[[0m
220 ^[[1;32minit^[[0m  ^[[0;0mperf.data.old^[[0m
221 ^[[0;0mjprobe_example.ko^[[0m ^[[1;34mproc^[[0m
222 [ perf record: Woken up 1 times to write data ]
223 [ perf record: Captured and wrote 0.002 MB perf.data (~104 samples) ]
224 / #

Also perf list shows "mem" option and checked the command format also
correct. [perf record -e "mem:0xffc00013f640:x"]
In fact none of the options under -e are functioning.

[I had tried without your patchset and behavior was same]

>
>>
>> On 18 October 2013 20:24, Jean Pihet  wrote:
>>> From: Jean Pihet 
>>>
>>> This patch implements the functions required for the perf registers API,
>>> allowing the perf tool to interface kernel register dumps with libunwind
>>> in order to provide userspace backtracing.
>>> Only the general purpose user space registers are exported, i.e.:
>>>  PERF_REG_ARM_X0,
>>>  ...
>>>  PERF_REG_ARM_X28,
>>>  PERF_REG_ARM_FP,
>>>  PERF_REG_ARM_LR,
>>>  PERF_REG_ARM_SP,
>>>  PERF_REG_ARM_PC
>>> and not the PERF_REG_ARM_V* registers.
>>>
>>> Dependencies:
>>>  . if present, libunwind >= 1.1 is needed to prevent a segfault when
>>>parsing the dwarf info,
>>>  . libunwind needs to be configured with --enable-debug-frame. Note:
>>>--enable-debug-frame is automatically selected on ARM, NOT on ARM64.
>>>
>>> The generated perf binary has been tested on ARMv8 (using the
>>> foundation model simulator) and x86_64, using the following commands:
>>>  perf record -g [fp,dwarf] -- 
>>>  perf report --sort symbol --call-graph --stdio
>>>
>>>
>>> Jean Pihet (2):
>>>   ARM64: perf: add support for perf registers API
>>>   ARM64: perf: wire up perf_regs and unwind support
>>>
>>>  arch/arm64/Kconfig|  2 +
>>>  arch/arm64/include/uapi/asm/Kbuild|  1 +
>>>  arch/arm64/include/uapi/asm/perf_regs.h   | 40 ++
>>>  arch/arm64/kernel/Makefile|  1 +
>>>  arch/arm64/kernel/pe

Re: [PATCH 0/2] ARM64: perf: add support for the perf registers and dwarf unwinding

[Sandeepa Prabhu]

Hi Jean,

I have applied this patchset on aarch64 upstream branch,cross-compiled
for arm64 and try running some tests for hardware breakpoints.

I cross-compiled perf using linaro toolchain
"gcc-linaro-aarch64-linux-gnu-4.7-2013.04-20130415_linux" as
 $ cd tools/perf/
 $ make LDFLAGS=-static ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-

Copied the static binary to my initrd image and tried running tests to
place hw_breakpoint on an kernel symbol as:

 # perf record -e "mem:0xffc00013f640:x"
737
738  usage: perf record [] []
739 or: perf record [] --  []
740
741 -e, --eventevent selector. use 'perf list' to list
available events
742 --filter 
743   event filter
744 -p, --pidrecord events on existing process id
.

Somehow perf is not able run record, seems not accepting any
arguments. Do you have any idea what could be going wrong? How did you
verify/cross-compile perf? Is the tool-chain version wrong?

Thanks,
Sandeepa

On 18 October 2013 20:24, Jean Pihet  wrote:
> From: Jean Pihet 
>
> This patch implements the functions required for the perf registers API,
> allowing the perf tool to interface kernel register dumps with libunwind
> in order to provide userspace backtracing.
> Only the general purpose user space registers are exported, i.e.:
>  PERF_REG_ARM_X0,
>  ...
>  PERF_REG_ARM_X28,
>  PERF_REG_ARM_FP,
>  PERF_REG_ARM_LR,
>  PERF_REG_ARM_SP,
>  PERF_REG_ARM_PC
> and not the PERF_REG_ARM_V* registers.
>
> Dependencies:
>  . if present, libunwind >= 1.1 is needed to prevent a segfault when
>parsing the dwarf info,
>  . libunwind needs to be configured with --enable-debug-frame. Note:
>--enable-debug-frame is automatically selected on ARM, NOT on ARM64.
>
> The generated perf binary has been tested on ARMv8 (using the
> foundation model simulator) and x86_64, using the following commands:
>  perf record -g [fp,dwarf] -- 
>  perf report --sort symbol --call-graph --stdio
>
>
> Jean Pihet (2):
>   ARM64: perf: add support for perf registers API
>   ARM64: perf: wire up perf_regs and unwind support
>
>  arch/arm64/Kconfig|  2 +
>  arch/arm64/include/uapi/asm/Kbuild|  1 +
>  arch/arm64/include/uapi/asm/perf_regs.h   | 40 ++
>  arch/arm64/kernel/Makefile|  1 +
>  arch/arm64/kernel/perf_regs.c | 29 ++
>  tools/perf/arch/arm64/Makefile|  7 +++
>  tools/perf/arch/arm64/include/perf_regs.h | 88 
> +++
>  tools/perf/arch/arm64/util/dwarf-regs.c   | 81 
>  tools/perf/arch/arm64/util/unwind.c   | 82 
>  tools/perf/config/Makefile|  6 +++
>  10 files changed, 337 insertions(+)
>  create mode 100644 arch/arm64/include/uapi/asm/perf_regs.h
>  create mode 100644 arch/arm64/kernel/perf_regs.c
>  create mode 100644 tools/perf/arch/arm64/Makefile
>  create mode 100644 tools/perf/arch/arm64/include/perf_regs.h
>  create mode 100644 tools/perf/arch/arm64/util/dwarf-regs.c
>  create mode 100644 tools/perf/arch/arm64/util/unwind.c
>
> --
> 1.7.11.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Re: Re: [PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

On 13 November 2013 20:01, Will Deacon  wrote:
> On Wed, Nov 13, 2013 at 06:55:33AM +0000, Sandeepa Prabhu wrote:
>> >>> I'm unsure about arm64's debug feature behavior, what does happen when
>> >>> it performs a single-step on sw-breakpoint?
>> >>>
>> >>>> Sandeepa: I think you need to retry Masami's test on the arm64 model, 
>> >>>> since
>> >>>> I'm fairly sure it won't work as expected without some additional code.
>> >>>
>> >>> OK, anyway, for testing same one, we need to port ftrace first. So the 
>> >>> next
>> >
>> > Sorry for confusion, s/next/fallback is what I meant. Making a kprobe 
>> > module
>> > can be done without ftrace port.
>> >
>> >>> plan is to make a kprobe module to put a probe (which just printk 
>> >>> something)
>> >>> on a specific function (e.g. vfs_symlink), and run perf record with
>> >>> hw-breakpoint as below
>> >>>
>> >>> $ perf record -e "mem:0xXX:k" ln -s /dev/null /tmp/foo
>> >>>
>> >>> Note that 0xXX is the address of vfs_symlink.
>> >>>
>> >>> After that, you can see the message in dmesg and also check the perf 
>> >>> result
>> >>> with "sudo perf script --dump" (you can find a PERF_RECORD_SAMPLE entry 
>> >>> if
>> >>> it works)
>> Hi Will, Masami,
>>
>> I am not sure of 'perf' right now (my minimal rootfs doesn't have) but
>> I tried to test hardware breakpoints using sample modules
>> "samples/hw_breakpoint/" on arm64 upstream branch. This should use
>> same kernel api as perf I believe.
>>
>> 1.  Placing watchpoint ( attr.bp_type = HW_BREAKPOINT_W |
>> HW_BREAKPOINT_R) upon vfs_symlink symbol, but seems watch-point is not
>> triggering at all.
>
> vfs_symlink is a function. Why would you expect to write it?
This is generic test module (samples/hw_breakpoint/data_breakpoint.ko)
which places watchpoint for bothe read/write.
Atleast watchpt should have triggered for Read right? I also tried
with othe functions like do_fork, vfs_read etc but no hit.

>
>> 2.  Placing text breakpoint (modified sample module with attr.bp_type
>> = HW_BREAKPOINT_X) upon vfs_symlink, and run "ln -s /dev/null
>> /tmp/foo".  This time, breakpoint hit but exception is re-cursing
>> infinitely!
>
> The problem here is that we expect the overflow handler to deal with the
> stepping (like GDB does via ptrace). If you don't register a handler, the
> kernel will do the step (like you would get if you used perf stat -e
> mem:0x:x).
[This test was done on upstream branch, without kprobes patches.]
Hmm, then this is expected with test breakpoint right? is this
handling to be done by perf and ptrace?
I did not see arm64 support in linux/tools/perf/,  there are multiple
patches in mailing list though. Are you aware of any version of perf
that work with arm64?

~Sandeepa
>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Re: Re: [PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

On 13 November 2013 12:25, Sandeepa Prabhu  wrote:
>>>> I'm unsure about arm64's debug feature behavior, what does happen when
>>>> it performs a single-step on sw-breakpoint?
>>>>
>>>>> Sandeepa: I think you need to retry Masami's test on the arm64 model, 
>>>>> since
>>>>> I'm fairly sure it won't work as expected without some additional code.
>>>>
>>>> OK, anyway, for testing same one, we need to port ftrace first. So the next
>>
>> Sorry for confusion, s/next/fallback is what I meant. Making a kprobe module
>> can be done without ftrace port.
>>
>>>> plan is to make a kprobe module to put a probe (which just printk 
>>>> something)
>>>> on a specific function (e.g. vfs_symlink), and run perf record with
>>>> hw-breakpoint as below
>>>>
>>>> $ perf record -e "mem:0xXX:k" ln -s /dev/null /tmp/foo
>>>>
>>>> Note that 0xXX is the address of vfs_symlink.
>>>>
>>>> After that, you can see the message in dmesg and also check the perf result
>>>> with "sudo perf script --dump" (you can find a PERF_RECORD_SAMPLE entry if
>>>> it works)
Hi Will, Masami,

I am not sure of 'perf' right now (my minimal rootfs doesn't have) but
I tried to test hardware breakpoints using sample modules
"samples/hw_breakpoint/" on arm64 upstream branch. This should use
same kernel api as perf I believe.

1.  Placing watchpoint ( attr.bp_type = HW_BREAKPOINT_W |
HW_BREAKPOINT_R) upon vfs_symlink symbol, but seems watch-point is not
triggering at all.
2.  Placing text breakpoint (modified sample module with attr.bp_type
= HW_BREAKPOINT_X) upon vfs_symlink, and run "ln -s /dev/null
/tmp/foo".  This time, breakpoint hit but exception is re-cursing
infinitely!

I have attached the kernel logs for reference. So wanted to check if
hw breakpoint/watch-points are working on the upstream branch? Has it
been tested recently with sample modules  or perf/ptrace?

Thanks,
Sandeepa
Initializing cgroup subsys cpu
Linux version 3.12.0-rc4+ (sandeepa@linaro-workstation) (gcc version 4.7.3 20130328 (prerelease) (crosstool-NG linaro-1.13.1-4.7-2013.04-20130415 - Linaro GCC 2013.04) ) #24 SMP PREEMPT Wed Nov 13 12:04:03 IST 2013
CPU: AArch64 Processor [410fd0f0] revision 0
Machine: RTSM_VE_AEMv8A
bootconsole [earlycon0] enabled
PERCPU: Embedded 10 pages/cpu @ffc87ffa8000 s11776 r8192 d20992 u40960
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 1034240
Kernel command line: console=ttyAMA0 root=/dev/mmcblk0p2 earlyprintk=pl011,0x1c09 consolelog=9 rw
PID hash table entries: 4096 (order: 3, 32768 bytes)
Dentry cache hash table entries: 524288 (order: 10, 4194304 bytes)
Inode-cache hash table entries: 262144 (order: 9, 2097152 bytes)
software IO TLB [mem 0x8f800-0x8fc00] (64MB) mapped at [ffc87800-ffc87bff]
Memory: 4058384K/4194304K available (3347K kernel code, 211K rwdata, 1164K rodata, 171K init, 154K bss, 135920K reserved)
Virtual kernel memory layout:
vmalloc : 0xff80 - 0xffbb   (245759 MB)
vmemmap : 0xffbc01c0 - 0xffbc1f80   (   476 MB)
modules : 0xffbffc00 - 0xffc0   (64 MB)
memory  : 0xffc0 - 0xffc88000   ( 34816 MB)
  .init : 0xffc0004e9000 - 0xffc000513e00   (   172 kB)
  .text : 0xffc8 - 0xffc0004e8cf4   (  4516 kB)
  .data : 0xffc000514000 - 0xffc000548e80   (   212 kB)
SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1
Preemptible hierarchical RCU implementation.
	RCU restricting CPUs from NR_CPUS=8 to nr_cpu_ids=4.
NR_IRQS:64 nr_irqs:64 0
Architected cp15 timer(s) running at 100.00MHz (phys).
Console: colour dummy device 80x25
Calibrating delay loop (skipped), value calculated using timer frequency.. 200.00 BogoMIPS (lpj=100)
pid_max: default: 32768 minimum: 301
Mount-cache hash table entries: 256
hw perfevents: enabled with arm/armv8-pmuv3 PMU driver, 9 counters available
CPU1: Booted secondary processor
CPU2: Booted secondary processor
CPU3: Booted secondary processor
Brought up 4 CPUs
SMP: Total of 4 processors activated.
devtmpfs: initialized
atomic64 test passed
NET: Registered protocol family 16
of_amba_device_create(): amba_device_add() failed (-19) for /smb/motherboard/iofpga@3,/sysctl@02
vdso: 2 pages (1 code, 1 data) at base ffc000519000
hw-breakpoint: found 16 breakpoint and 16 watchpoint registers.
Serial: AMBA PL011 UART driver
1c09.uart: ttyAMA0 at MMIO 0x1c09 (irq = 37, base_baud = 0) is a PL011 rev2
console [ttyAMA0] enabled, bootconsole disabled
console [ttyAMA0] enabled, bootconsole disabled
1c0a.uart: ttyAMA1 at MMIO 0x1c0a (irq = 3

Re: Re: Re: Re: [PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

On 12 November 2013 15:47, Masami Hiramatsu
 wrote:
> (2013/11/12 17:44), Sandeepa Prabhu wrote:
>> On 12 November 2013 12:57, Masami Hiramatsu
>>  wrote:
>>> (2013/11/12 15:23), Sandeepa Prabhu wrote:
>>>>>>> OK, I've ensured that the hw_breakpoint (from perf) can work
>>>>>>> with kprobes (from ftrace) at the same address on x86.
>>>>>>> So if arm64 already support hw_breakpoint on perf, kprobes should
>>>>>>> work with it.
>>>>>>
>>>>>> Single-stepping on x86 is different to the step behaviour on arm64 
>>>>>> afaik. On
>>>>>> ARM, we have to manually remove the breakpoint, perform a single-step, 
>>>>>> then
>>>>>> add the breakpoint again. If we re-enable debug exceptions in the kprobe
>>>>>> handler, the step will complete early and we'll never step off the
>>>>>> breakpoint.
>>>>>
>>>>> I'm unsure about arm64's debug feature behavior, what does happen when
>>>>> it performs a single-step on sw-breakpoint?
>>>>>
>>>>>> Sandeepa: I think you need to retry Masami's test on the arm64 model, 
>>>>>> since
>>>>>> I'm fairly sure it won't work as expected without some additional code.
>>>>>
>>>>> OK, anyway, for testing same one, we need to port ftrace first. So the 
>>>>> next
>>>
>>> Sorry for confusion, s/next/fallback is what I meant. Making a kprobe module
>>> can be done without ftrace port.
>> Yes, got it, all my verification until now are done using sample
>> modules only,  looking out for perf (or some other mechanism: ptrace?)
>> that uses v8 hw breakpoint.
>
> Yes, kprobe vs. perf and uprobe vs. ptrace :)
>
>
>>>>> plan is to make a kprobe module to put a probe (which just printk 
>>>>> something)
>>>>> on a specific function (e.g. vfs_symlink), and run perf record with
>>>>> hw-breakpoint as below
>>>>>
>>>>> $ perf record -e "mem:0xXX:k" ln -s /dev/null /tmp/foo
>>>>>
>>>>> Note that 0xXX is the address of vfs_symlink.
>>>>>
>>>>> After that, you can see the message in dmesg and also check the perf 
>>>>> result
>>>>> with "sudo perf script --dump" (you can find a PERF_RECORD_SAMPLE entry if
>>>>> it works)
>>>> Thanks for steps, ARM64 ftrace patches are under review on arm mailing
>>>> list, I can contact the (linaro) developer implementing ftrace on
>>>> what's supported and then figure-out a way to test this concurrency of
>>>> kprobes breakpoint and hardware breakpoint.
>>>
>>> Would you mean this? :)
>>> http://www.spinics.net/lists/arm-kernel/msg278477.html
>>>
>>> Wow, it seems that this also has some works around instruction
>>> manipulation (and confusable filenames...)
>> I referred to: http://lwn.net/Articles/572323/  which is another
>> implementation and on LAKML
>
> OK, I'll check that (and looks good at a glance).
> By the way, I concern about Linaro guys who looks working a bit far
> from the LKML and original feature maintainers. Please contact them,
> I'm sure they don't bite your hand :)
Hmm sure, will convey to our developers/leads :-)

>
> BTW, I'm currently trying a general housecleaning of __kprobes
> annotations. It may also have impact on your patch.
> https://lkml.org/lkml/2013/11/8/187
Hmm, we can help testing your patchset on arm64 platforms. Also have
many doubts on the changes you are working [blacklisting probes etc]

Basically I had tried placing kprobe on memcpy() and the model hung
(insmod never returned back!). Fast-model I have does not have option
of any debug so no clue what happened!.
memcpy() is low-level call being used internally within kprobes, so
probably we cannot handle probe on that routine, but then how to make
sure all such API are rejected by kprobe sub-system ?

~Sandeepa
>
> Thank you,
>
> --
> Masami HIRAMATSU
> IT Management Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu...@hitachi.com
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Re: Re: [PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

On 12 November 2013 12:57, Masami Hiramatsu
 wrote:
> (2013/11/12 15:23), Sandeepa Prabhu wrote:
>>>>> OK, I've ensured that the hw_breakpoint (from perf) can work
>>>>> with kprobes (from ftrace) at the same address on x86.
>>>>> So if arm64 already support hw_breakpoint on perf, kprobes should
>>>>> work with it.
>>>>
>>>> Single-stepping on x86 is different to the step behaviour on arm64 afaik. 
>>>> On
>>>> ARM, we have to manually remove the breakpoint, perform a single-step, then
>>>> add the breakpoint again. If we re-enable debug exceptions in the kprobe
>>>> handler, the step will complete early and we'll never step off the
>>>> breakpoint.
>>>
>>> I'm unsure about arm64's debug feature behavior, what does happen when
>>> it performs a single-step on sw-breakpoint?
>>>
>>>> Sandeepa: I think you need to retry Masami's test on the arm64 model, since
>>>> I'm fairly sure it won't work as expected without some additional code.
>>>
>>> OK, anyway, for testing same one, we need to port ftrace first. So the next
>
> Sorry for confusion, s/next/fallback is what I meant. Making a kprobe module
> can be done without ftrace port.
Yes, got it, all my verification until now are done using sample
modules only,  looking out for perf (or some other mechanism: ptrace?)
that uses v8 hw breakpoint.
>
>>> plan is to make a kprobe module to put a probe (which just printk something)
>>> on a specific function (e.g. vfs_symlink), and run perf record with
>>> hw-breakpoint as below
>>>
>>> $ perf record -e "mem:0xXX:k" ln -s /dev/null /tmp/foo
>>>
>>> Note that 0xXX is the address of vfs_symlink.
>>>
>>> After that, you can see the message in dmesg and also check the perf result
>>> with "sudo perf script --dump" (you can find a PERF_RECORD_SAMPLE entry if
>>> it works)
>> Thanks for steps, ARM64 ftrace patches are under review on arm mailing
>> list, I can contact the (linaro) developer implementing ftrace on
>> what's supported and then figure-out a way to test this concurrency of
>> kprobes breakpoint and hardware breakpoint.
>
> Would you mean this? :)
> http://www.spinics.net/lists/arm-kernel/msg278477.html
>
> Wow, it seems that this also has some works around instruction
> manipulation (and confusable filenames...)
I referred to: http://lwn.net/Articles/572323/  which is another
implementation and on LAKML

>
> Thank you,
>
> --
> Masami HIRAMATSU
> IT Management Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu...@hitachi.com
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

On 11 November 2013 16:51, Will Deacon  wrote:
> On Mon, Nov 11, 2013 at 05:35:37AM +0000, Sandeepa Prabhu wrote:
>> On 8 November 2013 22:26, Will Deacon  wrote:
>> >> diff --git a/arch/arm64/include/asm/kprobes.h 
>> >> b/arch/arm64/include/asm/kprobes.h
>> >> new file mode 100644
>> >> index 000..9b491d0
>> >> --- /dev/null
>> >> +++ b/arch/arm64/include/asm/kprobes.h
>> >> @@ -0,0 +1,59 @@
>> >> +/*
>> >> + * arch/arm64/include/asm/kprobes.h
>> >> + *
>> >> + * Copyright (C) 2013 Linaro Limited
>> >> + *
>> >> + * 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.
>> >> + *
>> >> + * This program is distributed in the hope that it will be useful,
>> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> >> + * General Public License for more details.
>> >> + */
>> >> +
>> >> +#ifndef _ARM_KPROBES_H
>> >> +#define _ARM_KPROBES_H
>> >> +
>> >> +#include 
>> >> +#include 
>> >> +#include 
>> >> +
>> >> +#define __ARCH_WANT_KPROBES_INSN_SLOT
>> >> +#define MAX_INSN_SIZE  2
>> >
>> > Why is this 2?
>> Second entry is to hold NOP instruction, absence of it cause abort
>> while instruction decode.
>
> Hmm, can you elaborate please? I'm not sure why you should get an abort
> decoding kernel addresses.
well, kprobes does not step from kernel address, but it prepares a
allocated memory(executable),  copies the instruction and update the
single step address (ELR) to enable stepping while ERET.
So, don't we need NOP at next location after the instruction because
next instruction will be in decode stage and might throw "undefined
instruction" error?
I have removed the trailing NOP and tested single step and it worked
fine!, so I am not sure if  MAX_INSN_SIZE can be 1, I would check v8
debug spec again.

>
>> >> +DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
>> >> +DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>> >> +
>> >> +static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
>> >> +{
>> >> +   int i;
>> >> +   /* prepare insn slot */
>> >> +   p->ainsn.insn[0] = p->opcode;
>> >> +   /* NOP for superscalar uArch decode */
>> >
>> > superscalar uArch?
>> well, the comment needs refining, what we mean is that one NOP should
>> follow the instruction in memory slot, for the decode stage(not to hit
>> undefined instruction).
>
> Is this undef related to your comment above?
[Yes,  again, I don't know if trailing NOP is necessary as step is
working without it -decode stage for next location is not throwing
"undefined instruction" error while single stepping previous
instruction]

>
>> > NAK. Unmasking debug exceptions from within a debug exception is not safe.
>> > I'd much rather we returned from handling this exception, then took 
>> > whatever
>> > other pending exception there was.
>> well, kprobes needs recursive breakpoints to be handled, and I am not
>> sure if this can be achieved other way than unmasking D-flag for a
>> shorter duration where we can expect re-entry (I would check if this
>> can be done without re-cursing)
>> I want to understand why unmasking D-flag is unsafe here, kprobes make
>> sure that recursion depth is only 2 (i.e. does not generate 3rd
>> Breakpoint trap) and interrupts are kept masked while recursion/single
>> stepping. Is it unsafe only if conflict with hardware breakpoint on
>> same CPU?
>
> Is this recursion only to support setting kprobes on the kprobe
> implementation? The problem is that the rest of the debug infrastructure is
> not set up to deal with recursive exceptions, so allowing them can break
> state machines maintained by code like hw_breakpoint.
No, upon one kprobe hit for an address, the subsystem can call the
user-defined handlers (pre- and -post) which can call same function
again. Example, if we place kprobe on "printk" entry, and registered
handler can invoke printk to print more info.
This will make kprobe to trigger again and re-enter, so the kprobe
subsystem need to handle the 2nd instance first, and then return back
to previous execution. D-

Re: Re: [PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

>>> OK, I've ensured that the hw_breakpoint (from perf) can work
>>> with kprobes (from ftrace) at the same address on x86.
>>> So if arm64 already support hw_breakpoint on perf, kprobes should
>>> work with it.
>>
>> Single-stepping on x86 is different to the step behaviour on arm64 afaik. On
>> ARM, we have to manually remove the breakpoint, perform a single-step, then
>> add the breakpoint again. If we re-enable debug exceptions in the kprobe
>> handler, the step will complete early and we'll never step off the
>> breakpoint.
>
> I'm unsure about arm64's debug feature behavior, what does happen when
> it performs a single-step on sw-breakpoint?
>
>> Sandeepa: I think you need to retry Masami's test on the arm64 model, since
>> I'm fairly sure it won't work as expected without some additional code.
>
> OK, anyway, for testing same one, we need to port ftrace first. So the next
> plan is to make a kprobe module to put a probe (which just printk something)
> on a specific function (e.g. vfs_symlink), and run perf record with
> hw-breakpoint as below
>
> $ perf record -e "mem:0xXX:k" ln -s /dev/null /tmp/foo
>
> Note that 0xXX is the address of vfs_symlink.
>
> After that, you can see the message in dmesg and also check the perf result
> with "sudo perf script --dump" (you can find a PERF_RECORD_SAMPLE entry if
> it works)
Thanks for steps, ARM64 ftrace patches are under review on arm mailing
list, I can contact the (linaro) developer implementing ftrace on
what's supported and then figure-out a way to test this concurrency of
kprobes breakpoint and hardware breakpoint.

Thanks,
Sandeepa
>
> Thank you,
>
> --
> Masami HIRAMATSU
> IT Management Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu...@hitachi.com
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC 4/6] arm64: Add kernel return probes support(kretprobes)

[Sandeepa Prabhu]

On 11 November 2013 13:23, AKASHI Takahiro  wrote:
> On 11/11/2013 01:29 PM, Sandeepa Prabhu wrote:
>>
>> On 8 November 2013 22:34, Will Deacon  wrote:
>>>>
>>>> +static inline long regs_return_value(struct pt_regs *regs)
>>>> +{
>>>> + return regs->regs[0];
>>>> +}
>>>
>>>
>>> This is also being implemented by another patch series (I think the audit
>>> stuff?).
>>
>> Not sure, I did not see this being implemented in audit(audit adds for
>> 'syscallno',  not for return value in x0)
>> I can rebase my code if this change is implemented and queued in other
>> patchset.
>
>
> Yes, my audit patch has it.
> The definition is referred to in include/linux/audit.h (and ptrace.h),
> too.
Thanks, I will rebase kprobes code to use your API.
>
> -Takahiro AKASHI
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC 3/6] arm64: Kprobes instruction simulation support

[Sandeepa Prabhu]

On 8 November 2013 22:33, Will Deacon  wrote:
> On Thu, Oct 17, 2013 at 12:17:48PM +0100, Sandeepa Prabhu wrote:
>> Add support for AArch64 instruction simulation in kprobes.
>>
>> Kprobes need simulation of instructions that cannot be stepped
>> right-away from different memory location. i.e. those instructions
>> that uses PC-relative addressing. In simulation, the behaviour
>> of the instruction is implemented using copy of pt_regs.
>>
>> Following instruction catagories are simulated:
>>  - All branching instructions(conditional, register, and immediate)
>>  - Literal access instructions(load-literal, adr/adrp)
>>
>> conditional execution are limited to branching instructions in
>> ARM v8. If conditions at PSTATE does not match the condition fields
>> of opcode, the instruction is effectively NOP. Kprobes consider
>> this case as 'miss'.
>
> [...]
>
>> diff --git a/arch/arm64/kernel/kprobes-arm64.c 
>> b/arch/arm64/kernel/kprobes-arm64.c
>> index 30d1c14..c690be3 100644
>> --- a/arch/arm64/kernel/kprobes-arm64.c
>> +++ b/arch/arm64/kernel/kprobes-arm64.c
>> @@ -20,6 +20,101 @@
>>
>>  #include "probes-decode.h"
>>  #include "kprobes-arm64.h"
>> +#include "simulate-insn.h"
>> +
>> +/*
>> + * condition check functions for kprobes simulation
>> + */
>> +static unsigned long __kprobes
>> +__check_pstate(struct kprobe *p, struct pt_regs *regs)
>> +{
>> +   struct arch_specific_insn *asi = &p->ainsn;
>> +   unsigned long pstate = regs->pstate & 0x;
>> +
>> +   return asi->pstate_cc(pstate);
>> +}
>> +
>> +static unsigned long __kprobes
>> +__check_cbz(struct kprobe *p, struct pt_regs *regs)
>> +{
>> +   return check_cbz((u32)p->opcode, regs);
>
> Isn't p->opcode already a u32? (by your definition of kprobe_opcode_t).
Yup, can avoid typecasting.
>
>> diff --git a/arch/arm64/kernel/simulate-insn.c 
>> b/arch/arm64/kernel/simulate-insn.c
>> new file mode 100644
>> index 000..10173cf
>> --- /dev/null
>> +++ b/arch/arm64/kernel/simulate-insn.c
>> @@ -0,0 +1,184 @@
>> +/*
>> + * arch/arm64/kernel/simulate-insn.c
>> + *
>> + * Copyright (C) 2013 Linaro Limited.
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "simulate-insn.h"
>> +
>> +#define sign_extend(x, signbit)\
>> +   ((x) | (0 - ((x) & (1 << (signbit)
>> +
>> +#define bbl_displacement(insn) \
>> +   sign_extend(((insn) & 0x3ff) << 2, 27)
>> +
>> +#define bcond_displacement(insn)   \
>> +   sign_extend(((insn >> 5) & 0xf) << 2, 21)
>> +
>> +#define cbz_displacement(insn) \
>> +   sign_extend(((insn >> 5) & 0xf) << 2, 21)
>> +
>> +#define tbz_displacement(insn) \
>> +   sign_extend(((insn >> 5) & 0x3fff) << 2, 15)
>> +
>> +#define ldr_displacement(insn) \
>> +   sign_extend(((insn >> 5) & 0xf) << 2, 21)
>
> The mask, shift and signbit position are all related here, so you could
> rework the definition of sign_extend to avoid having three magic numbers.
Hmm, mask and signbit are related, shift is based on instruction type
(conditional instructions need >> 5 for extracting immediate offset).
I will refine these macros in next version.

>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Re: [PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

On 9 November 2013 14:40, Masami Hiramatsu
 wrote:
> (2013/11/09 1:56), Will Deacon wrote:
>> Hi Sandeepa,
>>
>> On Thu, Oct 17, 2013 at 12:17:47PM +0100, Sandeepa Prabhu wrote:
>>> Add support for basic kernel probes(kprobes), jump probes (jprobes)
>>> for ARM64.
>>
>> I think this series will conflict quite heavily with the jump_label series,
>> since they both introduce some common instruction manipulation code. On the
>> debug side, there will also be conflicts with the kgdb series, so it might
>> make sense for us to merge those two first, then you can rebase on a stable
>> branch from us.
>
> [...]
>
>> In fact, how do you avoid a race with hardware breakpoints? E.g., somebody
>> places a hardware breakpoint on an instruction in the kernel for which
>> kprobes has patched in a brk. We take the hardware breakpoint, disable the
>> breakpoint and set up a single step before returning to the brk. The brk
>> then traps, but we must take care not to disable single-step and/or unmask
>> debug exceptions, because that will cause the hardware breakpoint code to
>> re-arm its breakpoint before we've stepped off the brk instruction.
>
> Hmm, frankly to say, this kind of race issue is not seriously discussed
> on x86 too, since kgdb is still a special tool (not used on the production
> system).
> I think under such situation kgdb operator must have full control of the
> system, and he can (and has to) avoid such kind of race.
Masami,

Hmm I think in same lines, but not sure if we expect kprobes to be
able to work fool-proof along with kgdb or hw breakpoints ?

Thanks,
Sandeepa
>
> Thank you,
>
> --
> Masami HIRAMATSU
> IT Management Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu...@hitachi.com
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

On 8 November 2013 22:26, Will Deacon  wrote:
> Hi Sandeepa,
>
> On Thu, Oct 17, 2013 at 12:17:47PM +0100, Sandeepa Prabhu wrote:
>> Add support for basic kernel probes(kprobes), jump probes (jprobes)
>> for ARM64.
>
> I think this series will conflict quite heavily with the jump_label series,
> since they both introduce some common instruction manipulation code. On the
> debug side, there will also be conflicts with the kgdb series, so it might
> make sense for us to merge those two first, then you can rebase on a stable
> branch from us.
Yes, I understand, also the 3 features (kgdb, kprobes, jump_labels)
share some API dependencies.
I can rebase and re-post kprobes after the other 2 are merged.

The patch  "[PATCH RFC v3 1/5] AArch64: Add single-step and breakpoint
handler hooks"  will merge first along with kgdb?

>
> [...]
>
>> diff --git a/arch/arm64/include/asm/kprobes.h 
>> b/arch/arm64/include/asm/kprobes.h
>> new file mode 100644
>> index 000..9b491d0
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/kprobes.h
>> @@ -0,0 +1,59 @@
>> +/*
>> + * arch/arm64/include/asm/kprobes.h
>> + *
>> + * Copyright (C) 2013 Linaro Limited
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + */
>> +
>> +#ifndef _ARM_KPROBES_H
>> +#define _ARM_KPROBES_H
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define __ARCH_WANT_KPROBES_INSN_SLOT
>> +#define MAX_INSN_SIZE  2
>
> Why is this 2?
Second entry is to hold NOP instruction, absence of it cause abort
while instruction decode.

>
>> +#define MAX_STACK_SIZE 128
>> +
>> +#define flush_insn_slot(p) do { } while (0)
>> +#define kretprobe_blacklist_size   0
>> +
>> +#include 
>> +
>> +struct prev_kprobe {
>> +   struct kprobe *kp;
>> +   unsigned int status;
>> +};
>> +
>> +/* Single step context for kprobe */
>> +struct kprobe_step_ctx {
>> +#define KPROBES_STEP_NONE  0x0
>> +#define KPROBES_STEP_PENDING   0x1
>
> Maybe use an enum to stay consistent with what you did for pc_restore_t?
OK, I will change it to enums.
>
>> diff --git a/arch/arm64/kernel/kprobes-arm64.c 
>> b/arch/arm64/kernel/kprobes-arm64.c
>> new file mode 100644
>> index 000..30d1c14
>> --- /dev/null
>> +++ b/arch/arm64/kernel/kprobes-arm64.c
>> @@ -0,0 +1,211 @@
>> +/*
>> + * arch/arm64/kernel/kprobes-arm64.c
>> + *
>> + * Copyright (C) 2013 Linaro Limited.
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "probes-decode.h"
>> +#include "kprobes-arm64.h"
>> +
>> +/* AArch64 instruction decode table for kprobes:
>> + * The instruction will fall into one of the 3 groups:
>> + *  1. Single stepped out-of-the-line slot.
>> + * -Most instructions fall in this group, those does not
>> + *  depend on PC address.
>> + *
>> + *  2. Should be simulated because of PC-relative/literal access.
>> + * -All branching and PC-relative insrtcutions are simulated
>> + *  in C code, making use of saved pt_regs
>> + *  Catch: SIMD/NEON register context are not saved while
>> + *  entering debug exception, so are rejected for now.
>> + *
>> + *  3. Cannot be probed(not safe) so are rejected.
>> + * - Exception generation and exception return instructions
>> + * - Exclusive monitor(LDREX/STREX family)
>> + *
>> + */
>> +static const struct aarch64_decode_item aarch64_decode_table[] = {
>> +   /*
>> +* Data processing -

Re: [PATCH RFC 4/6] arm64: Add kernel return probes support(kretprobes)

[Sandeepa Prabhu]

On 8 November 2013 22:34, Will Deacon  wrote:
> On Thu, Oct 17, 2013 at 12:17:49PM +0100, Sandeepa Prabhu wrote:
>> AArch64 ISA does not instructions to pop PC register value
>> from stack(like ARM v7 has ldmia {...,pc}) without using
>> one of the general purpose registers. This means return probes
>> cannot return to the actual return address directly without
>> modifying register context, and without trapping into debug exception.
>>
>> So like many other architectures, we prepare a global routine
>> with NOPs, which serve as trampoline to hack away the
>> function return address, by placing an extra kprobe on the
>> trampoline entry.
>>
>> The pre-handler of this special trampoline' kprobe execute return
>> probe handler functions and restore original return address in ELR_EL1,
>> this way, saved pt_regs still hold the original register context to be
>> carried back to the probed kernel function.
>>
>> Signed-off-by: Sandeepa Prabhu 
>> ---
>>  arch/arm64/Kconfig   |   1 +
>>  arch/arm64/include/asm/kprobes.h |   1 +
>>  arch/arm64/include/asm/ptrace.h  |   5 ++
>>  arch/arm64/kernel/kprobes.c  | 125 
>> ++-
>>  4 files changed, 129 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 2e89059..73eff55 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -28,6 +28,7 @@ config ARM64
>>   select HAVE_MEMBLOCK
>>   select HAVE_PERF_EVENTS
>>   select HAVE_KPROBES if !XIP_KERNEL
>> + select HAVE_KRETPROBES if (HAVE_KPROBES)
>
> Don't need the brackets.
OK.
>
>>   select IRQ_DOMAIN
>>   select MODULES_USE_ELF_RELA
>>   select NO_BOOTMEM
>> diff --git a/arch/arm64/include/asm/kprobes.h 
>> b/arch/arm64/include/asm/kprobes.h
>> index 9b491d0..eaca849 100644
>> --- a/arch/arm64/include/asm/kprobes.h
>> +++ b/arch/arm64/include/asm/kprobes.h
>> @@ -55,5 +55,6 @@ void arch_remove_kprobe(struct kprobe *);
>>  int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
>>  int kprobe_exceptions_notify(struct notifier_block *self,
>>unsigned long val, void *data);
>> +void kretprobe_trampoline(void);
>>
>>  #endif /* _ARM_KPROBES_H */
>> diff --git a/arch/arm64/include/asm/ptrace.h 
>> b/arch/arm64/include/asm/ptrace.h
>> index 89f1727..58b2589 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -166,6 +166,11 @@ static inline int valid_user_regs(struct user_pt_regs 
>> *regs)
>>  #define instruction_pointer(regs)(regs)->pc
>>  #define stack_pointer(regs)  ((regs)->sp)
>>
>> +static inline long regs_return_value(struct pt_regs *regs)
>> +{
>> + return regs->regs[0];
>> +}
>
> This is also being implemented by another patch series (I think the audit
> stuff?).
Not sure, I did not see this being implemented in audit(audit adds for
'syscallno',  not for return value in x0)
I can rebase my code if this change is implemented and queued in other patchset.

>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC 6/6] kprobes: Add cases for arm and arm64 in sample module

[Sandeepa Prabhu]

On 25 October 2013 20:54, Will Deacon  wrote:
> On Thu, Oct 17, 2013 at 12:17:51PM +0100, Sandeepa Prabhu wrote:
>> Add info prints in sample kprobe handlers for ARM and ARM64
>> architecture.
>>
>> Signed-off-by: Sandeepa Prabhu 
>> ---
>>  samples/kprobes/kprobe_example.c | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/samples/kprobes/kprobe_example.c 
>> b/samples/kprobes/kprobe_example.c
>> index 366db1a..0521246 100644
>> --- a/samples/kprobes/kprobe_example.c
>> +++ b/samples/kprobes/kprobe_example.c
>> @@ -42,6 +42,14 @@ static int handler_pre(struct kprobe *p, struct pt_regs 
>> *regs)
>>   " ex1 = 0x%lx\n",
>>   p->addr, regs->pc, regs->ex1);
>>  #endif
>> +#ifdef CONFIG_ARM
>> + printk(KERN_INFO "pre_handler: p->addr = 0x%p, pc = 0x%lx\n",
>> + p->addr, regs->ARM_pc);
>> +#endif
>> +#ifdef CONFIG_ARM64
>> + printk(KERN_INFO "pre_handler: p->addr = 0x%p, pc = 0x%lx\n",
>> + p->addr, (long)regs->pc);
>> +#endif
>
> Huh? Why can't you combine these two together and either unconditionall cast
> to long, or use void * and %p?
pt_regs member names are different: regs->pc (arm64) vs. regs->ARM_pc
(arm32). It is still possible to use instruction_pointer(regs) for
both architectures, but this needs including asm/ptrace.h which I am
not sure good thing to do in samples/

Thanks,
Sandeepa
>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 2/7] arm64: introduce interfaces to hotpatch kernel and module code

[Sandeepa Prabhu]

On 3 November 2013 23:55, Jiang Liu  wrote:
> On 10/30/2013 08:12 AM, Will Deacon wrote:
>> Hi Jinag Liu,
>>
>> Sorry for the delayed review, I've been travelling.
>>
>> On Fri, Oct 18, 2013 at 04:19:56PM +0100, Jiang Liu wrote:
>>> From: Jiang Liu 
>>
>> If I try and email you at your Huawei address, I get a bounce from the mail
>> server. Is that expected? If so, it's not very helpful from a commit log
>> perspective if you use that address as the author on your patches.
>>
> Hi Will,
> Sorry for the inconvenience. I have left Huawei recently and
> have had a vacation last two weeks. I will use my gmail account next
> time.
>
>>> Introduce three interfaces to patch kernel and module code:
>>> aarch64_insn_patch_text_nosync():
>>>  patch code without synchronization, it's caller's responsibility
>>>  to synchronize all CPUs if needed.
>>> aarch64_insn_patch_text_sync():
>>>  patch code and always synchronize with stop_machine()
>>> aarch64_insn_patch_text():
>>>  patch code and synchronize with stop_machine() if needed
>>>
>>> Signed-off-by: Jiang Liu 
>>> Cc: Jiang Liu 
>>> ---
>>>  arch/arm64/include/asm/insn.h | 19 -
>>>  arch/arm64/kernel/insn.c  | 91 
>>> +++
>>>  2 files changed, 109 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>>> index 7499490..7a69491 100644
>>> --- a/arch/arm64/include/asm/insn.h
>>> +++ b/arch/arm64/include/asm/insn.h
>>> @@ -71,8 +71,25 @@ enum aarch64_insn_hint_op {
>>>
>>>  bool aarch64_insn_is_nop(u32 insn);
>>>
>>> -enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
>>> +/*
>>> + * In ARMv8-A, A64 instructions have a fixed length of 32 bits and are 
>>> always
>>> + * little-endian.
>>> + */
>>> +static __always_inline u32 aarch64_insn_read(void *addr)
>>> +{
>>> +return le32_to_cpu(*(u32 *)addr);
>>> +}
>>>
>>> +static __always_inline void aarch64_insn_write(void *addr, u32 insn)
>>> +{
>>> +*(u32 *)addr = cpu_to_le32(insn);
>>> +}
>>
>> I wouldn't bother with these helpers. You should probably be using
>> probe_kernel_address or similar, then doing the endianness swabbing on the
>> return value in-line.
> How about keeping and refining aarch64_insn_read/write interfaces
> by using probe_kernel_address()? I think they may be used in other
> places when supporting big endian ARM64 kernel.
>
>>
>>> +enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
>>>  bool aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn);
>>>
>>> +int aarch64_insn_patch_text_nosync(void *addr, u32 insn);
>>> +int aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt);
>>> +int aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt);
>>> +
>>>  #endif  /* __ASM_INSN_H */
>>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
>>> index f5b779f..3879db4 100644
>>> --- a/arch/arm64/kernel/insn.c
>>> +++ b/arch/arm64/kernel/insn.c
>>> @@ -16,6 +16,9 @@
>>>   */
>>>  #include 
>>>  #include 
>>> +#include 
>>> +#include 
>>> +#include 
>>>  #include 
>>>
>>>  static int aarch64_insn_encoding_class[] = {
>>> @@ -88,3 +91,91 @@ bool __kprobes aarch64_insn_hotpatch_safe(u32 old_insn, 
>>> u32 new_insn)
>>>  return __aarch64_insn_hotpatch_safe(old_insn) &&
>>> __aarch64_insn_hotpatch_safe(new_insn);
>>>  }
>>> +
>>> +int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
>>> +{
>>> +u32 *tp = addr;
>>> +
>>> +/* A64 instructions must be word aligned */
>>> +if ((uintptr_t)tp & 0x3)
>>> +return -EINVAL;
>>> +
>>> +aarch64_insn_write(tp, insn);
>>> +flush_icache_range((uintptr_t)tp, (uintptr_t)tp + sizeof(u32));
>>
>> sizeof(insn) is clearer here.
>>
> Will make this change in next version.
>
>>> +
>>> +return 0;
>>> +}
>>> +
>>> +struct aarch64_insn_patch {
>>> +void**text_addrs;
>>> +u32 *new_insns;
>>> +int insn_cnt;
>>> +};
>>> +
>>> +static DEFINE_MUTEX(text_patch_lock);
>>> +static atomic_t text_patch_id;
>>> +
>>> +static int __kprobes aarch64_insn_patch_text_cb(void *arg)
>>> +{
>>> +int i, ret = 0;
>>> +struct aarch64_insn_patch *pp = arg;
>>> +
>>> +if (atomic_read(&text_patch_id) == smp_processor_id()) {
>>
>> You could actually pass in the test_patch_id as zero-initialised parameter
>> to this function (i.e. it points to something on the stack of the guy
>> issuing the stop_machine). Then you do an inc_return here. If you see zero,
>> you go ahead and do the patching.
> Good suggestion!
> Function stop_machine() already has a mutex to serialize all callers,
> so we don't need explicitly serialization here. Will simplify the
> code in next version.
>
>>
>>> +for (i = 0; ret == 0 && i < pp->insn_cnt; i++)
>>> +ret = aarch64_insn_patch_text_nosync(pp->text_addrs[i],
>>> + pp->new_insns[i]);
>>> +

Re: [PATCH RFC v2 0/6] ARM64: Add kernel probes(Kprobes) support

[Sandeepa Prabhu]

On 18 October 2013 14:02, Masami Hiramatsu
 wrote:
> (2013/10/17 20:17), Sandeepa Prabhu wrote:
>> This patchset adds support for kernel probes(kprobes), jump probes(jprobes)
>> and return probes(kretprobes) support for ARM64.
>>
>> Kprobes mechanism make use of software breakpoint and single stepping
>> support available in ARM v8 kernel.
>>
>
> Thank you! This looks much better for me now. ;)
>
>> This patchset make use of (and dependant upon) dynamic kernel patching
>> feature published in patch series: https://lkml.org/lkml/2013/10/15/891
>>
>> changes: v1 -> v2
>>  1. Implemented review comments on v1
>>  2. Debug montior hooks implementation is changed to use rwlocks
>> instead of rcu and spinlock.
>
> Eventually, we'd better reconsider using rcu there, or prohibit probing
> readlock related functions. For the first step, this will be good.
Okay, can be done while optimizing kprobes right, since systemtap/ktap
is yet to be verified on this patchset,
so can expect more scope for optimizing the paths for performance.
>
>>  3. Enabled recursing in kprobes handler for kprobes re-enter support.
>>  4. Re-split the patchset to seperate single-stepping, simulation and
>> kretprobes features.
>>  5. instruction simulation is made independent of 'struct kprobes'
>>  6. Added 'Linaro Copyright' statements in new added files.
>>  7. Used arm64 instead of aarch64 in file names and comments.
>>
>> Tested on ARM v8 fast model with sample modules from: samples/kprobes/
>>
>> Sandeepa Prabhu (6):
>>   arm64: support single-step and breakpoint handler hooks
>>   arm64: Kprobes with single stepping support
>>   arm64: Kprobes instruction simulation support
>>   arm64: Add kernel return probes support(kretprobes)
>>   arm64: Enable kprobes support for arm64 platform
>>   kprobes: Add cases for arm and arm64 in sample module
>>
>>  arch/arm64/Kconfig  |   2 +
>>  arch/arm64/configs/defconfig|  20 +-
>>  arch/arm64/include/asm/debug-monitors.h |  21 +
>>  arch/arm64/include/asm/kprobes.h|  60 +++
>>  arch/arm64/include/asm/probes.h |  50 +++
>>  arch/arm64/include/asm/ptrace.h |   6 +
>>  arch/arm64/kernel/Makefile  |   2 +
>>  arch/arm64/kernel/condn-helpers.c   | 120 ++
>>  arch/arm64/kernel/debug-monitors.c  |  86 +++-
>>  arch/arm64/kernel/entry.S   |   2 +
>
>
>>  arch/arm64/kernel/kprobes-arm64.c   | 313 +++
>>  arch/arm64/kernel/kprobes-arm64.h   |  30 ++
>
> One comment, this name looks a bit wired. Since it seems that these are
> for instruction decoding, can we merge it with probe-decode.h and
> rename it as probe-decode.{h,c}? When we implement uprobes, we still
> can expand it (add the decoding table for uprobes in the probe-decode.c).
probe-decode.c looks fine, as uprobes decode table can be added in
same place. -TODO for the next version.

>
>>  arch/arm64/kernel/kprobes.c | 682 
>> 
>>  arch/arm64/kernel/kprobes.h |  30 ++
>>  arch/arm64/kernel/probes-decode.h   | 110 ++
>>  arch/arm64/kernel/simulate-insn.c   | 184 +
>>  arch/arm64/kernel/simulate-insn.h   |  33 ++
>>  arch/arm64/kernel/vmlinux.lds.S |   1 +
>>  samples/kprobes/kprobe_example.c|  16 +
>>  19 files changed, 1756 insertions(+), 12 deletions(-)
>
> BTW, is there any public git repository which has this series?
Yes, uploaded on linaro git:
https://git.linaro.org/gitweb?p=people/sandeepa.prabhu/linux-aarch64.git;a=shortlog;h=refs/heads/arm64-kprobes-v2

>
> Thank you again!
>
> --
> Masami HIRAMATSU
> IT Management Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu...@hitachi.com
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH RFC v4 1/6] arm64: support single-step and breakpoint handler hooks

[Sandeepa Prabhu]

AArch64 Single Steping and Breakpoint debug exceptions will be
used by multiple debug framworks like kprobes & kgdb.

This patch implements the hooks for those frameworks to register
their own handlers for handling breakpoint and single step events.

Reworked the debug exception handler in entry.S: do_dbg to route
software breakpoint (BRK64) exception to do_debug_exception()

Signed-off-by: Sandeepa Prabhu 
Signed-off-by: Deepak Saxena 
---
 arch/arm64/include/asm/debug-monitors.h | 21 
 arch/arm64/kernel/debug-monitors.c  | 86 -
 arch/arm64/kernel/entry.S   |  2 +
 3 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h 
b/arch/arm64/include/asm/debug-monitors.h
index a2232d0..6231479 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -62,6 +62,27 @@ struct task_struct;
 
 #define DBG_ARCH_ID_RESERVED   0   /* In case of ptrace ABI updates. */
 
+#define DBG_HOOK_HANDLED   0
+#define DBG_HOOK_ERROR 1
+
+struct step_hook {
+   struct list_head node;
+   int (*fn)(struct pt_regs *regs, unsigned int esr);
+};
+
+void register_step_hook(struct step_hook *hook);
+void unregister_step_hook(struct step_hook *hook);
+
+struct break_hook {
+   struct list_head node;
+   u32 esr_val;
+   u32 esr_mask;
+   int (*fn)(struct pt_regs *regs, unsigned int esr);
+};
+
+void register_break_hook(struct break_hook *hook);
+void unregister_break_hook(struct break_hook *hook);
+
 u8 debug_monitors_arch(void);
 
 void enable_debug_monitors(enum debug_el el);
diff --git a/arch/arm64/kernel/debug-monitors.c 
b/arch/arm64/kernel/debug-monitors.c
index cbfacf7..e42ad5f 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -188,6 +188,48 @@ static void clear_regs_spsr_ss(struct pt_regs *regs)
regs->pstate = spsr;
 }
 
+/* EL1 Single Step Handler hooks */
+static LIST_HEAD(step_hook);
+DEFINE_RWLOCK(step_hook_lock);
+
+void register_step_hook(struct step_hook *hook)
+{
+   write_lock(&step_hook_lock);
+   list_add(&hook->node, &step_hook);
+   write_unlock(&step_hook_lock);
+}
+
+void unregister_step_hook(struct step_hook *hook)
+{
+   write_lock(&step_hook_lock);
+   list_del(&hook->node);
+   write_unlock(&step_hook_lock);
+}
+
+/*
+ * Call registered single step handers
+ * There is no Syndrome info to check for determining the handler.
+ * So we call all the registered handlers, until the right handler is
+ * found which returns zero.
+ */
+static int call_step_hook(struct pt_regs *regs, unsigned int esr)
+{
+   struct step_hook *hook;
+   int retval = DBG_HOOK_ERROR;
+
+   read_lock(&step_hook_lock);
+
+   list_for_each_entry(hook, &step_hook, node) {
+   retval = hook->fn(regs, esr);
+   if (retval == DBG_HOOK_HANDLED)
+   break;
+   }
+
+   read_unlock(&step_hook_lock);
+
+   return retval;
+}
+
 static int single_step_handler(unsigned long addr, unsigned int esr,
   struct pt_regs *regs)
 {
@@ -215,7 +257,10 @@ static int single_step_handler(unsigned long addr, 
unsigned int esr,
 */
user_rewind_single_step(current);
} else {
-   /* TODO: route to KGDB */
+   /* call registered single step handlers */
+   if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
+   return 0;
+
pr_warning("Unexpected kernel single-step exception at EL1\n");
/*
 * Re-enable stepping since we know that we will be
@@ -227,11 +272,50 @@ static int single_step_handler(unsigned long addr, 
unsigned int esr,
return 0;
 }
 
+
+static LIST_HEAD(break_hook);
+DEFINE_RWLOCK(break_hook_lock);
+
+void register_break_hook(struct break_hook *hook)
+{
+   write_lock(&break_hook_lock);
+   list_add(&hook->node, &break_hook);
+   write_unlock(&break_hook_lock);
+}
+
+void unregister_break_hook(struct break_hook *hook)
+{
+   write_lock(&break_hook_lock);
+   list_del(&hook->node);
+   write_unlock(&break_hook_lock);
+}
+
+static int call_break_hook(struct pt_regs *regs, unsigned int esr)
+{
+   struct break_hook *hook;
+   int (*fn)(struct pt_regs *regs, unsigned int esr) = NULL;
+
+   read_lock(&break_hook_lock);
+   list_for_each_entry(hook, &break_hook, node)
+   if ((esr & hook->esr_mask) == hook->esr_val)
+   fn = hook->fn;
+   read_unlock(&break_hook_lock);
+
+   return fn ? fn(regs, esr) : DBG_HOOK_ERROR;
+}
+
 static int brk_handler(unsigned long addr, unsigned int esr,
   struct pt_regs *regs)
 {
sigi

[PATCH RFC 3/6] arm64: Kprobes instruction simulation support

[Sandeepa Prabhu]

Add support for AArch64 instruction simulation in kprobes.

Kprobes need simulation of instructions that cannot be stepped
right-away from different memory location. i.e. those instructions
that uses PC-relative addressing. In simulation, the behaviour
of the instruction is implemented using copy of pt_regs.

Following instruction catagories are simulated:
 - All branching instructions(conditional, register, and immediate)
 - Literal access instructions(load-literal, adr/adrp)

conditional execution are limited to branching instructions in
ARM v8. If conditions at PSTATE does not match the condition fields
of opcode, the instruction is effectively NOP. Kprobes consider
this case as 'miss'.

Signed-off-by: Sandeepa Prabhu 
---
 arch/arm64/kernel/Makefile|   3 +-
 arch/arm64/kernel/condn-helpers.c | 120 +
 arch/arm64/kernel/kprobes-arm64.c | 120 +++--
 arch/arm64/kernel/kprobes-arm64.h |   2 +
 arch/arm64/kernel/kprobes.c   |  31 ++-
 arch/arm64/kernel/simulate-insn.c | 184 ++
 arch/arm64/kernel/simulate-insn.h |  33 +++
 7 files changed, 480 insertions(+), 13 deletions(-)
 create mode 100644 arch/arm64/kernel/condn-helpers.c
 create mode 100644 arch/arm64/kernel/simulate-insn.c
 create mode 100644 arch/arm64/kernel/simulate-insn.h

diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 12ef8d2..b000a51 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -19,7 +19,8 @@ arm64-obj-$(CONFIG_HW_PERF_EVENTS)+= perf_event.o
 arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT)+= hw_breakpoint.o
 arm64-obj-$(CONFIG_EARLY_PRINTK)   += early_printk.o
 arm64-obj-$(CONFIG_JUMP_LABEL) += jump_label.o
-arm64-obj-$(CONFIG_KPROBES)+= kprobes.o kprobes-arm64.o
+arm64-obj-$(CONFIG_KPROBES)+= kprobes.o kprobes-arm64.o
\
+  simulate-insn.o condn-helpers.o
 
 obj-y  += $(arm64-obj-y) vdso/
 obj-m  += $(arm64-obj-m)
diff --git a/arch/arm64/kernel/condn-helpers.c 
b/arch/arm64/kernel/condn-helpers.c
new file mode 100644
index 000..7abc7ec
--- /dev/null
+++ b/arch/arm64/kernel/condn-helpers.c
@@ -0,0 +1,120 @@
+/*
+ * arch/arm64/kernel/condn-helpers.c
+ *
+ * Copyright (C) 2013 Linaro Limited
+ *
+ * Copied from: arch/arm/kernel/kprobes-common.c
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * Description:
+ *
+ *  AArch64 and AArch32 shares same conditional(CNZV) flags encoding.
+ *  This file implements conditional check helpers compatible with
+ *  both AArch64 and AArch32 modes. Uprobes on v8 can handle both 32-bit
+ *  & 64-bit user-space instructions, so we abstract the common functions
+ *  in this file. While AArch64 and AArch32 specific instruction handling
+ *  are implemented in seperate files, this file contains common bits.
+ */
+#include 
+#include 
+#include 
+
+static unsigned long __kprobes __check_eq(unsigned long pstate)
+{
+   return pstate & PSR_Z_BIT;
+}
+
+static unsigned long __kprobes __check_ne(unsigned long pstate)
+{
+   return (~pstate) & PSR_Z_BIT;
+}
+
+static unsigned long __kprobes __check_cs(unsigned long pstate)
+{
+   return pstate & PSR_C_BIT;
+}
+
+static unsigned long __kprobes __check_cc(unsigned long pstate)
+{
+   return (~pstate) & PSR_C_BIT;
+}
+
+static unsigned long __kprobes __check_mi(unsigned long pstate)
+{
+   return pstate & PSR_N_BIT;
+}
+
+static unsigned long __kprobes __check_pl(unsigned long pstate)
+{
+   return (~pstate) & PSR_N_BIT;
+}
+
+static unsigned long __kprobes __check_vs(unsigned long pstate)
+{
+   return pstate & PSR_V_BIT;
+}
+
+static unsigned long __kprobes __check_vc(unsigned long pstate)
+{
+   return (~pstate) & PSR_V_BIT;
+}
+
+static unsigned long __kprobes __check_hi(unsigned long pstate)
+{
+   pstate &= ~(pstate >> 1);   /* PSR_C_BIT &= ~PSR_Z_BIT */
+   return pstate & PSR_C_BIT;
+}
+
+static unsigned long __kprobes __check_ls(unsigned long pstate)
+{
+   pstate &= ~(pstate >> 1);   /* PSR_C_BIT &= ~PSR_Z_BIT */
+   return (~pstate) & PSR_C_BIT;
+}
+
+static unsigned long __kprobes __check_ge(unsigned long pstate)
+{
+   pstate ^= (pstate << 3);/* PSR_N_BIT ^= PSR_V_BIT */
+   return (~pstate) & PSR_N_BIT;
+}
+
+static unsigned long __kprobes __check_lt(unsigned long pstate)
+{
+   pstate ^= (pstat

[PATCH RFC 6/6] kprobes: Add cases for arm and arm64 in sample module

[Sandeepa Prabhu]

Add info prints in sample kprobe handlers for ARM and ARM64
architecture.

Signed-off-by: Sandeepa Prabhu 
---
 samples/kprobes/kprobe_example.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/samples/kprobes/kprobe_example.c b/samples/kprobes/kprobe_example.c
index 366db1a..0521246 100644
--- a/samples/kprobes/kprobe_example.c
+++ b/samples/kprobes/kprobe_example.c
@@ -42,6 +42,14 @@ static int handler_pre(struct kprobe *p, struct pt_regs 
*regs)
" ex1 = 0x%lx\n",
p->addr, regs->pc, regs->ex1);
 #endif
+#ifdef CONFIG_ARM
+   printk(KERN_INFO "pre_handler: p->addr = 0x%p, pc = 0x%lx\n",
+   p->addr, regs->ARM_pc);
+#endif
+#ifdef CONFIG_ARM64
+   printk(KERN_INFO "pre_handler: p->addr = 0x%p, pc = 0x%lx\n",
+   p->addr, (long)regs->pc);
+#endif
 
/* A dump_stack() here will give a stack backtrace */
return 0;
@@ -67,6 +75,14 @@ static void handler_post(struct kprobe *p, struct pt_regs 
*regs,
printk(KERN_INFO "post_handler: p->addr = 0x%p, ex1 = 0x%lx\n",
p->addr, regs->ex1);
 #endif
+#ifdef CONFIG_ARM
+   printk(KERN_INFO "post_handler: p->addr = 0x%p, pc = 0x%lx\n",
+   p->addr, regs->ARM_pc);
+#endif
+#ifdef CONFIG_ARM64
+   printk(KERN_INFO "post_handler: p->addr = 0x%p, pc = 0x%lx\n",
+   p->addr, (long)regs->pc);
+#endif
 }
 
 /*
-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH RFC 4/6] arm64: Add kernel return probes support(kretprobes)

[Sandeepa Prabhu]

AArch64 ISA does not instructions to pop PC register value
from stack(like ARM v7 has ldmia {...,pc}) without using
one of the general purpose registers. This means return probes
cannot return to the actual return address directly without
modifying register context, and without trapping into debug exception.

So like many other architectures, we prepare a global routine
with NOPs, which serve as trampoline to hack away the
function return address, by placing an extra kprobe on the
trampoline entry.

The pre-handler of this special trampoline' kprobe execute return
probe handler functions and restore original return address in ELR_EL1,
this way, saved pt_regs still hold the original register context to be
carried back to the probed kernel function.

Signed-off-by: Sandeepa Prabhu 
---
 arch/arm64/Kconfig   |   1 +
 arch/arm64/include/asm/kprobes.h |   1 +
 arch/arm64/include/asm/ptrace.h  |   5 ++
 arch/arm64/kernel/kprobes.c  | 125 ++-
 4 files changed, 129 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 2e89059..73eff55 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -28,6 +28,7 @@ config ARM64
select HAVE_MEMBLOCK
select HAVE_PERF_EVENTS
select HAVE_KPROBES if !XIP_KERNEL
+   select HAVE_KRETPROBES if (HAVE_KPROBES)
select IRQ_DOMAIN
select MODULES_USE_ELF_RELA
select NO_BOOTMEM
diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
index 9b491d0..eaca849 100644
--- a/arch/arm64/include/asm/kprobes.h
+++ b/arch/arm64/include/asm/kprobes.h
@@ -55,5 +55,6 @@ void arch_remove_kprobe(struct kprobe *);
 int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
 int kprobe_exceptions_notify(struct notifier_block *self,
 unsigned long val, void *data);
+void kretprobe_trampoline(void);
 
 #endif /* _ARM_KPROBES_H */
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 89f1727..58b2589 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -166,6 +166,11 @@ static inline int valid_user_regs(struct user_pt_regs 
*regs)
 #define instruction_pointer(regs)  (regs)->pc
 #define stack_pointer(regs)((regs)->sp)
 
+static inline long regs_return_value(struct pt_regs *regs)
+{
+   return regs->regs[0];
+}
+
 #ifdef CONFIG_SMP
 extern unsigned long profile_pc(struct pt_regs *regs);
 #else
diff --git a/arch/arm64/kernel/kprobes.c b/arch/arm64/kernel/kprobes.c
index 1fa8690..0a6f31f 100644
--- a/arch/arm64/kernel/kprobes.c
+++ b/arch/arm64/kernel/kprobes.c
@@ -219,9 +219,16 @@ static void __kprobes setup_singlestep(struct kprobe *p,
 
/*
 * Needs restoring of return address after stepping xol.
+* If this happens to be a return probe, the exception
+* return address would have been hacked by the pre_handler
+* to point to trampoline, so we shall restore trampoline
+* address after stepping. Other cases, it is just next pc.
 */
-   p->ainsn.restore.addr = instruction_pointer(regs) +
-   sizeof(kprobe_opcode_t);
+   if ((long)p->addr == instruction_pointer(regs))
+   p->ainsn.restore.addr = instruction_pointer(regs) +
+   sizeof(kprobe_opcode_t); /* next pc */
+   else/* hacked ret addr, could be kretprobe */
+   p->ainsn.restore.addr = regs->pc;
 
p->ainsn.restore.type = RESTORE_PC;
 
@@ -542,6 +549,117 @@ int __kprobes longjmp_break_handler(struct kprobe *p, 
struct pt_regs *regs)
return 0;
 }
 
+/*
+ * Kretprobes: kernel return probes handling
+ *
+ * AArch64 mode does not support popping the PC value from the
+ * stack like on ARM 32-bit (ldmia {..,pc}), so atleast one
+ * register need to be used to achieve branching/return.
+ * It means return probes cannot return back to the original
+ * return address directly without modifying the register context.
+ *
+ * So like other architectures, we prepare a global routine
+ * with NOPs, which serve as trampoline address that hack away the
+ * function return, with the exact register context.
+ * Placing a kprobe on trampoline routine entry will trap again to
+ * execute return probe handlers and restore original return address
+ * in ELR_EL1, this way saved pt_regs still hold the original
+ * register values to be carried back to the caller.
+ */
+static void __used kretprobe_trampoline_holder(void)
+{
+   asm volatile (".global kretprobe_trampoline\n"
+   "kretprobe_trampoline:\n"
+   "NOP\n\t"
+   "NOP\n\t");
+}
+
+static int __kprobes
+trampoline_probe_handler(struct kprobe *

[PATCH RFC 2/6] arm64: Kprobes with single stepping support

[Sandeepa Prabhu]

Add support for basic kernel probes(kprobes), jump probes (jprobes)
for ARM64.

Kprobes will utilize software breakpoint and single step debug
exceptions supported on ARM v8.

software breakpoint is placed at the probe address to trap the
kernel execution into kprobe handler.

ARM v8 support single stepping to be enabled while exception return
(ERET) with next PC in exception return address (ELR_EL1).
kprobe handler prepares a executable memory slot for out-of-line
execution with the copy of the original instruction under probe, and
enable single stepping from the instruction slot. With this scheme,
the instruction is executed with the exact same register context
'except PC' that points to instruction slot.

Debug mask(PSTATE.D) is enabled while calling user pre & post handlers
since it can hit breakpoint again(like kprobe placed on printk, with
user handler invoking printk again). This allow kprobe re-entry and
any further re-entry is prevented by not calling handlers (counted as
missed kprobe)

Single stepping from slot has drawback on PC-relative accesses
like branching and symbolic literals access as offset from new PC
may not be ensured to fit in immediate value of opcode,
(usually +/-1MB range). So these instructions needs simulation, so
are not allowed to place probe on those instructions.

Instructions generating exceptions or cpu mode change are rejected,
and not allowed to insert probe for such instructions.

Instructions using Exclusive Monitor are rejected right now.

System instructions are mostly enabled for stepping, except MSR
immediate that update "daif" flags in PSTATE, which are not safe
for probing(rejected)

Signed-off-by: Sandeepa Prabhu 
---
 arch/arm64/Kconfig|   1 +
 arch/arm64/include/asm/kprobes.h  |  59 +
 arch/arm64/include/asm/probes.h   |  50 
 arch/arm64/include/asm/ptrace.h   |   1 +
 arch/arm64/kernel/Makefile|   1 +
 arch/arm64/kernel/kprobes-arm64.c | 211 +++
 arch/arm64/kernel/kprobes-arm64.h |  28 ++
 arch/arm64/kernel/kprobes.c   | 538 ++
 arch/arm64/kernel/kprobes.h   |  30 +++
 arch/arm64/kernel/probes-decode.h | 110 
 arch/arm64/kernel/vmlinux.lds.S   |   1 +
 11 files changed, 1030 insertions(+)
 create mode 100644 arch/arm64/include/asm/kprobes.h
 create mode 100644 arch/arm64/include/asm/probes.h
 create mode 100644 arch/arm64/kernel/kprobes-arm64.c
 create mode 100644 arch/arm64/kernel/kprobes-arm64.h
 create mode 100644 arch/arm64/kernel/kprobes.c
 create mode 100644 arch/arm64/kernel/kprobes.h
 create mode 100644 arch/arm64/kernel/probes-decode.h

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index da388e4..2e89059 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -27,6 +27,7 @@ config ARM64
select HAVE_HW_BREAKPOINT if PERF_EVENTS
select HAVE_MEMBLOCK
select HAVE_PERF_EVENTS
+   select HAVE_KPROBES if !XIP_KERNEL
select IRQ_DOMAIN
select MODULES_USE_ELF_RELA
select NO_BOOTMEM
diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
new file mode 100644
index 000..9b491d0
--- /dev/null
+++ b/arch/arm64/include/asm/kprobes.h
@@ -0,0 +1,59 @@
+/*
+ * arch/arm64/include/asm/kprobes.h
+ *
+ * Copyright (C) 2013 Linaro Limited
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef _ARM_KPROBES_H
+#define _ARM_KPROBES_H
+
+#include 
+#include 
+#include 
+
+#define __ARCH_WANT_KPROBES_INSN_SLOT
+#define MAX_INSN_SIZE  2
+#define MAX_STACK_SIZE 128
+
+#define flush_insn_slot(p) do { } while (0)
+#define kretprobe_blacklist_size   0
+
+#include 
+
+struct prev_kprobe {
+   struct kprobe *kp;
+   unsigned int status;
+};
+
+/* Single step context for kprobe */
+struct kprobe_step_ctx {
+#define KPROBES_STEP_NONE  0x0
+#define KPROBES_STEP_PENDING   0x1
+   unsigned long ss_status;
+   unsigned long match_addr;
+};
+
+/* per-cpu kprobe control block */
+struct kprobe_ctlblk {
+   unsigned int kprobe_status;
+   struct prev_kprobe prev_kprobe;
+   struct kprobe_step_ctx ss_ctx;
+   struct pt_regs jprobe_saved_regs;
+   char jprobes_stack[MAX_STACK_SIZE];
+};
+
+void arch_remove_kprobe(struct kprobe *);
+int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
+int kprobe_exceptions_notify(struct notifier_block *self,
+unsigned long val, void *data);
+
+#endif /* _ARM_KPROBES_H */
diff --git a/arch/arm64/include/asm/probes.h 

[PATCH RFC 5/6] arm64: Enable kprobes support for arm64 platform

[Sandeepa Prabhu]

This patch enables kprobes support in arm64 common defconfig
file.

Signed-off-by: Sandeepa Prabhu 
---
 arch/arm64/configs/defconfig | 20 +---
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 31c81e9..105f37c 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1,12 +1,11 @@
-CONFIG_EXPERIMENTAL=y
 # CONFIG_LOCALVERSION_AUTO is not set
 # CONFIG_SWAP is not set
 CONFIG_SYSVIPC=y
 CONFIG_POSIX_MQUEUE=y
-CONFIG_BSD_PROCESS_ACCT=y
-CONFIG_BSD_PROCESS_ACCT_V3=y
 CONFIG_NO_HZ=y
 CONFIG_HIGH_RES_TIMERS=y
+CONFIG_BSD_PROCESS_ACCT=y
+CONFIG_BSD_PROCESS_ACCT_V3=y
 CONFIG_IKCONFIG=y
 CONFIG_IKCONFIG_PROC=y
 CONFIG_LOG_BUF_SHIFT=14
@@ -19,6 +18,7 @@ CONFIG_BLK_DEV_INITRD=y
 CONFIG_KALLSYMS_ALL=y
 # CONFIG_COMPAT_BRK is not set
 CONFIG_PROFILING=y
+CONFIG_KPROBES=y
 CONFIG_MODULES=y
 CONFIG_MODULE_UNLOAD=y
 # CONFIG_BLK_DEV_BSG is not set
@@ -42,13 +42,12 @@ CONFIG_IP_PNP_BOOTP=y
 # CONFIG_WIRELESS is not set
 CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug"
 CONFIG_DEVTMPFS=y
-CONFIG_BLK_DEV=y
+CONFIG_VIRTIO_BLK=y
 CONFIG_SCSI=y
 # CONFIG_SCSI_PROC_FS is not set
 CONFIG_BLK_DEV_SD=y
 # CONFIG_SCSI_LOWLEVEL is not set
 CONFIG_NETDEVICES=y
-CONFIG_MII=y
 CONFIG_SMC91X=y
 # CONFIG_WLAN is not set
 CONFIG_INPUT_EVDEV=y
@@ -57,9 +56,9 @@ CONFIG_INPUT_EVDEV=y
 CONFIG_LEGACY_PTY_COUNT=16
 CONFIG_SERIAL_8250=y
 CONFIG_SERIAL_8250_CONSOLE=y
-CONFIG_SERIAL_OF_PLATFORM=y
 CONFIG_SERIAL_AMBA_PL011=y
 CONFIG_SERIAL_AMBA_PL011_CONSOLE=y
+CONFIG_SERIAL_OF_PLATFORM=y
 # CONFIG_HW_RANDOM is not set
 # CONFIG_HWMON is not set
 CONFIG_FB=y
@@ -69,12 +68,13 @@ CONFIG_LOGO=y
 # CONFIG_LOGO_LINUX_MONO is not set
 # CONFIG_LOGO_LINUX_VGA16 is not set
 # CONFIG_USB_SUPPORT is not set
+CONFIG_VIRTIO_MMIO=y
 # CONFIG_IOMMU_SUPPORT is not set
 CONFIG_EXT2_FS=y
 CONFIG_EXT3_FS=y
-CONFIG_EXT4_FS=y
 # CONFIG_EXT3_DEFAULTS_TO_ORDERED is not set
 # CONFIG_EXT3_FS_XATTR is not set
+CONFIG_EXT4_FS=y
 CONFIG_FUSE_FS=y
 CONFIG_CUSE=y
 CONFIG_VFAT_FS=y
@@ -84,12 +84,10 @@ CONFIG_NFS_FS=y
 CONFIG_ROOT_NFS=y
 CONFIG_NLS_CODEPAGE_437=y
 CONFIG_NLS_ISO8859_1=y
-CONFIG_MAGIC_SYSRQ=y
+CONFIG_DEBUG_INFO=y
 CONFIG_DEBUG_FS=y
+CONFIG_MAGIC_SYSRQ=y
 CONFIG_DEBUG_KERNEL=y
 # CONFIG_SCHED_DEBUG is not set
-CONFIG_DEBUG_INFO=y
 # CONFIG_FTRACE is not set
 CONFIG_ATOMIC64_SELFTEST=y
-CONFIG_VIRTIO_MMIO=y
-CONFIG_VIRTIO_BLK=y
-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH RFC v2 0/6] ARM64: Add kernel probes(Kprobes) support

[Sandeepa Prabhu]

This patchset adds support for kernel probes(kprobes), jump probes(jprobes)
and return probes(kretprobes) support for ARM64.

Kprobes mechanism make use of software breakpoint and single stepping
support available in ARM v8 kernel.

This patchset make use of (and dependant upon) dynamic kernel patching 
feature published in patch series: https://lkml.org/lkml/2013/10/15/891

changes: v1 -> v2
 1. Implemented review comments on v1
 2. Debug montior hooks implementation is changed to use rwlocks
instead of rcu and spinlock.
 3. Enabled recursing in kprobes handler for kprobes re-enter support.
 4. Re-split the patchset to seperate single-stepping, simulation and 
kretprobes features.
 5. instruction simulation is made independent of 'struct kprobes'
 6. Added 'Linaro Copyright' statements in new added files.
 7. Used arm64 instead of aarch64 in file names and comments.

Tested on ARM v8 fast model with sample modules from: samples/kprobes/

Sandeepa Prabhu (6):
  arm64: support single-step and breakpoint handler hooks
  arm64: Kprobes with single stepping support
  arm64: Kprobes instruction simulation support
  arm64: Add kernel return probes support(kretprobes)
  arm64: Enable kprobes support for arm64 platform
  kprobes: Add cases for arm and arm64 in sample module

 arch/arm64/Kconfig  |   2 +
 arch/arm64/configs/defconfig|  20 +-
 arch/arm64/include/asm/debug-monitors.h |  21 +
 arch/arm64/include/asm/kprobes.h|  60 +++
 arch/arm64/include/asm/probes.h |  50 +++
 arch/arm64/include/asm/ptrace.h |   6 +
 arch/arm64/kernel/Makefile  |   2 +
 arch/arm64/kernel/condn-helpers.c   | 120 ++
 arch/arm64/kernel/debug-monitors.c  |  86 +++-
 arch/arm64/kernel/entry.S   |   2 +
 arch/arm64/kernel/kprobes-arm64.c   | 313 +++
 arch/arm64/kernel/kprobes-arm64.h   |  30 ++
 arch/arm64/kernel/kprobes.c | 682 
 arch/arm64/kernel/kprobes.h |  30 ++
 arch/arm64/kernel/probes-decode.h   | 110 ++
 arch/arm64/kernel/simulate-insn.c   | 184 +
 arch/arm64/kernel/simulate-insn.h   |  33 ++
 arch/arm64/kernel/vmlinux.lds.S |   1 +
 samples/kprobes/kprobe_example.c|  16 +
 19 files changed, 1756 insertions(+), 12 deletions(-)
 create mode 100644 arch/arm64/include/asm/kprobes.h
 create mode 100644 arch/arm64/include/asm/probes.h
 create mode 100644 arch/arm64/kernel/condn-helpers.c
 create mode 100644 arch/arm64/kernel/kprobes-arm64.c
 create mode 100644 arch/arm64/kernel/kprobes-arm64.h
 create mode 100644 arch/arm64/kernel/kprobes.c
 create mode 100644 arch/arm64/kernel/kprobes.h
 create mode 100644 arch/arm64/kernel/probes-decode.h
 create mode 100644 arch/arm64/kernel/simulate-insn.c
 create mode 100644 arch/arm64/kernel/simulate-insn.h

-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 2/7] arm64: introduce interfaces to hotpatch kernel and module code

[Sandeepa Prabhu]

On 27 September 2013 21:11, Jiang Liu  wrote:
> On 09/25/2013 10:35 PM, Sandeepa Prabhu wrote:
>> On 25 September 2013 16:14, Jiang Liu  wrote:
>>> From: Jiang Liu 
>>>
>>> Introduce aarch64_insn_patch_text() and __aarch64_insn_patch_text()
>>> to patch kernel and module code.
>>>
>>> Function aarch64_insn_patch_text() is a heavy version which may use
>>> stop_machine() to serialize all online CPUs, and function
>>> __aarch64_insn_patch_text() is light version without explicitly
>>> serialization.
>> Hi Jiang,
>>
>> I have written kprobes support for aarch64, and need both the
>> functionality (lightweight and stop_machine() versions).
>> I would like to rebase these API in kprobes, however slight changes
>> would require in case of stop_machine version, which I explained
>> below.
>> [Though kprobes cannot share Instruction encode support of jump labels
>> as, decoding & simulation quite different for kprobes/uprobes and
>> based around single stepping]
>>
>>>
>>> Signed-off-by: Jiang Liu 
>>> Cc: Jiang Liu 
>>> ---
>>>  arch/arm64/include/asm/insn.h |  2 ++
>>>  arch/arm64/kernel/insn.c  | 64 
>>> +++
>>>  2 files changed, 66 insertions(+)
>>>
>>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>>> index e7d1bc8..0ea7193 100644
>>> --- a/arch/arm64/include/asm/insn.h
>>> +++ b/arch/arm64/include/asm/insn.h
>>> @@ -49,5 +49,7 @@ __AARCH64_INSN_FUNCS(nop, 0x, 0xD503201F)
>>>  enum aarch64_insn_class aarch64_get_insn_class(u32 insn);
>>>
>>>  bool aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn);
>>> +int aarch64_insn_patch_text(void *addr, u32 *insns, int cnt);
>>> +int __aarch64_insn_patch_text(void *addr, u32 *insns, int cnt);
>>>
>>>  #endif /* _ASM_ARM64_INSN_H */
>>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
>>> index 8541c3a..50facfc 100644
>>> --- a/arch/arm64/kernel/insn.c
>>> +++ b/arch/arm64/kernel/insn.c
>>> @@ -15,6 +15,8 @@
>>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>   */
>>>  #include 
>>> +#include 
>>> +#include 
>>>  #include 
>>>
>>>  static int aarch64_insn_cls[] = {
>>> @@ -69,3 +71,65 @@ bool __kprobes aarch64_insn_hotpatch_safe(u32 old_insn, 
>>> u32 new_insn)
>>> return __aarch64_insn_hotpatch_safe(old_insn) &&
>>>__aarch64_insn_hotpatch_safe(new_insn);
>>>  }
>>> +
>>> +struct aarch64_insn_patch {
>>> +   void*text_addr;
>>> +   u32 *new_insns;
>>> +   int insn_cnt;
>>> +};
>>> +
>>> +int __kprobes __aarch64_insn_patch_text(void *addr, u32 *insns, int cnt)
>>> +{
>>> +   int i;
>>> +   u32 *tp = addr;
>>> +
>>> +   /* instructions must be word aligned */
>>> +   if (cnt <= 0 || ((uintptr_t)addr & 0x3))
>>> +   return -EINVAL;
>>> +
>>> +   for (i = 0; i < cnt; i++)
>>> +   tp[i] = insns[i];
>>> +
>>> +   flush_icache_range((uintptr_t)tp, (uintptr_t)tp + cnt * 
>>> sizeof(u32));
>>> +
>>> +   return 0;
>>> +}
>> Looks fine, but do you need to check for CPU big endian mode here? (I
>> think swab32() needed if EL1 is in big-endian mode)
> Hi Sandeepa,
> Thanks for reminder, we do need to take care of data access endian
> issue here, will fix it in next version.
>
>>
>>> +
>>> +static int __kprobes aarch64_insn_patch_text_cb(void *arg)
>>> +{
>>> +   struct aarch64_insn_patch *pp = arg;
>>> +
>>> +   return __aarch64_insn_patch_text(pp->text_addr, pp->new_insns,
>>> +pp->insn_cnt);
>>> +}
>>> +
>>> +int __kprobes aarch64_insn_patch_text(void *addr, u32 *insns, int cnt)
>>> +{
>>> +   int ret;
>>> +   bool safe = false;
>>> +
>>> +   /* instructions must be word aligned */
>>> +   if (cnt <= 0 || ((uintptr_t)addr & 0x3))
>>> +   return -EINVAL;
>>> +
>>> +   if (cnt == 1)
>>> +   safe = aarch64_insn_hotpatch_safe(*(u32 *)addr, insns[0]);
>>> +
>>> +   if (saf

Re: [PATCH v1 2/7] arm64: introduce interfaces to hotpatch kernel and module code

[Sandeepa Prabhu]

On 25 September 2013 21:24, Jiang Liu  wrote:
> Hi Sandeepa,
> Great to know that you are working on kprobe for ARM64.
> I have done some investigation, found it's an huge work for me
> then gave up:(
> I will try refine the implementation based on your requirement.
> Thanks!
Hi Jiang,
Thanks. please CC me when you post next version of this patch, then I
can rebase my code and verify it.

Thanks,
Sandeepa

>
> On 09/25/2013 10:35 PM, Sandeepa Prabhu wrote:
>> On 25 September 2013 16:14, Jiang Liu  wrote:
>>> From: Jiang Liu 
>>>
>>> Introduce aarch64_insn_patch_text() and __aarch64_insn_patch_text()
>>> to patch kernel and module code.
>>>
>>> Function aarch64_insn_patch_text() is a heavy version which may use
>>> stop_machine() to serialize all online CPUs, and function
>>> __aarch64_insn_patch_text() is light version without explicitly
>>> serialization.
>> Hi Jiang,
>>
>> I have written kprobes support for aarch64, and need both the
>> functionality (lightweight and stop_machine() versions).
>> I would like to rebase these API in kprobes, however slight changes
>> would require in case of stop_machine version, which I explained
>> below.
>> [Though kprobes cannot share Instruction encode support of jump labels
>> as, decoding & simulation quite different for kprobes/uprobes and
>> based around single stepping]
>>
>>>
>>> Signed-off-by: Jiang Liu 
>>> Cc: Jiang Liu 
>>> ---
>>>  arch/arm64/include/asm/insn.h |  2 ++
>>>  arch/arm64/kernel/insn.c  | 64 
>>> +++
>>>  2 files changed, 66 insertions(+)
>>>
>>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>>> index e7d1bc8..0ea7193 100644
>>> --- a/arch/arm64/include/asm/insn.h
>>> +++ b/arch/arm64/include/asm/insn.h
>>> @@ -49,5 +49,7 @@ __AARCH64_INSN_FUNCS(nop, 0x, 0xD503201F)
>>>  enum aarch64_insn_class aarch64_get_insn_class(u32 insn);
>>>
>>>  bool aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn);
>>> +int aarch64_insn_patch_text(void *addr, u32 *insns, int cnt);
>>> +int __aarch64_insn_patch_text(void *addr, u32 *insns, int cnt);
>>>
>>>  #endif /* _ASM_ARM64_INSN_H */
>>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
>>> index 8541c3a..50facfc 100644
>>> --- a/arch/arm64/kernel/insn.c
>>> +++ b/arch/arm64/kernel/insn.c
>>> @@ -15,6 +15,8 @@
>>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>   */
>>>  #include 
>>> +#include 
>>> +#include 
>>>  #include 
>>>
>>>  static int aarch64_insn_cls[] = {
>>> @@ -69,3 +71,65 @@ bool __kprobes aarch64_insn_hotpatch_safe(u32 old_insn, 
>>> u32 new_insn)
>>> return __aarch64_insn_hotpatch_safe(old_insn) &&
>>>__aarch64_insn_hotpatch_safe(new_insn);
>>>  }
>>> +
>>> +struct aarch64_insn_patch {
>>> +   void*text_addr;
>>> +   u32 *new_insns;
>>> +   int insn_cnt;
>>> +};
>>> +
>>> +int __kprobes __aarch64_insn_patch_text(void *addr, u32 *insns, int cnt)
>>> +{
>>> +   int i;
>>> +   u32 *tp = addr;
>>> +
>>> +   /* instructions must be word aligned */
>>> +   if (cnt <= 0 || ((uintptr_t)addr & 0x3))
>>> +   return -EINVAL;
>>> +
>>> +   for (i = 0; i < cnt; i++)
>>> +   tp[i] = insns[i];
>>> +
>>> +   flush_icache_range((uintptr_t)tp, (uintptr_t)tp + cnt * 
>>> sizeof(u32));
>>> +
>>> +   return 0;
>>> +}
>> Looks fine, but do you need to check for CPU big endian mode here? (I
>> think swab32() needed if EL1 is in big-endian mode)
>>
>>> +
>>> +static int __kprobes aarch64_insn_patch_text_cb(void *arg)
>>> +{
>>> +   struct aarch64_insn_patch *pp = arg;
>>> +
>>> +   return __aarch64_insn_patch_text(pp->text_addr, pp->new_insns,
>>> +pp->insn_cnt);
>>> +}
>>> +
>>> +int __kprobes aarch64_insn_patch_text(void *addr, u32 *insns, int cnt)
>>> +{
>>> +   int ret;
>>> +   bool safe = false;
>>> +
>>> +   /* instructions must be word aligned */
>>> +   if (cnt &l

Re: [PATCH v1 2/7] arm64: introduce interfaces to hotpatch kernel and module code

[Sandeepa Prabhu]

On 25 September 2013 20:46, Steven Rostedt  wrote:
> On Wed, 25 Sep 2013 20:12:17 +0530
> Sandeepa Prabhu  wrote:
>
>
>> > On aarch64, are instructions always word aligned? If not, it should be
>> > safe for stop machine to modify non word aligned instructions, but this
>> > patch looks like it doesn't allow stop_machine() to do so.
>> Steve,
>>
>> Yes, aarch64 instructions must be word-aligned, else instruction fetch
>> would generate Misaligned PC fault.
>>
>
> Thanks for clarifying, as IIUC, there's ARM architectures that allow
> for 2 and 4 byte instructions.
Yes, ARM 32-bit mode would support both 32-bit and 16-bit alignment
based on ARM or Thumb mode, whereas
AArch64 (in arch/arm64/) is always 32-bit instructions and PC need to
be aligned to 32-bit address.

Thanks,
Sandeepa
>
> -- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 2/7] arm64: introduce interfaces to hotpatch kernel and module code

[Sandeepa Prabhu]

On 25 September 2013 20:05, Steven Rostedt  wrote:
> On Wed, 25 Sep 2013 18:44:22 +0800
> Jiang Liu  wrote:
>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
>> index 8541c3a..50facfc 100644
>> --- a/arch/arm64/kernel/insn.c
>> +++ b/arch/arm64/kernel/insn.c
>> @@ -15,6 +15,8 @@
>>   * along with this program.  If not, see .
>>   */
>>  #include 
>> +#include 
>> +#include 
>>  #include 
>>
>>  static int aarch64_insn_cls[] = {
>> @@ -69,3 +71,65 @@ bool __kprobes aarch64_insn_hotpatch_safe(u32 old_insn, 
>> u32 new_insn)
>>   return __aarch64_insn_hotpatch_safe(old_insn) &&
>>  __aarch64_insn_hotpatch_safe(new_insn);
>>  }
>> +
>> +struct aarch64_insn_patch {
>> + void*text_addr;
>> + u32 *new_insns;
>> + int insn_cnt;
>> +};
>> +
>> +int __kprobes __aarch64_insn_patch_text(void *addr, u32 *insns, int cnt)
>> +{
>> + int i;
>> + u32 *tp = addr;
>> +
>> + /* instructions must be word aligned */
>> + if (cnt <= 0 || ((uintptr_t)addr & 0x3))
>> + return -EINVAL;
>
> On aarch64, are instructions always word aligned? If not, it should be
> safe for stop machine to modify non word aligned instructions, but this
> patch looks like it doesn't allow stop_machine() to do so.
Steve,

Yes, aarch64 instructions must be word-aligned, else instruction fetch
would generate Misaligned PC fault.

Thanks,
Sandeepa
>
> -- Steve
>
>> +
>> + for (i = 0; i < cnt; i++)
>> + tp[i] = insns[i];
>> +
>> + flush_icache_range((uintptr_t)tp, (uintptr_t)tp + cnt * sizeof(u32));
>> +
>> + return 0;
>> +}
>> +
>> +static int __kprobes aarch64_insn_patch_text_cb(void *arg)
>> +{
>> + struct aarch64_insn_patch *pp = arg;
>> +
>> + return __aarch64_insn_patch_text(pp->text_addr, pp->new_insns,
>> +  pp->insn_cnt);
>> +}
>> +
>> +int __kprobes aarch64_insn_patch_text(void *addr, u32 *insns, int cnt)
>> +{
>> + int ret;
>> + bool safe = false;
>> +
>> + /* instructions must be word aligned */
>> + if (cnt <= 0 || ((uintptr_t)addr & 0x3))
>> + return -EINVAL;
>> +
>> + if (cnt == 1)
>> + safe = aarch64_insn_hotpatch_safe(*(u32 *)addr, insns[0]);
>> +
>> + if (safe) {
>> + ret = __aarch64_insn_patch_text(addr, insns, cnt);
>> + } else {
>> + struct aarch64_insn_patch patch = {
>> + .text_addr = addr,
>> + .new_insns = insns,
>> + .insn_cnt = cnt,
>> + };
>> +
>> + /*
>> +  * Execute __aarch64_insn_patch_text() on every online CPU,
>> +  * which ensure serialization among all online CPUs.
>> +  */
>> + ret = stop_machine(aarch64_insn_patch_text_cb, &patch, NULL);
>> + }
>> +
>> + return ret;
>> +}
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 2/7] arm64: introduce interfaces to hotpatch kernel and module code

[Sandeepa Prabhu]

On 25 September 2013 16:14, Jiang Liu  wrote:
> From: Jiang Liu 
>
> Introduce aarch64_insn_patch_text() and __aarch64_insn_patch_text()
> to patch kernel and module code.
>
> Function aarch64_insn_patch_text() is a heavy version which may use
> stop_machine() to serialize all online CPUs, and function
> __aarch64_insn_patch_text() is light version without explicitly
> serialization.
Hi Jiang,

I have written kprobes support for aarch64, and need both the
functionality (lightweight and stop_machine() versions).
I would like to rebase these API in kprobes, however slight changes
would require in case of stop_machine version, which I explained
below.
[Though kprobes cannot share Instruction encode support of jump labels
as, decoding & simulation quite different for kprobes/uprobes and
based around single stepping]

>
> Signed-off-by: Jiang Liu 
> Cc: Jiang Liu 
> ---
>  arch/arm64/include/asm/insn.h |  2 ++
>  arch/arm64/kernel/insn.c  | 64 
> +++
>  2 files changed, 66 insertions(+)
>
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index e7d1bc8..0ea7193 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -49,5 +49,7 @@ __AARCH64_INSN_FUNCS(nop, 0x, 0xD503201F)
>  enum aarch64_insn_class aarch64_get_insn_class(u32 insn);
>
>  bool aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn);
> +int aarch64_insn_patch_text(void *addr, u32 *insns, int cnt);
> +int __aarch64_insn_patch_text(void *addr, u32 *insns, int cnt);
>
>  #endif /* _ASM_ARM64_INSN_H */
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 8541c3a..50facfc 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -15,6 +15,8 @@
>   * along with this program.  If not, see .
>   */
>  #include 
> +#include 
> +#include 
>  #include 
>
>  static int aarch64_insn_cls[] = {
> @@ -69,3 +71,65 @@ bool __kprobes aarch64_insn_hotpatch_safe(u32 old_insn, 
> u32 new_insn)
> return __aarch64_insn_hotpatch_safe(old_insn) &&
>__aarch64_insn_hotpatch_safe(new_insn);
>  }
> +
> +struct aarch64_insn_patch {
> +   void*text_addr;
> +   u32 *new_insns;
> +   int insn_cnt;
> +};
> +
> +int __kprobes __aarch64_insn_patch_text(void *addr, u32 *insns, int cnt)
> +{
> +   int i;
> +   u32 *tp = addr;
> +
> +   /* instructions must be word aligned */
> +   if (cnt <= 0 || ((uintptr_t)addr & 0x3))
> +   return -EINVAL;
> +
> +   for (i = 0; i < cnt; i++)
> +   tp[i] = insns[i];
> +
> +   flush_icache_range((uintptr_t)tp, (uintptr_t)tp + cnt * sizeof(u32));
> +
> +   return 0;
> +}
Looks fine, but do you need to check for CPU big endian mode here? (I
think swab32() needed if EL1 is in big-endian mode)

> +
> +static int __kprobes aarch64_insn_patch_text_cb(void *arg)
> +{
> +   struct aarch64_insn_patch *pp = arg;
> +
> +   return __aarch64_insn_patch_text(pp->text_addr, pp->new_insns,
> +pp->insn_cnt);
> +}
> +
> +int __kprobes aarch64_insn_patch_text(void *addr, u32 *insns, int cnt)
> +{
> +   int ret;
> +   bool safe = false;
> +
> +   /* instructions must be word aligned */
> +   if (cnt <= 0 || ((uintptr_t)addr & 0x3))
> +   return -EINVAL;
> +
> +   if (cnt == 1)
> +   safe = aarch64_insn_hotpatch_safe(*(u32 *)addr, insns[0]);
> +
> +   if (safe) {
> +   ret = __aarch64_insn_patch_text(addr, insns, cnt);
> +   } else {

Can you move the code below this into separate API that just apply
patch with stop_machine? And then a wrapper function for jump label
specific handling that checks for aarch64_insn_hotpatch_safe() ?
Also, it will be good to move the patching code out of insn.c to
patch.c (refer to arch/arm/kernel/patch.c).

Please refer to attached file (my current implementation) to make
sense of exactly what kprobes would need (ignore the big-endian part
for now). I think splitting the code should be straight-forward and we
can avoid two different implementations. Please let me know if this
can be done, I will rebase my patches above your next version.

Thanks,
Sandeepa
> +   struct aarch64_insn_patch patch = {
> +   .text_addr = addr,
> +   .new_insns = insns,
> +   .insn_cnt = cnt,
> +   };
> +
> +   /*
> +* Execute __aarch64_insn_patch_text() on every online CPU,
> +* which ensure serialization among all online CPUs.
> +*/
> +   ret = stop_machine(aarch64_insn_patch_text_cb, &patch, NULL);
> +   }
> +
> +   return ret;
> +}
> --
> 1.8.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  h