Re: [PATCH] uprobes: mmap_region() corrupts mm->mm_rb if uprobe_mmap() fails

[Srikar Dronamraju <srikar@linux.vnet.ibm.com>]

> 
> If mmap_region()->uprobe_mmap() fails, unmap_and_free_vma path
> does unmap_region() but does not remove the soon-to-be-freed vma
> from rb tree (actually there are more problems).
> 
> Perhaps we could do do_munmap() + return in this case, but in fact
> it is simply wrong to abort if uprobe_mmap() fails. Until at least
> we move the !UPROBE_COPY_INSN code from install_breakpoint() to
> uprobe_register().
> 
> For example, uprobe_mmap()->install_breakpoint() can fail if the
> probed insn is not supported (remember, uprobe_register() succeeds
> if nobody mmaps inode/offset), mmap() should not fail in this case.
> 
> dup_mmap()->uprobe_mmap() is wrong too by the same reason, fork()
> can race with uprobe_register() and fail for no reason if it wins
> the race and does install_breakpoint() first.
> 
> Change mmap_region() and dup_mmap() to ignore the error code from
> uprobe_mmap().
> 
> Reported-by: William Cohen <wcohen@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Cc: <stable@vger.kernel.org> # v3.5
> ---
>  kernel/fork.c |    4 ++--
>  mm/mmap.c     |    5 ++---
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index ab5211b..54bb88a 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
>  		if (retval)
>  			goto out;
> 
> -		if (file && uprobe_mmap(tmp))
> -			goto out;
> +		if (file)
> +			uprobe_mmap(tmp);

I am not comfortable with this fix.

Lets say there were 10 probes that were to be installed in that vma.
we were able to install five probes and the 6th one happened to fail
because of invalid instruction; we dont continue with the registering
probes for the remaining 4 probes.

Your fix allows probe hits for 5 registered probes that can lead to
misleading analysis. For example if one of the first five probes was a
malloc and the probe at free() was one of the last probes which wasnt
registered, then a person doing an analysis based on probes can say
there was a memory leak.

The intention behind failing mmap()/fork() if uprobe_mmap failed,
was to make sure that we always report the correct number of events.

Infact this was something that Peter advocated very strongly
http://article.gmane.org/gmane.linux.kernel.mm/59956

I think the long term solution is as you mentioned, move the
instruction analysis to register.

Till then should we just ignore invalid instruction probes similar to 
existing probes. i.e we return -ESRCH or some such which is ignored in
uprobe_mmap(), but is taken care of in the uprobe_register path.

The above may not be an elegant solution but.. 

-- 
thanks and regards
Srikar