Re: [PATCH v2 04/12] uprobes: revamp uprobe refcounting and lifetime management

[Oleg Nesterov <oleg@redhat.com>]

And I forgot to mention...

In any case __uprobe_unregister() can't ignore the error code from
register_for_each_vma(). If it fails to restore the original insn,
we should not remove this uprobe from uprobes_tree.

Otherwise the next handle_swbp() will send SIGTRAP to the (no longer)
probed application.

On 07/05, Oleg Nesterov wrote:
>
> Tried to read this patch, but I fail to understand it. It looks
> obvioulsy wrong to me, see below.
>
> I tend to agree with the comments from Peter, but lets ignore them
> for the moment.
>
> On 07/01, Andrii Nakryiko wrote:
> >
> >  static void put_uprobe(struct uprobe *uprobe)
> >  {
> > -	if (refcount_dec_and_test(&uprobe->ref)) {
> > +	s64 v;
> > +
> > +	/*
> > +	 * here uprobe instance is guaranteed to be alive, so we use Tasks
> > +	 * Trace RCU to guarantee that uprobe won't be freed from under us, if
> > +	 * we end up being a losing "destructor" inside uprobe_treelock'ed
> > +	 * section double-checking uprobe->ref value below.
> > +	 * Note call_rcu_tasks_trace() + uprobe_free_rcu below.
> > +	 */
> > +	rcu_read_lock_trace();
> > +
> > +	v = atomic64_add_return(UPROBE_REFCNT_PUT, &uprobe->ref);
> > +
> > +	if (unlikely((u32)v == 0)) {
>
> I must have missed something, but how can this ever happen?
>
> Suppose uprobe_register(inode) is called the 1st time. To simplify, suppose
> that this binary is not used, so _register() doesn't install breakpoints/etc.
>
> IIUC, with this change (u32)uprobe->ref == 1 when uprobe_register() succeeds.
>
> Now suppose that uprobe_unregister() is called right after that. It does
>
> 	uprobe = find_uprobe(inode, offset);
>
> this increments the counter, (u32)uprobe->ref == 2
>
> 	__uprobe_unregister(...);
>
> this wont't change the counter,
>
> 	put_uprobe(uprobe);
>
> this drops the reference added by find_uprobe(), (u32)uprobe->ref == 1.
>
> Where should the "final" put_uprobe() come from?
>
> IIUC, this patch lacks another put_uprobe() after consumer_del(), no?
>
> Oleg.