Re: [PATCH v2 7/11] Uprobes Implementation

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

* Oleg Nesterov <oleg@redhat.com> [2010-04-22 17:40:59]:

> On 04/22, Srikar Dronamraju wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> [2010-04-21 18:05:15]:
> >
> > > 3. mprotect(). write_opcode() checks !VM_WRITE. This is correct,
> > >    otherwise we can race with the user-space writing to the same
> > >    page.
> > >
> > >    But suppose that the application does mprotect(PROT_WRITE) after
> > >    register_uprobe() installs the bp, now unregister_uprobe/etc can't
> > >    restore the original insn?
> > >
> >
> > I still need to verify this. I shall get back to you on this.
> > However are there applications that mprotect(PROT_WRITE) text pages?
> 
> Well, I think the kernel should assume that the user-space can do
> anything.
> 
> Hmm. And if this vma is VM_SHARED, then this bp could be actually
> written to vm_file after mprotect().

When I look through the load_.*_binary and load_.*_library functions,
they seem to map the text regions MAP_PRIVATE|MAP_DENY_WRITE. (Few
exceptions like load_som_binary that seem to map text regions with
MAP_PRIVATE only).

Also if vma are marked VM_SHARED and bp are inserted through ptrace,
i.e(access_process_vm/get_user_pages), then we would still be writing to
vm_file after mprotect?

Again, I am not sure if executable pages should be marked VM_SHARED.

> 
> But I think this doesn't really matter. When I actually look at
> patches 3 and 4, I am starting to think this all is very wrong.
> 
> > I am copying Mel Gorman and Andrea Arcangeli so that they can provide
> > their inputs on VM and KSM related issues.
> 
> Yes. We need vm experts here, I am not. Still, I'd like to share my
> concerns. I also added Rik and Hugh.
> 
> 
> So, 3/11 does
> 
> 	@@ -2617,7 +2617,10 @@ int replace_page(struct vm_area_struct *vma, struct page *page,
> 		}
> 	 
> 		get_page(kpage);
> 	-	page_add_anon_rmap(kpage, vma, addr);
> 	+	if (PageAnon(kpage))
> 	+		page_add_anon_rmap(kpage, vma, addr);
> 	+	else
> 	+		page_add_file_rmap(kpage);
> 	 
> 		flush_cache_page(vma, addr, pte_pfn(*ptep));
> 		ptep_clear_flush(vma, addr, ptep);
> 
> I see no point in this patch, please see below.
> 
> The next 4/11 patch introduces write_opcode() which roughly does:
> 
> 	int write_opcode(unsigned long vaddr, user_bkpt_opcode_t opcode)
> 	{
> 		get_user_pages(write => false, &old_page);
> 
> 		new_page = alloc_page_vma(...);
> 
> 		... insert the bp into the new_page ...
> 
> 		new_page->mapping = old_page->mapping;
> 		new_page->index = old_page->index;
> 
> 		replace_page(old_page, new_page);
> 	}
> 
> This doesn't look right at all to me.
> 
> 	IF PageAnon(old_page):
		   ^^^ newpage

> 
> 		in this case replace_page() calls page_add_anon_rmap() which
> 		needs the locked page.
> 
> 	ELSE:
> 
> 		I don't think the new page should evere preserve the mapping,
> 		this looks just wrong. It should be always anonymous.

I did verify that page_add_file_rmap gets called from replace_page when 
we insert or remove a probe.
This should be because uprobes doesnt do a anon_vma_prepare() before the
alloc_page_vma(). 
I would leave it for vm experts to decide what the right thing to do.

> 
> 
> And in fact, I do not understand why write_opcode() needs replace_page().
> It could just use get_user_pages(FOLL_WRITE | FOLL_FORCE), no? It should
> create the anonymous page correctly.

We were earlier doing access_process_vm that would inturn call
get_user_pages to COW the page. However that needed that the threads of
the target process be stopped.

In the access_process_vm method,
1. we get a copy of page, 
2. flush the tlbs.
3. modify the page. 

The concern was if the threads were executing in the vicinity.
Hence we were stopping all threads while inserting/deleting breakpoints.


Background page replacement was suggested by Linus and Peter. 
In this method.
1. we get a copy of the page.
2. modify the page 
3. flush the tlbs.

This method is suppose to be atomic enuf that we dont need to stop the
threads.

> 
> Either way, I think register_uprobe() should disallow the probes in
> VM_SHARED/VM_MAYWRITE vmas.

Yes, we certainly could add that check. 

--
Thanks and Regards
Srikar