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:
+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;
+
+ err = set_swbp(&uprobe->arch, vma, vaddr);
+ if (err > 0 && 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 (err > 0 && uprobe->ref_ctr_offset)
+ update_ref_ctr(uprobe, vma->vm_mm, -1);
+ return err;
+}
but then what if update_ref_ctr fails..
>
> -------------------------------------------------------------------------------
> OTOH, I think that the current logic is not really correct too,
>
> /* Revert back reference counter if instruction update failed. */
> if (ret < 0 && is_register && ref_ctr_updated)
> update_ref_ctr(uprobe, mm, -1);
>
> I think that "Revert back reference counter" logic should not depend on
> is_register. Otherwise we can have the unbalanced update_ref_ctr(-1) if
> uprobe_unregister() fails, then another uprobe_register() comes at the
> same address, and after that uprobe_unregister() succeeds.
sounds good to me
jirka
