Re: [PATCH 05/10] perf/uprobe: SRCU-ify uprobe->consumer list

[Oleg Nesterov <oleg@redhat.com>]

On 07/09, Peter Zijlstra wrote:
>
> On Tue, Jul 09, 2024 at 03:33:49PM +0200, Oleg Nesterov wrote:
> > On 07/09, Peter Zijlstra wrote:
> > >
> > > > +	guard(srcu)(&uprobes_srcu);
> > > > +
> > > > +	for_each_consumer_rcu(uc, uprobe->consumers) {
> > > >  		int rc = 0;
> > > >
> > > >  		if (uc->handler) {
> > > > @@ -2116,7 +2126,6 @@ static void handler_chain(struct uprobe
> > > >  		WARN_ON(!uprobe_is_active(uprobe));
> > > >  		unapply_uprobe(uprobe, current->mm);
> > >
> > >    ^^^ this remove case needs more thought.
> >
> > Yeah... that is why the current code doesn't use ->consumer_rwsem, iirc.
>
> AFAICT something like the below should work. Concurrent
> remove_breakpoint() should already be possible today and doesn't appear
> to be a problem.

Sorry, I don't understand how can this patch help. Yes, it removes the
uprobe->consumers != NULL check, but this is minor.

To simplify, suppose we have a single consumer which is not interested
in this task/mm, it returns UPROBE_HANDLER_REMOVE.

For example, event->hw.target != NULL and the current task is the forked
child which hits the breakpoint copied by dup_mmap().

Now. We need to ensure that another (say system-wide) consumer can't come
and call register_for_each_vma() before unapply_uprobe().

But perhaps I missed your point...

Oleg.

> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1260,6 +1260,10 @@ int uprobe_apply(struct inode *inode, lo
>  	return ret;
>  }
>
> +/*
> + * Can race against uprobe_unregister() / register_for_each_vma(), and relies
> + * on duplicate remove_breakpoint() being a no-op.
> + */
>  static int unapply_uprobe(struct uprobe *uprobe, struct mm_struct *mm)
>  {
>  	VMA_ITERATOR(vmi, mm, 0);
> @@ -2101,6 +2105,7 @@ static void handler_chain(struct uprobe
>  	struct uprobe_consumer *uc;
>  	int remove = UPROBE_HANDLER_REMOVE;
>  	bool need_prep = false; /* prepare return uprobe, when needed */
> +	bool had_handler = false;
>
>  	down_read(&uprobe->register_rwsem);
>  	for (uc = uprobe->consumers; uc; uc = uc->next) {
> @@ -2115,16 +2120,26 @@ static void handler_chain(struct uprobe
>  		if (uc->ret_handler)
>  			need_prep = true;
>
> +		/*
> +		 * A single handler that does not mask out REMOVE, means the
> +		 * probe stays.
> +		 */
> +		had_handler = true;
>  		remove &= rc;
>  	}
>
> +	/*
> +	 * If there were no handlers called, nobody asked for it to be removed
> +	 * but also nobody got to mask the value. Fix it up.
> +	 */
> +	if (!had_handler)
> +		remove = 0;
> +
>  	if (need_prep && !remove)
>  		prepare_uretprobe(uprobe, regs); /* put bp at return */
>
> -	if (remove && uprobe->consumers) {
> -		WARN_ON(!uprobe_is_active(uprobe));
> +	if (remove)
>  		unapply_uprobe(uprobe, current->mm);
> -	}
>  	up_read(&uprobe->register_rwsem);
>  }
>
>