uprobes: Remove redundant lock_page/unlock_page

Since read_opcode() reads from the referenced page and doesnt modify
the page contents nor the page attributes, there is no need to lock
the page.

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
uprobes: don't recheck vma/f_mapping in write_opcode()

write_opcode() rechecks valid_vma() and ->f_mapping, this is pointless.
The caller, register_for_each_vma() or uprobe_mmap(), has already done
these checks under mmap_sem.

To clarify, uprobe_mmap() checks valid_vma() only, but we can rely on
build_probe_list(vm_file->f_mapping->host).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: __replace_page() should not use page_address_in_vma()

On 06/24, Oleg Nesterov wrote:
>
> However, page_address_in_vma() can actually fail if page->mapping was
> cleared by __delete_from_page_cache() after get_user_pages() returns.
> But this means the race with page reclaim, write_opcode() should not
> fail, it should retry and read this page again. Not sure this race is
> really possible though, page_freeze_refs() logic should prevent it.

This looks a bit confusing, I'll try to to make this more clear...
The patch itself was not changed.

------------------------------------------------------------------------------
Subject: [PATCH v2 2/5] uprobes: __replace_page() should not use page_address_in_vma()

page_address_in_vma(old_page) in __replace_page() is ugly and wrong.
The caller already knows the correct virtual address, this page was
found by get_user_pages(vaddr).

However, page_address_in_vma() can actually fail if page->mapping was
cleared by __delete_from_page_cache() after get_user_pages() returns.
But this means the race with page reclaim, write_opcode() should not
fail, it should retry and read this page again. Probably the race with
remove_mapping() is not possible due to page_freeze_refs() logic, but
afaics at least shmem_writepage()->shmem_delete_from_page_cache() can
clear ->mapping.

We could change __replace_page() to return -EAGAIN in this case, but
it would be better to simply use the caller's vaddr and rely on
page_check_address().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: kill write_opcode()->lock_page(new_page)

write_opcode() does lock_page(new_page) for no reason. Nobody can
see this page until __replace_page() exposes it under ptl lock, and
we do nothing with this page after pte_unmap_unlock().

If nothing else, the similar code in do_wp_page() doesn't lock the
new page for page_add_new_anon_rmap/set_pte_at_notify.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: cleanup and document write_opcode()->lock_page(old_page)

The comment above write_opcode()->lock_page(old_page) tells about
the race with do_wp_page(). I don't really understand which exactly
race it means, but afaics this lock_page() was not enough to close
all races with do_wp_page().

Anyway, since 77fc4af1 this code is always called with ->mmap_sem
hold for writing so we can forget about do_wp_page().

However, we can't simply remove this lock_page(), and the only
(afaics) reason is __replace_page()->try_to_free_swap().

Nothing in write_opcode() needs it, move it into __replace_page()
and fix the comment.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: uprobe_mmap/munmap needs list_for_each_entry_safe()

The bug was introduced by me in 449d0d7c "uprobes: Simplify the
usage of uprobe->pending_list".

Yes, we do not care about uprobe->pending_list after return and
nobody can remove the current list entry, but put_uprobe(uprobe)
can actually free it and thus we need list_for_each_safe().

Reported-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: suppress uprobe_munmap() from mmput()

uprobe_munmap() does get_user_pages() and it is also called from
the final mmput()->exit_mmap() path. This slows down exit/mmput()
for no reason, and I think  it is simply dangerous/wrong to try to
fault-in a page into the dying mm. If nothing else, this happens
after the last sync_mm_rss(), afaics handle_mm_fault() can change
the task->rss_stat and make the subsequent check_mm() unhappy.

Change uprobe_munmap() to check mm->mm_users != 0.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: fix overflow in vma_address/find_active_uprobe

On 07/08, Joe Perches wrote:
>
> On Sun, 2012-07-08 at 22:30 +0200, Oleg Nesterov wrote:
> > @@ -1450,7 +1450,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> >
> >  			inode = vma->vm_file->f_mapping->host;
> >  			offset = bp_vaddr - vma->vm_start;
> > -			offset += (vma->vm_pgoff << PAGE_SHIFT);
> > +			offset += ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
>
> It's be nice to take remove the unnecessary parenthesis
> and make it consistent with the vaddr use above it too.

