On Wed, Jun 25, 2025 at 03:42:59PM +0900, Masami Hiramatsu wrote: > On Thu, 5 Jun 2025 15:23:34 +0200 > Jiri Olsa <jo...@kernel.org> wrote: > > > Making update_ref_ctr call in uprobe_write conditional based > > on do_ref_ctr argument. This way we can use uprobe_write for > > instruction update without doing ref_ctr_offset update. > > > > Can we just decouple this update from uprobe_write()? > If we do this exclusively, I think we can do something like; > > lock() > update_ref_ctr(uprobe, mm, +1); > ... > ret = uprobe_write(); > ... > if (ret < 0) > update_ref_ctr(uprobe, mm, -1); > unlock() > > Thank you,
it was the intention in the v1 but as Oleg pointed out [1] it won't work, because the set_orig_refctr part of this change does not know if the refctr should be really updated or not while inside uprobe_write it happens right after verify callback and we make sure to update refctr only if the instruction was updated thanks, jirka [1] https://lore.kernel.org/bpf/20250427141335.ga9...@redhat.com/ > > > > Acked-by: Oleg Nesterov <o...@redhat.com> > > Signed-off-by: Jiri Olsa <jo...@kernel.org> > > --- > > include/linux/uprobes.h | 2 +- > > kernel/events/uprobes.c | 8 ++++---- > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > > index 518b26756469..5080619560d4 100644 > > --- a/include/linux/uprobes.h > > +++ b/include/linux/uprobes.h > > @@ -200,7 +200,7 @@ extern unsigned long uprobe_get_trap_addr(struct > > pt_regs *regs); > > extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct > > vm_area_struct *vma, unsigned long vaddr, uprobe_opcode_t, > > bool is_register); > > extern int uprobe_write(struct arch_uprobe *auprobe, struct vm_area_struct > > *vma, const unsigned long opcode_vaddr, > > - uprobe_opcode_t *insn, int nbytes, > > uprobe_write_verify_t verify, bool is_register); > > + uprobe_opcode_t *insn, int nbytes, > > uprobe_write_verify_t verify, bool is_register, bool do_update_ref_ctr); > > 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 1e5dc3b30707..6795b8d82b9c 100644 > > --- a/kernel/events/uprobes.c > > +++ b/kernel/events/uprobes.c > > @@ -492,12 +492,12 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, > > struct vm_area_struct *vma, > > bool is_register) > > { > > return uprobe_write(auprobe, vma, opcode_vaddr, &opcode, > > UPROBE_SWBP_INSN_SIZE, > > - verify_opcode, is_register); > > + verify_opcode, is_register, true /* > > do_update_ref_ctr */); > > } > > > > int uprobe_write(struct arch_uprobe *auprobe, struct vm_area_struct *vma, > > const unsigned long insn_vaddr, uprobe_opcode_t *insn, int > > nbytes, > > - uprobe_write_verify_t verify, bool is_register) > > + uprobe_write_verify_t verify, bool is_register, bool > > do_update_ref_ctr) > > { > > const unsigned long vaddr = insn_vaddr & PAGE_MASK; > > struct mm_struct *mm = vma->vm_mm; > > @@ -538,7 +538,7 @@ int uprobe_write(struct arch_uprobe *auprobe, struct > > vm_area_struct *vma, > > } > > > > /* We are going to replace instruction, update ref_ctr. */ > > - if (!ref_ctr_updated && uprobe->ref_ctr_offset) { > > + if (do_update_ref_ctr && !ref_ctr_updated && uprobe->ref_ctr_offset) { > > ret = update_ref_ctr(uprobe, mm, is_register ? 1 : -1); > > if (ret) { > > folio_put(folio); > > @@ -590,7 +590,7 @@ int uprobe_write(struct arch_uprobe *auprobe, struct > > vm_area_struct *vma, > > > > out: > > /* Revert back reference counter if instruction update failed. */ > > - if (ret < 0 && ref_ctr_updated) > > + if (do_update_ref_ctr && ret < 0 && ref_ctr_updated) > > update_ref_ctr(uprobe, mm, is_register ? -1 : 1); > > > > /* try collapse pmd for compound page */ > > -- > > 2.49.0 > > > > > -- > Masami Hiramatsu (Google) <mhira...@kernel.org>
