[PATCH 0/2] uprobes: typeof(arch_uprobe->insn) cleanups

[PATCH 0/2] uprobes: typeof(arch_uprobe->insn) cleanups

[Oleg Nesterov]

Hello.

Ananth, could you please explicitly ack or nack 2/2 ? It is
really simple, but obviously I can't test it. And even if it
is correct it should be merged only if you like it, this is
the minor cleanup.

Oleg.

[PATCH 1/2] uprobes: Don't assume that arch_uprobe->insn/ixol is u8[MAX_UINSN_BYTES]

[Oleg Nesterov]

arch_uprobe should be opaque as much as possible to the generic
code, but currently it assumes that insn/ixol must be u8[] of the
known size. Remove this unnecessary dependency, we can use "&" and
and sizeof() with the same effect.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 2546a7b..acc0317 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -330,7 +330,7 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned
 int __weak
 set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-	return uprobe_write_opcode(mm, vaddr, *(uprobe_opcode_t *)auprobe->insn);
+	return uprobe_write_opcode(mm, vaddr, *(uprobe_opcode_t *)&auprobe->insn);
 }
 
 static int match_uprobe(struct uprobe *l, struct uprobe *r)
@@ -529,8 +529,8 @@ static int copy_insn(struct uprobe *uprobe, struct file *filp)
 {
 	struct address_space *mapping = uprobe->inode->i_mapping;
 	loff_t offs = uprobe->offset;
-	void *insn = uprobe->arch.insn;
-	int size = MAX_UINSN_BYTES;
+	void *insn = &uprobe->arch.insn;
+	int size = sizeof(uprobe->arch.insn);
 	int len, err = -EIO;
 
 	/* Copy only available bytes, -EIO if nothing was read */
@@ -569,7 +569,7 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
 		goto out;
 
 	ret = -ENOTSUPP;
-	if (is_trap_insn((uprobe_opcode_t *)uprobe->arch.insn))
+	if (is_trap_insn((uprobe_opcode_t *)&uprobe->arch.insn))
 		goto out;
 
 	ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
@@ -1264,7 +1264,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
 
 	/* Initialize the slot */
 	copy_to_page(area->page, xol_vaddr,
-			uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
+			&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.
-- 
1.5.5.1

Re: [PATCH 1/2] uprobes: Don't assume that arch_uprobe->insn/ixol is u8[MAX_UINSN_BYTES]

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-11-09 18:54:06]:

> arch_uprobe should be opaque as much as possible to the generic
> code, but currently it assumes that insn/ixol must be u8[] of the
> known size. Remove this unnecessary dependency, we can use "&" and
> and sizeof() with the same effect.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

-- 
Thanks and Regards
Srikar Dronamraju

[PATCH 2/2] uprobes/powerpc: Kill arch_uprobe->ainsn

[Oleg Nesterov]

powerpc has both arch_uprobe->insn and arch_uprobe->ainsn to
make the generic code happy. This is no longer needed after
the previous change, powerpc can just use "u32 insn".

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/powerpc/include/asm/uprobes.h |    5 ++---
 arch/powerpc/kernel/uprobes.c      |    2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
index 75c6ecd..7422a99 100644
--- a/arch/powerpc/include/asm/uprobes.h
+++ b/arch/powerpc/include/asm/uprobes.h
@@ -36,9 +36,8 @@ typedef ppc_opcode_t uprobe_opcode_t;
 
 struct arch_uprobe {
 	union {
-		u8	insn[MAX_UINSN_BYTES];
-		u8	ixol[MAX_UINSN_BYTES];
-		u32	ainsn;
+		u32	insn;
+		u32	ixol;
 	};
 };
 
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index 59f419b..003b209 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -186,7 +186,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	 * emulate_step() returns 1 if the insn was successfully emulated.
 	 * For all other cases, we need to single-step in hardware.
 	 */
-	ret = emulate_step(regs, auprobe->ainsn);
+	ret = emulate_step(regs, auprobe->insn);
 	if (ret > 0)
 		return true;
 
-- 
1.5.5.1

Re: [PATCH 2/2] uprobes/powerpc: Kill arch_uprobe->ainsn

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-11-09 18:54:09]:

> powerpc has both arch_uprobe->insn and arch_uprobe->ainsn to
> make the generic code happy. This is no longer needed after
> the previous change, powerpc can just use "u32 insn".
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Re: [PATCH 2/2] uprobes/powerpc: Kill arch_uprobe->ainsn

[Ananth N Mavinakayanahalli]

On Sat, Nov 09, 2013 at 06:54:09PM +0100, Oleg Nesterov wrote:
> powerpc has both arch_uprobe->insn and arch_uprobe->ainsn to
> make the generic code happy. This is no longer needed after
> the previous change, powerpc can just use "u32 insn".
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/powerpc/include/asm/uprobes.h |    5 ++---
>  arch/powerpc/kernel/uprobes.c      |    2 +-
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
> index 75c6ecd..7422a99 100644
> --- a/arch/powerpc/include/asm/uprobes.h
> +++ b/arch/powerpc/include/asm/uprobes.h
> @@ -36,9 +36,8 @@ typedef ppc_opcode_t uprobe_opcode_t;
> 
>  struct arch_uprobe {
>  	union {
> -		u8	insn[MAX_UINSN_BYTES];
> -		u8	ixol[MAX_UINSN_BYTES];
> -		u32	ainsn;
> +		u32	insn;
> +		u32	ixol;
>  	};
>  };
> 
> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
> index 59f419b..003b209 100644
> --- a/arch/powerpc/kernel/uprobes.c
> +++ b/arch/powerpc/kernel/uprobes.c
> @@ -186,7 +186,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  	 * emulate_step() returns 1 if the insn was successfully emulated.
>  	 * For all other cases, we need to single-step in hardware.
>  	 */
> -	ret = emulate_step(regs, auprobe->ainsn);
> +	ret = emulate_step(regs, auprobe->insn);
>  	if (ret > 0)
>  		return true;

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Thanks Oleg.

Ananth

Re: [PATCH 0/2] uprobes: typeof(arch_uprobe->insn) cleanups

[Ananth N Mavinakayanahalli]

On Sat, Nov 09, 2013 at 06:53:50PM +0100, Oleg Nesterov wrote:
> Hello.
> 
> Ananth, could you please explicitly ack or nack 2/2 ? It is
> really simple, but obviously I can't test it. And even if it
> is correct it should be merged only if you like it, this is
> the minor cleanup.

The changes look good and I have acked it.

Thanks Oleg!

Ananth