Re: [PATCH 1/2] uprobes: document the usage of mm->mmap_lock

[Google]

On Thu, 11 Jul 2024 11:49:40 +0200
Oleg Nesterov <oleg@xxxxxxxxxx> wrote:

> On 07/11, Masami Hiramatsu wrote:
> >
> > On Wed, 10 Jul 2024 17:10:07 +0200
> > Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> >
> > > > >  int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
> > > > > @@ -1046,7 +1046,12 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
> > > > >
> > > > >  		if (err && is_register)
> > > > >  			goto free;
> > > > > -
> > > > > +		/*
> > > > > +		 * We take mmap_lock for writing to avoid the race with
> > > > > +		 * find_active_uprobe(), install_breakpoint() must not
> > > > > +		 * make is_trap_at_addr() true right after find_uprobe()
> > > > > +		 * returns NULL.
> > > >
> 
> ...
> 
> > OK, but it seems we should write the above longer explanation here.
> > What about the comment like this?
> 
> Well, I am biased, but your version looks much more confusing to me...
> 
> > /*
> >  * We take mmap_lock for writing to avoid the race with
> >  * find_active_uprobe() and is_trap_at_adder() in reader
> >  * side.
> >  * If the reader, which hits a swbp and is handling it,
> >  * does not take mmap_lock for reading,
> 
> this looks as if the reader which hits a swbp takes mmap_lock for reading
> because of this race. No, find_active_uprobe() needs mmap_read_lock() for
> vma_lookup, get_user_pages, etc.

OK, so it is for looking up VMA. (But in the end, this rock protects both
the VMAs and uprobes, right?)

> 
> > it is possible
> >  * that find_active_uprobe() returns NULL (because
> >  * uprobe_unregister() removes uprobes right before that),
> >  * but is_trap_at_addr() can return true afterwards (because
> >  * another thread calls uprobe_register() on the same address).
>      ^^^^^^^^^^^^^^^
> We are the thread which called uprobe_register(), we are going to
> do install_breakpoint().

Ah, yes :)

What about this?

	 * We take mmap_lock for writing to avoid the race with
	 * find_active_uprobe(), which takes mmap_lock for reading.
	 * Thus this install_breakpoint() must not make
	 * is_trap_at_addr() true right after find_uprobe()
	 * returns NULL in find_active_uprobe().


> 
> And btw, not that I think this makes sense, but register_for_each_vma()
> could probably do
> 
> 	if (is_register)
> 		mmap_write_lock(mm);
> 	else
> 		mmap_read_lock(mm);

Agreed.

Thank you,

> 
> Oleg.
> 


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>