uprobes: introduce arch_uprobe->ixol

Commit Message

Oleg Nesterov Nov. 4, 2013, 7:49 p.m. UTC

On 10/29, Oleg Nesterov wrote:
>
> David. Perhaps we can avoid the new hook altogether? What if we do
> the simple change below (it ignores powerpc) ?
>
> Then arm can add "unsigned long ixol[2]" into its arch_uprobe, and
> arch_uprobe_analyze_insn() can initialize this member correctly.
>
> What do you think?

Seriouly, how about the patch below?

In fact, given that you are going to reimplement set_swbp/orig_insn,
the new member is not strictly needed (afaics). But it looks more
clear this way, and we need s/MAX_UINSN_BYTES/sizeof()/ anyway.

Oleg.
---

Subject: [PATCH] uprobes: introduce arch_uprobe->ixol

Currently xol_get_insn_slot() assumes that we should simply copy
arch_uprobe->insn[] which is (ignoring arch_uprobe_analyze_insn)
just the copy of the original insn.

This is not true for arm which needs to create another insn to
execute it out-of-line.

So this patch simply adds the new member, ->ixol into the union.
This doesn't make any difference for x86 and powerpc, but arm
can divorce insn/ixol and initialize the correct xol insn in
arch_uprobe_analyze_insn().

Signed-off-by: Oleg Nesterov <[e-mail]>
---
 arch/powerpc/include/asm/uprobes.h |    1 +
 arch/x86/include/asm/uprobes.h     |    5 ++++-
 kernel/events/uprobes.c            |    3 ++-
 3 files changed, 7 insertions(+), 2 deletions(-)

Comments

David Long Nov. 4, 2013, 9:25 p.m. UTC | #1

On 11/04/13 14:49, Oleg Nesterov wrote:
> On 10/29, Oleg Nesterov wrote:
>>
>> David. Perhaps we can avoid the new hook altogether? What if we do
>> the simple change below (it ignores powerpc) ?
>>
>> Then arm can add "unsigned long ixol[2]" into its arch_uprobe, and
>> arch_uprobe_analyze_insn() can initialize this member correctly.
>>
>> What do you think?
>
> Seriouly, how about the patch below?
>
> In fact, given that you are going to reimplement set_swbp/orig_insn,
> the new member is not strictly needed (afaics). But it looks more
> clear this way, and we need s/MAX_UINSN_BYTES/sizeof()/ anyway.
>
> Oleg.
>


Sorry for the delay, have not quite been successful in getting this to 
work yet.  Hopefully will have better results shortly.

-dl

David Long Nov. 5, 2013, 4:04 p.m. UTC | #2

On 11/04/13 14:49, Oleg Nesterov wrote:
> On 10/29, Oleg Nesterov wrote:
>>
>> David. Perhaps we can avoid the new hook altogether? What if we do
>> the simple change below (it ignores powerpc) ?
>>
>> Then arm can add "unsigned long ixol[2]" into its arch_uprobe, and
>> arch_uprobe_analyze_insn() can initialize this member correctly.
>>
>> What do you think?
>
> Seriouly, how about the patch below?
>
> In fact, given that you are going to reimplement set_swbp/orig_insn,
> the new member is not strictly needed (afaics). But it looks more
> clear this way, and we need s/MAX_UINSN_BYTES/sizeof()/ anyway.
>
> Oleg.
> ---

I agree that this is cleaner than another weak callout.  I have it 
working for ARM now.

Thanks,
-dl

Oleg Nesterov Nov. 5, 2013, 6:01 p.m. UTC | #3

On 11/05, David Long wrote:
>
> On 11/04/13 14:49, Oleg Nesterov wrote:
>> On 10/29, Oleg Nesterov wrote:
>>>
>> Seriouly, how about the patch below?
>>
>> In fact, given that you are going to reimplement set_swbp/orig_insn,
>> the new member is not strictly needed (afaics). But it looks more
>> clear this way, and we need s/MAX_UINSN_BYTES/sizeof()/ anyway.
>>
>> Oleg.
>> ---
>
> I agree that this is cleaner than another weak callout.  I have it
> working for ARM now.

OK, I am going to add this patch to my tree, thanks. Can I add your
acked-by ?

Oleg.

David Long Nov. 5, 2013, 6:45 p.m. UTC | #4

On 11/05/13 13:01, Oleg Nesterov wrote:
> On 11/05, David Long wrote:
>>
>> On 11/04/13 14:49, Oleg Nesterov wrote:
>>> On 10/29, Oleg Nesterov wrote:
>>>>
>>> Seriouly, how about the patch below?
>>>
>>> In fact, given that you are going to reimplement set_swbp/orig_insn,
>>> the new member is not strictly needed (afaics). But it looks more
>>> clear this way, and we need s/MAX_UINSN_BYTES/sizeof()/ anyway.
>>>
>>> Oleg.
>>> ---
>>
>> I agree that this is cleaner than another weak callout.  I have it
>> working for ARM now.
>
> OK, I am going to add this patch to my tree, thanks. Can I add your
> acked-by ?
>
> Oleg.
>


yes.

-dl

Patch

diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
index 2301602..541fd6f 100644
--- a/arch/powerpc/include/asm/uprobes.h
+++ b/arch/powerpc/include/asm/uprobes.h
@@ -37,6 +37,7 @@  typedef ppc_opcode_t uprobe_opcode_t;
 struct arch_uprobe {
 	union {
 		u8	insn[MAX_UINSN_BYTES];
+		u8	ixol[MAX_UINSN_BYTES];
 		u32	ainsn;
 	};
 };
diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 6e51979..2a24180 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -35,7 +35,10 @@  typedef u8 uprobe_opcode_t;
 
 struct arch_uprobe {
 	u16				fixups;
-	u8				insn[MAX_UINSN_BYTES];
+	union {
+		u8			insn[MAX_UINSN_BYTES];
+		u8			ixol[MAX_UINSN_BYTES];
+	};
 #ifdef CONFIG_X86_64
 	unsigned long			rip_rela_target_address;
 #endif
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index ad8e1bd..6aef5ad 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1256,7 +1256,8 @@  static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
 		return 0;
 
 	/* Initialize the slot */
-	copy_to_page(area->page, xol_vaddr, uprobe->arch.insn, MAX_UINSN_BYTES);
+	copy_to_page(area->page, xol_vaddr,
+			uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
 	/*
 	 * We probably need flush_icache_user_range() but it needs vma.
 	 * This should work on supported architectures too.