| Message ID | 20241105133405.2703607-4-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Masami Hiramatsu |
| Headers | show |
| Series | uprobes: Add support to optimize usdt probes on x86_64 | expand |
On Tue, Nov 5, 2024 at 5:34 AM Jiri Olsa <jolsa@kernel.org> wrote: > > Adding len argument to uprobe_write_opcode as preparation > fo writing longer instructions in following changes. typo: for > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/linux/uprobes.h | 3 ++- > kernel/events/uprobes.c | 14 ++++++++------ > 2 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > index 28068f9fcdc1..7d23a4fee6f4 100644 > --- a/include/linux/uprobes.h > +++ b/include/linux/uprobes.h > @@ -181,7 +181,8 @@ extern bool is_swbp_insn(uprobe_opcode_t *insn); > extern bool is_trap_insn(uprobe_opcode_t *insn); > extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs); > extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs); > -extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t); > +extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > + unsigned long vaddr, uprobe_opcode_t *insn, int len); is len in sizeof(uprobe_opcode_t) units or in bytes? it would be good to make this clearer it feels like passing `void *` for insns would be better, tbh... > extern struct uprobe *uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc); > extern int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool); > extern void uprobe_unregister_nosync(struct uprobe *uprobe, struct uprobe_consumer *uc); > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index e9308649bba3..3e275717789b 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -471,7 +471,7 @@ static int update_ref_ctr(struct uprobe *uprobe, struct mm_struct *mm, > * Return 0 (success) or a negative errno. > */ > int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > - unsigned long vaddr, uprobe_opcode_t opcode) > + unsigned long vaddr, uprobe_opcode_t *insn, int len) > { > struct uprobe *uprobe; > struct page *old_page, *new_page; > @@ -480,7 +480,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > bool orig_page_huge = false; > unsigned int gup_flags = FOLL_FORCE; > > - is_register = is_swbp_insn(&opcode); > + is_register = is_swbp_insn(insn); > uprobe = container_of(auprobe, struct uprobe, arch); > > retry: > @@ -491,7 +491,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > if (IS_ERR(old_page)) > return PTR_ERR(old_page); > > - ret = verify_opcode(old_page, vaddr, &opcode); > + ret = verify_opcode(old_page, vaddr, insn); > if (ret <= 0) > goto put_old; > > @@ -525,7 +525,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > > __SetPageUptodate(new_page); > copy_highpage(new_page, old_page); > - copy_to_page(new_page, vaddr, &opcode, UPROBE_SWBP_INSN_SIZE); > + copy_to_page(new_page, vaddr, insn, len); > > if (!is_register) { > struct page *orig_page; > @@ -582,7 +582,9 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > */ > int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr) > { > - return uprobe_write_opcode(auprobe, mm, vaddr, UPROBE_SWBP_INSN); > + uprobe_opcode_t insn = UPROBE_SWBP_INSN; > + > + return uprobe_write_opcode(auprobe, mm, vaddr, &insn, UPROBE_SWBP_INSN_SIZE); > } > > /** > @@ -598,7 +600,7 @@ int __weak > set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr) > { > return uprobe_write_opcode(auprobe, mm, vaddr, > - *(uprobe_opcode_t *)&auprobe->insn); > + (uprobe_opcode_t *)&auprobe->insn, UPROBE_SWBP_INSN_SIZE); > } > > /* uprobe should have guaranteed positive refcount */ > -- > 2.47.0 >
On Thu, Nov 14, 2024 at 03:41:03PM -0800, Andrii Nakryiko wrote: > On Tue, Nov 5, 2024 at 5:34 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > Adding len argument to uprobe_write_opcode as preparation > > fo writing longer instructions in following changes. > > typo: for > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > include/linux/uprobes.h | 3 ++- > > kernel/events/uprobes.c | 14 ++++++++------ > > 2 files changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > > index 28068f9fcdc1..7d23a4fee6f4 100644 > > --- a/include/linux/uprobes.h > > +++ b/include/linux/uprobes.h > > @@ -181,7 +181,8 @@ extern bool is_swbp_insn(uprobe_opcode_t *insn); > > extern bool is_trap_insn(uprobe_opcode_t *insn); > > extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs); > > extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs); > > -extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t); > > +extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > > + unsigned long vaddr, uprobe_opcode_t *insn, int len); > > is len in sizeof(uprobe_opcode_t) units or in bytes? it would be good > to make this clearer > > it feels like passing `void *` for insns would be better, tbh... good catch, it's meant as bytes, uprobe_opcode_t is u8 on x86, but other archs treat it differently, void * might solve that in this function, will check thanks, jirka > > > > > extern struct uprobe *uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc); > > extern int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool); > > extern void uprobe_unregister_nosync(struct uprobe *uprobe, struct uprobe_consumer *uc); > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > > index e9308649bba3..3e275717789b 100644 > > --- a/kernel/events/uprobes.c > > +++ b/kernel/events/uprobes.c > > @@ -471,7 +471,7 @@ static int update_ref_ctr(struct uprobe *uprobe, struct mm_struct *mm, > > * Return 0 (success) or a negative errno. > > */ > > int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > > - unsigned long vaddr, uprobe_opcode_t opcode) > > + unsigned long vaddr, uprobe_opcode_t *insn, int len) > > { > > struct uprobe *uprobe; > > struct page *old_page, *new_page; > > @@ -480,7 +480,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > > bool orig_page_huge = false; > > unsigned int gup_flags = FOLL_FORCE; > > > > - is_register = is_swbp_insn(&opcode); > > + is_register = is_swbp_insn(insn); > > uprobe = container_of(auprobe, struct uprobe, arch); > > > > retry: > > @@ -491,7 +491,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > > if (IS_ERR(old_page)) > > return PTR_ERR(old_page); > > > > - ret = verify_opcode(old_page, vaddr, &opcode); > > + ret = verify_opcode(old_page, vaddr, insn); > > if (ret <= 0) > > goto put_old; > > > > @@ -525,7 +525,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > > > > __SetPageUptodate(new_page); > > copy_highpage(new_page, old_page); > > - copy_to_page(new_page, vaddr, &opcode, UPROBE_SWBP_INSN_SIZE); > > + copy_to_page(new_page, vaddr, insn, len); > > > > if (!is_register) { > > struct page *orig_page; > > @@ -582,7 +582,9 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > > */ > > int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr) > > { > > - return uprobe_write_opcode(auprobe, mm, vaddr, UPROBE_SWBP_INSN); > > + uprobe_opcode_t insn = UPROBE_SWBP_INSN; > > + > > + return uprobe_write_opcode(auprobe, mm, vaddr, &insn, UPROBE_SWBP_INSN_SIZE); > > } > > > > /** > > @@ -598,7 +600,7 @@ int __weak > > set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr) > > { > > return uprobe_write_opcode(auprobe, mm, vaddr, > > - *(uprobe_opcode_t *)&auprobe->insn); > > + (uprobe_opcode_t *)&auprobe->insn, UPROBE_SWBP_INSN_SIZE); > > } > > > > /* uprobe should have guaranteed positive refcount */ > > -- > > 2.47.0 > >
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h index 28068f9fcdc1..7d23a4fee6f4 100644 --- a/include/linux/uprobes.h +++ b/include/linux/uprobes.h @@ -181,7 +181,8 @@ extern bool is_swbp_insn(uprobe_opcode_t *insn); extern bool is_trap_insn(uprobe_opcode_t *insn); extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs); extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs); -extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t); +extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, + unsigned long vaddr, uprobe_opcode_t *insn, int len); extern struct uprobe *uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc); extern int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool); extern void uprobe_unregister_nosync(struct uprobe *uprobe, struct uprobe_consumer *uc); diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index e9308649bba3..3e275717789b 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -471,7 +471,7 @@ static int update_ref_ctr(struct uprobe *uprobe, struct mm_struct *mm, * Return 0 (success) or a negative errno. */ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, - unsigned long vaddr, uprobe_opcode_t opcode) + unsigned long vaddr, uprobe_opcode_t *insn, int len) { struct uprobe *uprobe; struct page *old_page, *new_page; @@ -480,7 +480,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, bool orig_page_huge = false; unsigned int gup_flags = FOLL_FORCE; - is_register = is_swbp_insn(&opcode); + is_register = is_swbp_insn(insn); uprobe = container_of(auprobe, struct uprobe, arch); retry: @@ -491,7 +491,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, if (IS_ERR(old_page)) return PTR_ERR(old_page); - ret = verify_opcode(old_page, vaddr, &opcode); + ret = verify_opcode(old_page, vaddr, insn); if (ret <= 0) goto put_old; @@ -525,7 +525,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, __SetPageUptodate(new_page); copy_highpage(new_page, old_page); - copy_to_page(new_page, vaddr, &opcode, UPROBE_SWBP_INSN_SIZE); + copy_to_page(new_page, vaddr, insn, len); if (!is_register) { struct page *orig_page; @@ -582,7 +582,9 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, */ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr) { - return uprobe_write_opcode(auprobe, mm, vaddr, UPROBE_SWBP_INSN); + uprobe_opcode_t insn = UPROBE_SWBP_INSN; + + return uprobe_write_opcode(auprobe, mm, vaddr, &insn, UPROBE_SWBP_INSN_SIZE); } /** @@ -598,7 +600,7 @@ int __weak set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr) { return uprobe_write_opcode(auprobe, mm, vaddr, - *(uprobe_opcode_t *)&auprobe->insn); + (uprobe_opcode_t *)&auprobe->insn, UPROBE_SWBP_INSN_SIZE); } /* uprobe should have guaranteed positive refcount */
Adding len argument to uprobe_write_opcode as preparation fo writing longer instructions in following changes. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- include/linux/uprobes.h | 3 ++- kernel/events/uprobes.c | 14 ++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-)