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.
