Re: [PATCH v2 4/6] uprobes: travers uprobe's consumer list locklessly under SRCU protection

[Oleg Nesterov <oleg@redhat.com>]

On 08/07, Andrii Nakryiko wrote:
>
> @@ -1127,18 +1105,30 @@ 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 */
> -		if (unlikely(err))
> -			uprobe_warn(current, "unregister, leaking uprobe");
> -	}
> +
> +	list_del_rcu(&uc->cons_node);
> +	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 (unlikely(err)) {
> +		uprobe_warn(current, "unregister, leaking uprobe");
> +		return;

Looks wrong... We can (should) skip put_uprobe(), but we can't avoid
synchronize_srcu().

The caller can free the consumer right after return. You even added
a fat comment below.

Yes, the problem will go away after you split it into nosync/sync, but
still.

Oleg.