[GIT PULL] uprobes: preparations for arm port

[GIT PULL] uprobes: preparations for arm port

[Oleg Nesterov]

Ingo, please pull from

  git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core


I have also attached the cumulative diff below. As you can see,
the changes are really simple and there are no functional changes.

However I think it makes sense to merge this now, this way the
upcoming arm port doesn't (almost) need the changes outside of
arch/arm and thus it would be simpler to route everything via
arm trees.


David A. Long (1):
      uprobes: Move function declarations out of arch

Oleg Nesterov (3):
      uprobes: Kill module_init() and module_exit()
      uprobes: Introduce arch_uprobe->ixol
      uprobes: Export write_opcode() as uprobe_write_opcode()

 arch/powerpc/include/asm/uprobes.h |    8 +-------
 arch/x86/include/asm/uprobes.h     |   12 ++++--------
 include/linux/uprobes.h            |    9 +++++++++
 kernel/events/uprobes.c            |   24 ++++++++++--------------
 4 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
index 2301602..75c6ecd 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;
 	};
 };
@@ -45,11 +46,4 @@ struct arch_uprobe_task {
 	unsigned long	saved_trap_nr;
 };
 
-extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
-extern int  arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
-extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
-extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
-extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
-extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
-extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs);
 #endif	/* _ASM_UPROBES_H */
diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 6e51979..3087ea9 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
@@ -49,11 +52,4 @@ struct arch_uprobe_task {
 	unsigned int			saved_tf;
 };
 
-extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
-extern int  arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
-extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
-extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
-extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
-extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
-extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs);
 #endif	/* _ASM_UPROBES_H */
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 9e0d5a6..319eae7 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -30,6 +30,7 @@
 struct vm_area_struct;
 struct mm_struct;
 struct inode;
+struct notifier_block;
 
 #ifdef CONFIG_ARCH_SUPPORTS_UPROBES
 # include <asm/uprobes.h>
@@ -108,6 +109,7 @@ extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsign
 extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
 extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
 extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
+extern int uprobe_write_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
 extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
 extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
@@ -125,6 +127,13 @@ extern void uprobe_notify_resume(struct pt_regs *regs);
 extern bool uprobe_deny_signal(void);
 extern bool arch_uprobe_skip_sstep(struct arch_uprobe *aup, struct pt_regs *regs);
 extern void uprobe_clear_state(struct mm_struct *mm);
+extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
+extern int  arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
+extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
+extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index ae9e1d2..0ac346a 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -245,12 +245,12 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t
  * the architecture. If an arch has variable length instruction and the
  * breakpoint instruction is not of the smallest length instruction
  * supported by that architecture then we need to modify is_trap_at_addr and
- * write_opcode accordingly. This would never be a problem for archs that
- * have fixed length instructions.
+ * uprobe_write_opcode accordingly. This would never be a problem for archs
+ * that have fixed length instructions.
  */
 
 /*
- * write_opcode - write the opcode at a given virtual address.
+ * uprobe_write_opcode - write the opcode at a given virtual address.
  * @mm: the probed process address space.
  * @vaddr: the virtual address to store the opcode.
  * @opcode: opcode to be written at @vaddr.
@@ -261,7 +261,7 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t
  * For mm @mm, write the opcode at @vaddr.
  * Return 0 (success) or a negative errno.
  */