OK, please find v2 below.

------------------------------------------------------------------------------
Subject: [PATCH] uprobes: fix overflow in vma_address/find_active_uprobe

vma->vm_pgoff is "unsigned long", it should be promoted to loff_t
before the multiplication to avoid the overflow.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: kill copy_vma()->uprobe_mmap()

Kill copy_vma()->uprobe_mmap(new_vma), it is absolutely wrong.

This new_vma was just initialized to represent the new unmapped area,
[vm_start, vm_end) was returned by get_unmapped_area() in the caller.

This means that uprobe_mmap()->get_user_pages() will fail for sure,
simply because find_vma() can never succeed. And I verified that
sys_mremap()->mremap_to() indeed always fails with the wrong ENOMEM
code if [addr, addr+old_len] is probed.

And why this uprobe_mmap() was added? I believe the intent was wrong.
Note that the caller is going to do move_page_tables(), all registered
uprobes are already faulted in, we only change the virtual addresses.

NOTE: However, somehow we need to close the race with uprobe_register()
which relies on map_info->vaddr. This needs another fix I'll try to do
later. Probably we need uprobe_mmap() in move_vma() but we can not do
this right now, this can confuse uprobes_state.counter (which I still
hope we are going to kill).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: kill insert_vm_struct()->uprobe_mmap()

Kill insert_vm_struct()->uprobe_mmap(). It is not needed, nobody
except arch/ia64/kernel/perfmon.c uses insert_vm_struct(vma) with
vma->vm_file != NULL.

And it is wrong. Again, get_user_pages() can not succeed before
vma_link(vma) makes is visible to find_vma(). And even if this
worked, we must not insert the new bp before this mapping is
visible to vma_prio_tree_foreach() for uprobe_unregister().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: teach build_probe_list() to consider the range

Currently build_probe_list() builds the list of all uprobes attached
to the given inode, and the caller should filter out those who don't
fall into the [start,end) range, this is sub-optimal.

This patch turns find_least_offset_node() into find_node_in_range()
which returns the first node inside the [min,max] range, and changes
build_probe_list() to use this node as a starting point for rb_prev()
and rb_next() to find all other nodes the caller needs. The resulting
list is no longer sorted but we do not care.

This can speed up both build_probe_list() and the callers, but there
is another reason to introduce find_node_in_range(). It can be used
to figure out whether the given vma has uprobes or not, this will be
needed soon.

While at it, shift INIT_LIST_HEAD(tmp_list) into build_probe_list().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: introduce vaddr_to_offset(vma, vaddr)

Add the new helper, vaddr_to_offset(vma, vaddr) which returns the
offset in vma->vm_file this vaddr is mapped at.

Change build_probe_list() and find_active_uprobe() to use the new
helper, the next patch adds another user.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: fix register_for_each_vma()->vma_address() check

1. register_for_each_vma() checks that vma_address() == vaddr but
   this is not enough. We should also ensure that vaddr >= vm_start,
   find_vma() guarantees "vaddr < vm_end" only.

2. After the prevous changes, register_for_each_vma() is the only
   reason why vma_address() has to return loff_t, all other users
   know that we have the valid mapping at this offset and thus the
   overflow is not possible.

   Change the code to use vaddr_to_offset() instead, imho this looks
   more clean/understandable and now we can change vma_address().

3. While at it, remove the unnecessary type-cast.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: rename vma_address() and make it return "unsigned long"

1. vma_address() returns loff_t, this looks confusing and this is
   unnecessary after the previous change. Make it return "ulong",
   all callers truncate the result anyway.

2. Its name conflicts with mm/rmap.c:vma_address(), rename it to
   offset_to_vaddr(), this matches vaddr_to_offset().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
uprobes: __replace_page() needs munlock_vma_page()

Like do_wp_page(), __replace_page() should do munlock_vma_page()
for the case when the old page still has other !VM_LOCKED mappings.
Unfortunately this needs mm/internal.h.

Also, move put_page() outside of ptl lock. This doesn't really
matter but looks a bit better.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>