On Thu, Aug 1, 2024 at 7:27 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Wed, Jul 31, 2024 at 02:42:53PM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> > static int __copy_insn(struct address_space *mapping, struct file *filp,
> > void *insn, int nbytes, loff_t offset)
> > {
> > @@ -924,7 +901,8 @@ static bool filter_chain(struct uprobe *uprobe, struct mm_struct *mm)
> > bool ret = false;
> >
> > down_read(&uprobe->consumer_rwsem);
> > - for (uc = uprobe->consumers; uc; uc = uc->next) {
> > + list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
> > + srcu_read_lock_held(&uprobes_srcu)) {
> > ret = consumer_filter(uc, mm);
> > if (ret)
> > break;
> > @@ -1120,17 +1098,19 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> > int err;
> >
> > down_write(&uprobe->register_rwsem);
> > - if (WARN_ON(!consumer_del(uprobe, uc))) {
> > - err = -ENOENT;
> > - } else {
> > - err = register_for_each_vma(uprobe, NULL);
> > - /* TODO : cant unregister? schedule a worker thread */
> > - WARN(err, "leaking uprobe due to failed unregistration");
> > - }
> > +
> > + list_del_rcu(&uc->cons_node);
>
> hum, so previous code had a check to verify that consumer is actually
> registered in the uprobe, so it'd survive wrong argument while the new
> code could likely do things?
correct, passing consumer that's not really registered to
uprobe_unregister() is a huge violation of uprobe API contract and it
should never happen (and it doesn't), so it feels like we can drop
this overly cautious and permissive part (we don't protect against
passing wrong pointers, NULLs, etc, right? so why would we protect
against wrong unregister or say double unregister?)
>
> > + err = register_for_each_vma(uprobe, NULL);
> > +
> > up_write(&uprobe->register_rwsem);
> >
> > - if (!err)
> > - put_uprobe(uprobe);
> > + /* TODO : cant unregister? schedule a worker thread */
> > + if (WARN(err, "leaking uprobe due to failed unregistration"))
> > + return;
> > +
> > + put_uprobe(uprobe);
> > +
> > + synchronize_srcu(&uprobes_srcu);
>
> could you comment on why it's needed in here? there's already potential
> call_srcu(&uprobes_srcu, ... ) call in put_uprobe above
>
yep, I should. This is because we might have handle_swbp() traversing
the consumer list in parallel with unregistration, and so it might
have already seen this consumer and is calling its callback. So we
need to wait for srcu grace period to make sure we don't have any
calls to consumer's callback. If we don't do that, the caller can free
the consumer's memory as handle_swbp() is still using/calling into it.
> thanks,
> jirka
