On Thu, Aug 1, 2024 at 4:09 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Wed, Jul 31, 2024 at 02:42:50PM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> > static void put_uprobe(struct uprobe *uprobe)
> > {
> > - if (refcount_dec_and_test(&uprobe->ref)) {
> > - /*
> > - * If application munmap(exec_vma) before uprobe_unregister()
> > - * gets called, we don't get a chance to remove uprobe from
> > - * delayed_uprobe_list from remove_breakpoint(). Do it here.
> > - */
> > - mutex_lock(&delayed_uprobe_lock);
> > - delayed_uprobe_remove(uprobe, NULL);
> > - mutex_unlock(&delayed_uprobe_lock);
> > - kfree(uprobe);
> > - }
> > + if (!refcount_dec_and_test(&uprobe->ref))
> > + return;
> > +
> > + write_lock(&uprobes_treelock);
> > +
> > + if (uprobe_is_active(uprobe))
> > + rb_erase(&uprobe->rb_node, &uprobes_tree);
> > +
> > + write_unlock(&uprobes_treelock);
> > +
> > + /*
> > + * If application munmap(exec_vma) before uprobe_unregister()
> > + * gets called, we don't get a chance to remove uprobe from
> > + * delayed_uprobe_list from remove_breakpoint(). Do it here.
> > + */
> > + mutex_lock(&delayed_uprobe_lock);
> > + delayed_uprobe_remove(uprobe, NULL);
> > + mutex_unlock(&delayed_uprobe_lock);
>
> we should do kfree(uprobe) in here, right?
heh, yep, seems like I lost it while rebasing or something, good catch! fixed.
>
> I think this is fixed later on when uprobe_free_rcu is introduced
>
> SNIP
>
> > @@ -1159,27 +1180,16 @@ struct uprobe *uprobe_register(struct inode *inode,
> > if (!IS_ALIGNED(ref_ctr_offset, sizeof(short)))
> > return ERR_PTR(-EINVAL);
> >
> > - retry:
> > uprobe = alloc_uprobe(inode, offset, ref_ctr_offset);
> > if (IS_ERR(uprobe))
> > return uprobe;
> >
> > - /*
> > - * We can race with uprobe_unregister()->delete_uprobe().
> > - * Check uprobe_is_active() and retry if it is false.
> > - */
> > down_write(&uprobe->register_rwsem);
> > - ret = -EAGAIN;
> > - if (likely(uprobe_is_active(uprobe))) {
> > - consumer_add(uprobe, uc);
> > - ret = register_for_each_vma(uprobe, uc);
> > - }
> > + consumer_add(uprobe, uc);
> > + ret = register_for_each_vma(uprobe, uc);
> > up_write(&uprobe->register_rwsem);
> > - put_uprobe(uprobe);
> >
> > if (ret) {
> > - if (unlikely(ret == -EAGAIN))
> > - goto retry;
>
> nice, I like getting rid of this.. so far lgtm ;-)
>
> jirka
>
>
> > uprobe_unregister(uprobe, uc);
> > return ERR_PTR(ret);
> > }
> > @@ -1286,15 +1296,19 @@ static void build_probe_list(struct inode *inode,
> > u = rb_entry(t, struct uprobe, rb_node);
> > if (u->inode != inode || u->offset < min)
> > break;
> > + u = try_get_uprobe(u);
> > + if (!u) /* uprobe already went away, safe to ignore */
> > + continue;
> > list_add(&u->pending_list, head);
> > - get_uprobe(u);
> > }
> > for (t = n; (t = rb_next(t)); ) {
> > u = rb_entry(t, struct uprobe, rb_node);
> > if (u->inode != inode || u->offset > max)
> > break;
> > + u = try_get_uprobe(u);
> > + if (!u) /* uprobe already went away, safe to ignore */
> > + continue;
> > list_add(&u->pending_list, head);
> > - get_uprobe(u);
> > }
> > }
> > read_unlock(&uprobes_treelock);
> > @@ -1752,6 +1766,12 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
> > return -ENOMEM;
> >
> > *n = *o;
> > + /*
> > + * uprobe's refcnt has to be positive at this point, kept by
> > + * utask->return_instances items; return_instances can't be
> > + * removed right now, as task is blocked due to duping; so
> > + * get_uprobe() is safe to use here.
> > + */
> > get_uprobe(n->uprobe);
> > n->next = NULL;
> >
> > @@ -1894,7 +1914,10 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> > }
> > orig_ret_vaddr = utask->return_instances->orig_ret_vaddr;
> > }
> > -
> > + /*
> > + * uprobe's refcnt is positive, held by caller, so it's safe to
> > + * unconditionally bump it one more time here
> > + */
> > ri->uprobe = get_uprobe(uprobe);
> > ri->func = instruction_pointer(regs);
> > ri->stack = user_stack_pointer(regs);
> > --
> > 2.43.0
> >