-static int write_opcode(struct mm_struct *mm, unsigned long vaddr,
+int uprobe_write_opcode(struct mm_struct *mm, unsigned long vaddr,
 			uprobe_opcode_t opcode)
 {
 	struct page *old_page, *new_page;
@@ -315,7 +315,7 @@ put_old:
  */
 int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-	return write_opcode(mm, vaddr, UPROBE_SWBP_INSN);
+	return uprobe_write_opcode(mm, vaddr, UPROBE_SWBP_INSN);
 }
 
 /**
@@ -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 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)
@@ -577,7 +577,7 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
 	if (ret)
 		goto out;
 
-	/* write_opcode() assumes we don't cross page boundary */
+	/* uprobe_write_opcode() assumes we don't cross page boundary */
 	BUG_ON((uprobe->offset & ~PAGE_MASK) +
 			UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
 
@@ -1264,7 +1264,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.
@@ -1941,9 +1942,4 @@ static int __init init_uprobes(void)
 
 	return register_die_notifier(&uprobe_exception_nb);
 }
-module_init(init_uprobes);
-
-static void __exit exit_uprobes(void)
-{
-}
-module_exit(exit_uprobes);
+__initcall(init_uprobes);

Re: [GIT PULL] uprobes: preparations for arm port

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-11-06 20:19:13]:

> Ingo, please pull from
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core
> 
> 
> I have also attached the cumulative diff below. As you can see,
> the changes are really simple and there are no functional changes.
> 
> However I think it makes sense to merge this now, this way the
> upcoming arm port doesn't (almost) need the changes outside of
> arch/arm and thus it would be simpler to route everything via
> arm trees.
> 
> 
> David A. Long (1):
>       uprobes: Move function declarations out of arch
> 
> Oleg Nesterov (3):
>       uprobes: Kill module_init() and module_exit()
>       uprobes: Introduce arch_uprobe->ixol
>       uprobes: Export write_opcode() as uprobe_write_opcode()
> 

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
for this series.

-- 
Thanks and Regards
Srikar Dronamraju

Re: [GIT PULL] uprobes: preparations for arm port

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> Ingo, please pull from
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core
> 
> 
> I have also attached the cumulative diff below. As you can see,
> the changes are really simple and there are no functional changes.
> 
> However I think it makes sense to merge this now, this way the
> upcoming arm port doesn't (almost) need the changes outside of
> arch/arm and thus it would be simpler to route everything via
> arm trees.
> 
> 
> David A. Long (1):
>       uprobes: Move function declarations out of arch
> 
> Oleg Nesterov (3):
>       uprobes: Kill module_init() and module_exit()
>       uprobes: Introduce arch_uprobe->ixol
>       uprobes: Export write_opcode() as uprobe_write_opcode()
> 
>  arch/powerpc/include/asm/uprobes.h |    8 +-------
>  arch/x86/include/asm/uprobes.h     |   12 ++++--------
>  include/linux/uprobes.h            |    9 +++++++++
>  kernel/events/uprobes.c            |   24 ++++++++++--------------
>  4 files changed, 24 insertions(+), 29 deletions(-)

Pulled into tip:perf/core, thanks a lot Oleg!

	Ingo

Re: [GIT PULL] uprobes: preparations for arm port

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> --- 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;
>  	};
>  };

> --- 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

Btw., at least on the surface, the powerpc and x86 definitions seem rather 
similar, barring senseless variations. Would it make sense to generalize 
the data structure a bit more?

Also, we all hate data structures that are not self-documenting. What does 
'ixol' mean and what is its role? Is it obvious to the reader of that 
file?

Thanks,

	Ingo

Re: [GIT PULL] uprobes: preparations for arm port

[Oleg Nesterov]

On 11/07, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > --- 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;
> >  	};
> >  };
>
> > --- 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
>
> Btw., at least on the surface, the powerpc and x86 definitions seem rather
> similar, barring senseless variations. Would it make sense to generalize
> the data structure a bit more?

Heh. You know, I have another patch, see below. It was not tested yet, it
should be splitted into 3 changes, and we need to cleanup copy_insn() first.
I didn't sent it now because I wanted to merge the minimal changes which
allow us to avoid the new arm arch_upobe_* hooks. And of course it needs
the review.

But in short, I do not think we should try to unify/generalize insn/ixol.

For the moment, please ignore the patch which adds the new ->ixol member.

The generic code knows nothing about uprobe->arch, except: arch_uprobe
must have "u8 insn[MAX_UINSN_BYTES]". And this is why powerpc also has
arch_uprobe->ainsn, it defines ->insn to make the generic code happy.

