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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jul 06, 2024 at 07:00:34PM +0200, Jiri Olsa wrote:
> On Fri, Jul 05, 2024 at 05:37:05PM +0200, 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,
> 
> __uprobe_unregister calls delete_uprobe that calls put_uprobe ?

ugh, wrong sources.. ok, don't know ;-)

jirka

> 
> jirka
> 
> > 
> > 	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.
> > 




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux