linux-kernel - Re: [PATCH 3/8] uprobes: protected uprobe lifetime with SRCU

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: <CAEf4BzYnYZAOg1syEhDYG-B4Te-f1YccdJjO466h1pUJ5G4Erg@mail.gmail.com>
Date: Mon, 5 Aug 2024 10:31:36 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, linux-trace-kernel@...r.kernel.org, 
	peterz@...radead.org, rostedt@...dmis.org, mhiramat@...nel.org, 
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org, jolsa@...nel.org, 
	paulmck@...nel.org
Subject: Re: [PATCH 3/8] uprobes: protected uprobe lifetime with SRCU

On Mon, Aug 5, 2024 at 7:52 AM Oleg Nesterov <oleg@...hat.com> wrote:
>
> LGTM, just a few notes...
>
> On 07/31, Andrii Nakryiko wrote:
> >
> > @@ -59,6 +61,7 @@ struct uprobe {
> >       struct list_head        pending_list;
> >       struct uprobe_consumer  *consumers;
> >       struct inode            *inode;         /* Also hold a ref to inode */
> > +     struct rcu_head         rcu;
>
> you can probably put the new member into the union with, say, rb_node.

yep, good point, will do

>
> > @@ -1945,9 +1950,14 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
> >       if (!utask)
> >               return -ENOMEM;
> >
> > +     if (!try_get_uprobe(uprobe))
> > +             return -EINVAL;
> > +
>
> a bit off-topic right now, but it seems that we can simply kill
> utask->active_uprobe. We can turn into into "bool has_active_uprobe"
> and copy uprobe->arch into uprobe_task. Lets discuss this later.

I'm going to change this active_uprobe thing to be either refcounted
or SRCU-protected (but with timeout), so I'll need a bit more
structure around this. Let's see how that lands and if we still can
get rid of it, we can discuss.

>
> > @@ -2201,13 +2215,15 @@ static void handle_swbp(struct pt_regs *regs)
> >  {
> >       struct uprobe *uprobe;
> >       unsigned long bp_vaddr;
> > -     int is_swbp;
> > +     int is_swbp, srcu_idx;
> >
> >       bp_vaddr = uprobe_get_swbp_addr(regs);
> >       if (bp_vaddr == uprobe_get_trampoline_vaddr())
> >               return uprobe_handle_trampoline(regs);
> >
> > -     uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> > +     srcu_idx = srcu_read_lock(&uprobes_srcu);
> > +
> > +     uprobe = find_active_uprobe_rcu(bp_vaddr, &is_swbp);
> >       if (!uprobe) {
> >               if (is_swbp > 0) {
> >                       /* No matching uprobe; signal SIGTRAP. */
> > @@ -2223,6 +2239,7 @@ static void handle_swbp(struct pt_regs *regs)
> >                        */
> >                       instruction_pointer_set(regs, bp_vaddr);
> >               }
> > +             srcu_read_unlock(&uprobes_srcu, srcu_idx);
> >               return;
>
> Why not
>                 goto out;
>
> ?
>

Good point, can be goto out, will change.

> Oleg.
>