| Message ID | 20250224140151.667679-7-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | uprobes: Add support to optimize usdt probes on x86_64 | expand |
On Mon, Feb 24, 2025 at 6:03 AM Jiri Olsa <jolsa@kernel.org> wrote: > > The uprobe_write has special path to restore the original page when > we write original instruction back. > > This happens when uprobe_write detects that we want to write anything > else but breakpoint instruction. > > In following changes we want to use uprobe_write function for multiple > updates, so adding new function argument to denote that this is the > original instruction update. This way uprobe_write can make appropriate > checks and restore the original page when possible. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > arch/arm/probes/uprobes/core.c | 2 +- > include/linux/uprobes.h | 5 +++-- > kernel/events/uprobes.c | 22 ++++++++++------------ > 3 files changed, 14 insertions(+), 15 deletions(-) > [...] > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index ad5879fc2d26..2b542043089e 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -471,25 +471,23 @@ 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 opcode, bool orig) > { > - return uprobe_write(auprobe, mm, vaddr, &opcode, UPROBE_SWBP_INSN_SIZE, verify_opcode); > + return uprobe_write(auprobe, mm, vaddr, &opcode, UPROBE_SWBP_INSN_SIZE, verify_opcode, orig); > } > > int uprobe_write(struct arch_uprobe *auprobe, struct mm_struct *mm, > unsigned long vaddr, uprobe_opcode_t *insn, > - int nbytes, uprobe_write_verify_t verify) > + int nbytes, uprobe_write_verify_t verify, bool orig) why not call orig -> is_register and avoid a bunch of code churn?... (and while "is_register" is not awesome name, still a bit more clear compared to "orig", IMO) > { > struct page *old_page, *new_page; > struct vm_area_struct *vma; > - int ret, is_register; > + int ret; > bool orig_page_huge = false; > unsigned int gup_flags = FOLL_FORCE; > > - is_register = is_swbp_insn(insn); > - [...]
On Fri, Feb 28, 2025 at 11:07:38AM -0800, Andrii Nakryiko wrote: > On Mon, Feb 24, 2025 at 6:03 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > The uprobe_write has special path to restore the original page when > > we write original instruction back. > > > > This happens when uprobe_write detects that we want to write anything > > else but breakpoint instruction. > > > > In following changes we want to use uprobe_write function for multiple > > updates, so adding new function argument to denote that this is the > > original instruction update. This way uprobe_write can make appropriate > > checks and restore the original page when possible. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > arch/arm/probes/uprobes/core.c | 2 +- > > include/linux/uprobes.h | 5 +++-- > > kernel/events/uprobes.c | 22 ++++++++++------------ > > 3 files changed, 14 insertions(+), 15 deletions(-) > > > > [...] > > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > > index ad5879fc2d26..2b542043089e 100644 > > --- a/kernel/events/uprobes.c > > +++ b/kernel/events/uprobes.c > > @@ -471,25 +471,23 @@ 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 opcode, bool orig) > > { > > - return uprobe_write(auprobe, mm, vaddr, &opcode, UPROBE_SWBP_INSN_SIZE, verify_opcode); > > + return uprobe_write(auprobe, mm, vaddr, &opcode, UPROBE_SWBP_INSN_SIZE, verify_opcode, orig); > > } > > > > int uprobe_write(struct arch_uprobe *auprobe, struct mm_struct *mm, > > unsigned long vaddr, uprobe_opcode_t *insn, > > - int nbytes, uprobe_write_verify_t verify) > > + int nbytes, uprobe_write_verify_t verify, bool orig) > > why not call orig -> is_register and avoid a bunch of code churn?... > (and while "is_register" is not awesome name, still a bit more clear > compared to "orig", IMO) I see the logic in the function the other way around: if you want to try and load the original page as part of your update, then you pass true to orig argument also the is_register makes sense to me in the old code where you have just 2 states: breakpoint or original instruction ... now when we added call instruction the is_register would need to cover that as well jirka > > > { > > struct page *old_page, *new_page; > > struct vm_area_struct *vma; > > - int ret, is_register; > > + int ret; > > bool orig_page_huge = false; > > unsigned int gup_flags = FOLL_FORCE; > > > > - is_register = is_swbp_insn(insn); > > - > > [...]
diff --git a/arch/arm/probes/uprobes/core.c b/arch/arm/probes/uprobes/core.c index f5f790c6e5f8..54a90b565285 100644 --- a/arch/arm/probes/uprobes/core.c +++ b/arch/arm/probes/uprobes/core.c @@ -30,7 +30,7 @@ int set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr) { return uprobe_write_opcode(auprobe, mm, vaddr, - __opcode_to_mem_arm(auprobe->bpinsn)); + __opcode_to_mem_arm(auprobe->bpinsn), false); } bool arch_uprobe_ignore(struct arch_uprobe *auprobe, struct pt_regs *regs) diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h index 8867b6a168b2..1abcae9cde48 100644 --- a/include/linux/uprobes.h +++ b/include/linux/uprobes.h @@ -194,9 +194,10 @@ 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, bool); extern int uprobe_write(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, - uprobe_opcode_t *insn, int nbytes, uprobe_write_verify_t verify); + uprobe_opcode_t *insn, int nbytes, uprobe_write_verify_t verify, bool orig); 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 ad5879fc2d26..2b542043089e 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -471,25 +471,23 @@ 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 opcode, bool orig) { - return uprobe_write(auprobe, mm, vaddr, &opcode, UPROBE_SWBP_INSN_SIZE, verify_opcode); + return uprobe_write(auprobe, mm, vaddr, &opcode, UPROBE_SWBP_INSN_SIZE, verify_opcode, orig); } int uprobe_write(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t *insn, - int nbytes, uprobe_write_verify_t verify) + int nbytes, uprobe_write_verify_t verify, bool orig) { struct page *old_page, *new_page; struct vm_area_struct *vma; - int ret, is_register; + int ret; bool orig_page_huge = false; unsigned int gup_flags = FOLL_FORCE; - is_register = is_swbp_insn(insn); - retry: - if (is_register) + if (!orig) gup_flags |= FOLL_SPLIT_PMD; /* Read the page with vaddr into memory */ old_page = get_user_page_vma_remote(mm, vaddr, gup_flags, &vma); @@ -500,14 +498,14 @@ int uprobe_write(struct arch_uprobe *auprobe, struct mm_struct *mm, if (ret <= 0) goto put_old; - if (WARN(!is_register && PageCompound(old_page), + if (WARN(orig && PageCompound(old_page), "uprobe unregister should never work on compound page\n")) { ret = -EINVAL; goto put_old; } ret = 0; - if (!is_register && !PageAnon(old_page)) + if (orig && !PageAnon(old_page)) goto put_old; ret = anon_vma_prepare(vma); @@ -523,7 +521,7 @@ int uprobe_write(struct arch_uprobe *auprobe, struct mm_struct *mm, copy_highpage(new_page, old_page); uprobe_copy_to_page(new_page, vaddr, insn, nbytes); - if (!is_register) { + if (orig) { struct page *orig_page; pgoff_t index; @@ -574,7 +572,7 @@ int uprobe_write(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); + return uprobe_write_opcode(auprobe, mm, vaddr, UPROBE_SWBP_INSN, false); } static int set_swbp_refctr(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr) @@ -609,7 +607,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, true); } static int set_orig_refctr(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
The uprobe_write has special path to restore the original page when we write original instruction back. This happens when uprobe_write detects that we want to write anything else but breakpoint instruction. In following changes we want to use uprobe_write function for multiple updates, so adding new function argument to denote that this is the original instruction update. This way uprobe_write can make appropriate checks and restore the original page when possible. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- arch/arm/probes/uprobes/core.c | 2 +- include/linux/uprobes.h | 5 +++-- kernel/events/uprobes.c | 22 ++++++++++------------ 3 files changed, 14 insertions(+), 15 deletions(-)