Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1742116 > unrolled thread

Re: [PATCH v3 3/3] kernel/uprobes: Fix check for active uprobe

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 3/3] kernel/uprobes: Fix check for active uprobe Srikar Dronamraju <srikar@linux.vnet.ibm.com> - 2017-09-29 15:30 +0200
    Re: [PATCH v3 3/3] kernel/uprobes: Fix check for active uprobe Oleg Nesterov <oleg@redhat.com> - 2017-09-29 19:00 +0200

#1742116 — Re: [PATCH v3 3/3] kernel/uprobes: Fix check for active uprobe

> >
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -1751,6 +1751,19 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> >  			uprobe = find_uprobe(inode, offset);
> >  		}
> >  
> > +		/* Ensure that the breakpoint was actually installed */
> > +		if (uprobe) {
> > +			/*
> > +			 * TODO: move copy_insn/etc into _register and remove
> > +			 * this hack.  After we hit the bp, _unregister +
> > +			 * _register can install the new and not-yet-analyzed
> > +			 * uprobe at the same address, restart.
> > +			 */
> > +			smp_rmb(); /* pairs with wmb() in prepare_uprobe() */
> > +			if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
> > +				uprobe = NULL;
> > +		}
> 
> ACK ...
> 
> but the comment is no longer valid, it only mentions the race  unregister +
> register.
> 
> And note that "restart" is not true in that we are not going to simply restart,
> we will check is_trap_at_addr() and then either send SIGTRAP or restart.
> 
> This is correct because we do this check under mmap_sem so we can't race with
> install_breakpoint(), so is_trap_at_addr() == T can't be falsely true if
> UPROBE_COPY_INSN is not set.
> 

Right, Given that we are doing this in the mmap_sem, we should also be
removing the rmb/wmb pairs too.

Right Oleg?


> And btw, perhaps you should do this check right after find_uprobe() in the
> if (valid_vma) block.
> 
> Oleg.
> 

[toc] | [next] | [standalone]

#1742243

On 09/29, Srikar Dronamraju wrote:
>
> > This is correct because we do this check under mmap_sem so we can't race with
> > install_breakpoint(), so is_trap_at_addr() == T can't be falsely true if
> > UPROBE_COPY_INSN is not set.
> >
>
> Right, Given that we are doing this in the mmap_sem, we should also be
> removing the rmb/wmb pairs too.

Well, down_read(&mm->mmap_sem) can only guarantee that this mm can not be
modified by install_breakpoint().

But what if, say, another task with different ->mm does uprobe_mmap() and
calls prepare_uprobe() for the 1st time?

Or suppose we race with unregister+register...

OTOH, I agree that we can remove these barriers, but this needs a lengthy
comment while the current code looks "obviously correct" in that you do
not even need to think about potential races.

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web