Fortunately, array == &array, so we can remove this dependency and just
require arch_uprobe->insn of any type, this is what the patch below tries
to do.

> Also, we all hate data structures that are not self-documenting. What does
> 'ixol' mean and what is its role? Is it obvious to the reader of that
> file?

Probably it makes sense a comment near "struct uprobe" anyway, will try
to do this. But I think that with the patch below the role of insn/ixol
is obvious:

	arch_uprobe->insn: this is where we copy the original instruction,
	                   arch/ can do whatever it wants with this data.

	arch_uprobe->ixol: this is what we copy into xol slot, arch/ should
	                   initialize this data.

x86 and powerpc can use the same member, arm needs to create ixol != insn.

Note: we also have set_swbp() (which doesn't use ->insn) and

	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);
	}

I _think_ this should be cleanuped too, but I am not sure and this will
conflict with the pending arm changes. So I am going to try to do this
later, after we merge arm port. And this is really minor.

Oleg.

 arch/powerpc/include/asm/uprobes.h |    5 ++---
 arch/powerpc/kernel/uprobes.c      |    2 +-
 kernel/events/uprobes.c            |   24 +++++++++++-------------
 3 files changed, 14 insertions(+), 17 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;
 
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index e615b78..aba4ce9 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -329,7 +329,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)
@@ -504,7 +504,7 @@ static bool consumer_del(struct uprobe *uprobe, struct uprobe_consumer *uc)
 }
 
 static int
-__copy_insn(struct address_space *mapping, struct file *filp, char *insn,
+__copy_insn(struct address_space *mapping, struct file *filp, void *insn,
 			unsigned long nbytes, loff_t offset)
 {
 	struct page *page;
@@ -527,28 +527,26 @@ __copy_insn(struct address_space *mapping, struct file *filp, char *insn,
 
 static int copy_insn(struct uprobe *uprobe, struct file *filp)
 {
-	struct address_space *mapping;
-	unsigned long nbytes;
-	int bytes;
+	struct address_space *mapping = uprobe->inode->i_mapping;
+	int bytes = sizeof(uprobe->arch.insn);
+	void *insn = &uprobe->arch.insn;
+	int nbytes;
 
 	nbytes = PAGE_SIZE - (uprobe->offset & ~PAGE_MASK);
-	mapping = uprobe->inode->i_mapping;
 
 	/* Instruction at end of binary; copy only available bytes */
-	if (uprobe->offset + MAX_UINSN_BYTES > uprobe->inode->i_size)
+	if (uprobe->offset + bytes > uprobe->inode->i_size)
 		bytes = uprobe->inode->i_size - uprobe->offset;
-	else
-		bytes = MAX_UINSN_BYTES;
 
 	/* Instruction at the page-boundary; copy bytes in second page */
 	if (nbytes < bytes) {
-		int err = __copy_insn(mapping, filp, uprobe->arch.insn + nbytes,
+		int err = __copy_insn(mapping, filp, insn + nbytes,
 				bytes - nbytes, uprobe->offset + nbytes);
 		if (err)
 			return err;
 		bytes = nbytes;
 	}
-	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
+	return __copy_insn(mapping, filp, insn, bytes, uprobe->offset);
 }
 
 static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
@@ -569,7 +567,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);
@@ -1257,7 +1255,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.

Re: [GIT PULL] uprobes: preparations for arm port

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 11/07, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > --- 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;
> > >  	};
> > >  };
> >
> > > --- 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
> >
> > Btw., at least on the surface, the powerpc and x86 definitions seem rather
> > similar, barring senseless variations. Would it make sense to generalize
> > the data structure a bit more?
> 
> Heh. You know, I have another patch, see below. It was not tested yet, 
> it should be splitted into 3 changes, and we need to cleanup copy_insn() 
> first. I didn't sent it now because I wanted to merge the minimal 
> changes which allow us to avoid the new arm arch_upobe_* hooks. And of 
> course it needs the review.
> 
> But in short, I do not think we should try to unify/generalize 
> insn/ixol.

That's OK.

> For the moment, please ignore the patch which adds the new ->ixol 
> member.

I didn't actually disagree with it so I pulled it - I was just wondering 
about those cleanliness details.

Thanks,

	Ingo

Re: [GIT PULL] uprobes: preparations for arm port

[Oleg Nesterov]

On 11/07, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > and we need to cleanup copy_insn()
> > first.

Heh. I never read it carefully, I always knew it should be cleanuped.

But when I looked at it now I realized that it is very wrong, and it
is very easy to crash the kernel (fortunately only root can enable
uprobes).

So we need to fix it (and cleanup), I'll try to make the patch asap.

> > For the moment, please ignore the patch which adds the new ->ixol
> > member.
>
> I didn't actually disagree with it so I pulled it

Thanks,

> - I was just wondering
> about those cleanliness details.

And you are right, this doesn't look clean. I should have mentioned
in the changelog that we need to (at least try) to cleanup this all.

Oleg.

[PATCH 0/1] uprobes: Fix the memory out of bound overwrite in copy_insn()

[Oleg Nesterov]

On 11/07, Oleg Nesterov wrote:
>
> But when I looked at it now I realized that it is very wrong, and it
> is very easy to crash the kernel (fortunately only root can enable
> uprobes).
>
> So we need to fix it (and cleanup), I'll try to make the patch asap.

Just truncate the binary after uprobe was enabled, then mmap() the
truncated area. This crashes the kernel if nobody else mmaped this
area in between.

Oleg.

[PATCH 1/1] uprobes: Fix the memory out of bound overwrite in copy_insn()

[Oleg Nesterov]

1. copy_insn() doesn't look very nice, all calculations are
   confusing and it is not immediately clear why do we read
   the 2nd page first.

2. The usage of inode->i_size is wrong on 32-bit machines.

3. "Instruction at end of binary" logic is simply wrong, it
   doesn't handle the case when uprobe->offset > inode->i_size.

   In this case "bytes" overflows, and __copy_insn() writes to
   the memory outside of uprobe->arch.insn.

   Yes, uprobe_register() checks i_size_read(), but this file
   can be truncated after that. All i_size checks are racy, we
   do this only to catch the obvious mistakes.

Change copy_insn() to call __copy_insn() in a loop, simplify
and fix the bytes/nbytes calculations.

Note: we do not care if offset + size > i_size, the users of
arch_uprobe->insn can't know how many bytes were actually copied
anyway. But perhaps this needs more changes.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index e615b78..db2802b 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -503,9 +503,8 @@ static bool consumer_del(struct uprobe *uprobe, struct uprobe_consumer *uc)
 	return ret;
 }
 
