On Mon, Apr 28, 2025 at 12:51:57PM +0200, Jiri Olsa wrote:
> On Sun, Apr 27, 2025 at 04:13:35PM +0200, Oleg Nesterov wrote:
> > On 04/21, Jiri Olsa wrote:
> > >
> > > +static int set_swbp_refctr(struct uprobe *uprobe, struct vm_area_struct
> > > *vma, unsigned long vaddr)
> > > +{
> > > + struct mm_struct *mm = vma->vm_mm;
> > > + int err;
> > > +
> > > + /* We are going to replace instruction, update ref_ctr. */
> > > + if (uprobe->ref_ctr_offset) {
> > > + err = update_ref_ctr(uprobe, mm, 1);
> > > + if (err)
> > > + return err;
> > > + }
> > > +
> > > + err = set_swbp(&uprobe->arch, vma, vaddr);
> > > +
> > > + /* Revert back reference counter if instruction update failed. */
> > > + if (err && uprobe->ref_ctr_offset)
> > > + update_ref_ctr(uprobe, mm, -1);
> > > + return err;
> > > }
> > ...
> > > +static int set_orig_refctr(struct uprobe *uprobe, struct vm_area_struct
> > > *vma, unsigned long vaddr)
> > > +{
> > > + int err = set_orig_insn(&uprobe->arch, vma, vaddr);
> > > +
> > > + /* Revert back reference counter even if instruction update failed. */
> > > + if (uprobe->ref_ctr_offset)
> > > + update_ref_ctr(uprobe, vma->vm_mm, -1);
> > > + return err;
> > > }
> >
> > This doesn't look right even in the simplest case...
> >
> > To simplify, suppose that uprobe_register() needs to change a single mm/vma
> > and set_swbp() fails. In this case uprobe_register() calls
> > uprobe_unregister()
> > which will find the same vma and call set_orig_refctr(). set_orig_insn()
> > will
> > do nothing. But update_ref_ctr(uprobe, vma->vm_mm, -1) is wrong/unbalanced.
> >
> > The current code updates ref_ctr after the verify_opcode() check, so it
> > doesn't
> > have this problem.
>
> ah right :-\
>
> could set_swbp/set_orig_insn return > 0 in case the memory was actually
> updated?
> and we would update the refctr based on that, like:
ok, I think we need to keep the refcnt update inside write_insn and enable it
through argument, so I can use write_insn from swbp_optimize/swbp_unoptimize
and tell it not to do refcnt update
jirka
