linux-kernel - Re: [PATCH 04/10] perf/uprobe: RCU-ify find

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: <20240709143255.GD28495@redhat.com>
Date: Tue, 9 Jul 2024 16:32:55 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: mingo@...nel.org, andrii@...nel.org, linux-kernel@...r.kernel.org,
	rostedt@...dmis.org, mhiramat@...nel.org, jolsa@...nel.org,
	clm@...a.com, paulmck@...nel.org
Subject: Re: [PATCH 04/10] perf/uprobe: RCU-ify find_uprobe()

On 07/08, Peter Zijlstra wrote:
>
> On Mon, Jul 08, 2024 at 06:35:45PM +0200, Oleg Nesterov wrote:
> >
> > Suppose we have uprobe U which has no extra refs, so uprobe_unregister()
> > called by the task X should remove it from uprobes_tree and kfree.
> >
> > Suppose that the task T hits the breakpoint and enters handle_swbp().
> >
> > Now,
> >
> > 	- X calls find_uprobe(), this increments U->ref from 1 to 2
> >
> > 	  register_for_each_vma() succeeds
> >
> > 	  X enters delete_uprobe()
> >
> > 	- T calls find_active_uprobe() -> find_uprobe()
> >
> > 	  __read_seqcount_begin__read_seqcount_begin() returns an even number
> >
> > 	  __find_uprobe() -> rb_find_rcu() succeeds
> >
> > 	- X continues and returns from delete_uprobe(), U->ref == 1
> >
> > 	  then it does the final uprobe_unregister()->put_uprobe(U),
> > 	  refcount_dec_and_test() succeeds, X calls call_rcu(uprobe_free_rcu).
> >
> > 	- T does get_uprobe() which changes U->ref from 0 to 1, __find_uprobe()
> > 	  returns, find_uprobe() doesn't check read_seqcount_retry().
>
> I think you're right. However, this get_uprobe() will go away in a few
> patches.

OK, I am looking at 7/10 perf/uprobe: Convert (some) uprobe->refcount to SRCU

Yes, __find_uprobe() no longer does get_uprobe(). But at first glance we have
the same problem with this change

	@@ -1977,7 +1979,7 @@ pre_ssout(struct uprobe *uprobe, struct
			return err;
		}
	 
	-	utask->active_uprobe = uprobe;
	+	utask->active_uprobe = get_uprobe(uprobe);
		utask->state = UTASK_SSTEP;
		return 0;
	 }

from 7/12 above. It can change uprobe->ref from 0 to 1 when
call_srcu(&uprobes_srcu, uprobe_free_rcu) was already scheduled.

Once guard(srcu)(&uprobes_srcu) in handle_swbp() drops the uprobes_srcu lock,
utask->active_uprobe can be freed.

Oleg.