linux-kernel - Re: [PATCH 2/8] uprobes: revamp uprobe refcounting and lifetime management

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4BzaHpGGhZzxTTS_2-MBBjXy_mU9yjg5pTc_bq6JtcFLO1w@mail.gmail.com>
Date: Thu, 1 Aug 2024 09:49:22 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jiri Olsa <olsajiri@...il.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, linux-trace-kernel@...r.kernel.org, 
	peterz@...radead.org, oleg@...hat.com, rostedt@...dmis.org, 
	mhiramat@...nel.org, bpf@...r.kernel.org, linux-kernel@...r.kernel.org, 
	paulmck@...nel.org
Subject: Re: [PATCH 2/8] uprobes: revamp uprobe refcounting and lifetime management

On Thu, Aug 1, 2024 at 4:09 AM Jiri Olsa <olsajiri@...il.com> wrote:
>
> On Wed, Jul 31, 2024 at 02:42:50PM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> >  static void put_uprobe(struct uprobe *uprobe)
> >  {
> > -     if (refcount_dec_and_test(&uprobe->ref)) {
> > -             /*
> > -              * If application munmap(exec_vma) before uprobe_unregister()
> > -              * gets called, we don't get a chance to remove uprobe from
> > -              * delayed_uprobe_list from remove_breakpoint(). Do it here.
> > -              */
> > -             mutex_lock(&delayed_uprobe_lock);
> > -             delayed_uprobe_remove(uprobe, NULL);
> > -             mutex_unlock(&delayed_uprobe_lock);
> > -             kfree(uprobe);
> > -     }
> > +     if (!refcount_dec_and_test(&uprobe->ref))
> > +             return;
> > +
> > +     write_lock(&uprobes_treelock);
> > +
> > +     if (uprobe_is_active(uprobe))
> > +             rb_erase(&uprobe->rb_node, &uprobes_tree);
> > +
> > +     write_unlock(&uprobes_treelock);
> > +
> > +     /*
> > +      * If application munmap(exec_vma) before uprobe_unregister()
> > +      * gets called, we don't get a chance to remove uprobe from
> > +      * delayed_uprobe_list from remove_breakpoint(). Do it here.
> > +      */
> > +     mutex_lock(&delayed_uprobe_lock);
> > +     delayed_uprobe_remove(uprobe, NULL);
> > +     mutex_unlock(&delayed_uprobe_lock);
>
> we should do kfree(uprobe) in here, right?

heh, yep, seems like I lost it while rebasing or something, good catch! fixed.


>
> I think this is fixed later on when uprobe_free_rcu is introduced
>
> SNIP
>
> > @@ -1159,27 +1180,16 @@ struct uprobe *uprobe_register(struct inode *inode,
> >       if (!IS_ALIGNED(ref_ctr_offset, sizeof(short)))
> >               return ERR_PTR(-EINVAL);
> >
> > - retry:
> >       uprobe = alloc_uprobe(inode, offset, ref_ctr_offset);
> >       if (IS_ERR(uprobe))
> >               return uprobe;
> >
> > -     /*
> > -      * We can race with uprobe_unregister()->delete_uprobe().
> > -      * Check uprobe_is_active() and retry if it is false.
> > -      */
> >       down_write(&uprobe->register_rwsem);
> > -     ret = -EAGAIN;
> > -     if (likely(uprobe_is_active(uprobe))) {
> > -             consumer_add(uprobe, uc);
> > -             ret = register_for_each_vma(uprobe, uc);
> > -     }
> > +     consumer_add(uprobe, uc);
> > +     ret = register_for_each_vma(uprobe, uc);
> >       up_write(&uprobe->register_rwsem);
> > -     put_uprobe(uprobe);
> >
> >       if (ret) {
> > -             if (unlikely(ret == -EAGAIN))
> > -                     goto retry;
>
> nice, I like getting rid of this.. so far lgtm ;-)
>
> jirka
>
>
> >               uprobe_unregister(uprobe, uc);
> >               return ERR_PTR(ret);
> >       }
> > @@ -1286,15 +1296,19 @@ static void build_probe_list(struct inode *inode,
> >                       u = rb_entry(t, struct uprobe, rb_node);
> >                       if (u->inode != inode || u->offset < min)
> >                               break;
> > +                     u = try_get_uprobe(u);
> > +                     if (!u) /* uprobe already went away, safe to ignore */
> > +                             continue;
> >                       list_add(&u->pending_list, head);
> > -                     get_uprobe(u);
> >               }
> >               for (t = n; (t = rb_next(t)); ) {
> >                       u = rb_entry(t, struct uprobe, rb_node);
> >                       if (u->inode != inode || u->offset > max)
> >                               break;
> > +                     u = try_get_uprobe(u);
> > +                     if (!u) /* uprobe already went away, safe to ignore */
> > +                             continue;
> >                       list_add(&u->pending_list, head);
> > -                     get_uprobe(u);
> >               }
> >       }
> >       read_unlock(&uprobes_treelock);
> > @@ -1752,6 +1766,12 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
> >                       return -ENOMEM;
> >
> >               *n = *o;
> > +             /*
> > +              * uprobe's refcnt has to be positive at this point, kept by
> > +              * utask->return_instances items; return_instances can't be
> > +              * removed right now, as task is blocked due to duping; so
> > +              * get_uprobe() is safe to use here.
> > +              */
> >               get_uprobe(n->uprobe);
> >               n->next = NULL;
> >
> > @@ -1894,7 +1914,10 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> >               }
> >               orig_ret_vaddr = utask->return_instances->orig_ret_vaddr;
> >       }
> > -
> > +      /*
> > +       * uprobe's refcnt is positive, held by caller, so it's safe to
> > +       * unconditionally bump it one more time here
> > +       */
> >       ri->uprobe = get_uprobe(uprobe);
> >       ri->func = instruction_pointer(regs);
> >       ri->stack = user_stack_pointer(regs);
> > --
> > 2.43.0
> >