-static int
-__copy_insn(struct address_space *mapping, struct file *filp, char *insn,
-			unsigned long nbytes, loff_t offset)
+static int __copy_insn(struct address_space *mapping, struct file *filp,
+			void *insn, int nbytes, loff_t offset)
 {
 	struct page *page;
 
@@ -527,28 +526,28 @@ __copy_insn(struct address_space *mapping, struct file *filp, char *insn,
 
 static int copy_insn(struct uprobe *uprobe, struct file *filp)
 {
-	struct address_space *mapping;
-	unsigned long nbytes;
-	int bytes;
-
-	nbytes = PAGE_SIZE - (uprobe->offset & ~PAGE_MASK);
-	mapping = uprobe->inode->i_mapping;
+	struct address_space *mapping = uprobe->inode->i_mapping;
+	loff_t offs = uprobe->offset;
+	void *insn = uprobe->arch.insn;
+	int size = MAX_UINSN_BYTES;
+	int len, err = -EIO;
 
-	/* Instruction at end of binary; copy only available bytes */
-	if (uprobe->offset + MAX_UINSN_BYTES > uprobe->inode->i_size)
-		bytes = uprobe->inode->i_size - uprobe->offset;
-	else
-		bytes = MAX_UINSN_BYTES;
+	do {
+		/* Copy only available bytes, but -EIO if nothing was read */
+		if (offs >= i_size_read(uprobe->inode))
+			break;
 
-	/* Instruction at the page-boundary; copy bytes in second page */
-	if (nbytes < bytes) {
-		int err = __copy_insn(mapping, filp, uprobe->arch.insn + nbytes,
-				bytes - nbytes, uprobe->offset + nbytes);
+		len = min_t(int, size, PAGE_SIZE - (offs & ~PAGE_MASK));
+		err = __copy_insn(mapping, filp, insn, len, offs);
 		if (err)
-			return err;
-		bytes = nbytes;
-	}
-	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
+			break;
+
+		insn += len;
+		offs += len;
+		size -= len;
+	} while (size);
+
+	return err;
 }
 
 static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
-- 
1.5.5.1

Re: [PATCH 1/1] uprobes: Fix the memory out of bound overwrite in copy_insn()

[Oleg Nesterov]

On 11/07, Oleg Nesterov wrote:
>
> Note: we do not care if offset + size > i_size, the users of
> arch_uprobe->insn can't know how many bytes were actually copied
> anyway. But perhaps this needs more changes.

I guess this not is not very clear...

What I tried to say. Suppose that offset == 0 and i_size = 1.
In this case we still copy MAX_UINSN_BYTES into arch.insn
because the task gets the same page after page fault anyway,
and we can't inform arch_uprobe_analyze_insn() that we have
only read 1 byte. So this doesn't matter.

I changed this as follows:

	Note: we do not care if we read extra bytes after inode->i_size
	if we got the valid page. This is fine because the task gets the
	same page after page-fault, and arch_uprobe_analyze_insn() can't
	know how how many bytes were actually read anyway.

I also moved the comment up, before the main loop.

Srikar, I am going to add this to my tree unless you object now.
I verified that this indeed fixes the crash and the code looks
"obviously correct, see below.

Oleg.

static int __copy_insn(struct address_space *mapping, struct file *filp,
			void *insn, int nbytes, loff_t offset)
{
	struct page *page;

	if (!mapping->a_ops->readpage)
		return -EIO;
	/*
	 * Ensure that the page that has the original instruction is
	 * populated and in page-cache.
	 */
	page = read_mapping_page(mapping, offset >> PAGE_CACHE_SHIFT, filp);
	if (IS_ERR(page))
		return PTR_ERR(page);

	copy_from_page(page, offset, insn, nbytes);
	page_cache_release(page);

	return 0;
}

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;
	int len, err = -EIO;

	/* Copy only available bytes, -EIO if nothing was read */
	do {
		if (offs >= i_size_read(uprobe->inode))
			break;

		len = min_t(int, size, PAGE_SIZE - (offs & ~PAGE_MASK));
		err = __copy_insn(mapping, filp, insn, len, offs);
		if (err)
			break;

		insn += len;
		offs += len;
		size -= len;
	} while (size);

	return err;
}

Re: [PATCH 1/1] uprobes: Fix the memory out of bound overwrite in copy_insn()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-11-07 20:40:32]:

> 1. copy_insn() doesn't look very nice, all calculations are
>    confusing and it is not immediately clear why do we read
>    the 2nd page first.
> 
> 2. The usage of inode->i_size is wrong on 32-bit machines.
> 
> 3. "Instruction at end of binary" logic is simply wrong, it
>    doesn't handle the case when uprobe->offset > inode->i_size.
> 
>    In this case "bytes" overflows, and __copy_insn() writes to
>    the memory outside of uprobe->arch.insn.
> 
>    Yes, uprobe_register() checks i_size_read(), but this file
>    can be truncated after that. All i_size checks are racy, we
>    do this only to catch the obvious mistakes.
> 
> Change copy_insn() to call __copy_insn() in a loop, simplify
> and fix the bytes/nbytes calculations.
> 
> Note: we do not care if offset + size > i_size, the users of
> arch_uprobe->insn can't know how many bytes were actually copied
> anyway. But perhaps this needs more changes.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

-- 
Thanks and Regards
Srikar Dronamraju