Re: [PATCH 04/10] perf/uprobe: RCU-ify find_uprobe()

[Oleg Nesterov]

On 07/08, Peter Zijlstra wrote:
>
> On Mon, Jul 08, 2024 at 06:35:45PM +0200, Oleg Nesterov wrote:
> >
> > Suppose we have uprobe U which has no extra refs, so uprobe_unregister()
> > called by the task X should remove it from uprobes_tree and kfree.
> >
> > Suppose that the task T hits the breakpoint and enters handle_swbp().
> >
> > Now,
> >
> > 	- X calls find_uprobe(), this increments U->ref from 1 to 2
> >
> > 	  register_for_each_vma() succeeds
> >
> > 	  X enters delete_uprobe()
> >
> > 	- T calls find_active_uprobe() -> find_uprobe()
> >
> > 	  __read_seqcount_begin__read_seqcount_begin() returns an even number
> >
> > 	  __find_uprobe() -> rb_find_rcu() succeeds
> >
> > 	- X continues and returns from delete_uprobe(), U->ref == 1
> >
> > 	  then it does the final uprobe_unregister()->put_uprobe(U),
> > 	  refcount_dec_and_test() succeeds, X calls call_rcu(uprobe_free_rcu).
> >
> > 	- T does get_uprobe() which changes U->ref from 0 to 1, __find_uprobe()
> > 	  returns, find_uprobe() doesn't check read_seqcount_retry().
>
> I think you're right. However, this get_uprobe() will go away in a few
> patches.

OK, I am looking at 7/10 perf/uprobe: Convert (some) uprobe->refcount to SRCU

Yes, __find_uprobe() no longer does get_uprobe(). But at first glance we have
the same problem with this change

	@@ -1977,7 +1979,7 @@ pre_ssout(struct uprobe *uprobe, struct
			return err;
		}
	 
	-	utask->active_uprobe = uprobe;
	+	utask->active_uprobe = get_uprobe(uprobe);
		utask->state = UTASK_SSTEP;
		return 0;
	 }

from 7/12 above. It can change uprobe->ref from 0 to 1 when
call_srcu(&uprobes_srcu, uprobe_free_rcu) was already scheduled.

Once guard(srcu)(&uprobes_srcu) in handle_swbp() drops the uprobes_srcu lock,
utask->active_uprobe can be freed.

Oleg.