Re: [PATCH v2 7/11] Uprobes Implementation

[Oleg Nesterov <oleg@redhat.com>]

On 04/23, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2010-04-22 17:40:59]:
>
> > On 04/22, Srikar Dronamraju wrote:
> > >
> > > 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.

Sure, I didn't mean exec can use MAP_SHARED or mprotect().

> 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?

Yes, that is why I mentioned register_uprobe() should check SHARED/MAYWRITE.

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

Again, I didn't mean they should. But they can.

Not only VM_SHARED, the application can create the anonymous PROT_EXEC region,
in this case write_opcode() looks wrong, please see below.

> > 	@@ -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

Yes,

> > 		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.

Of course! but see above, PageAnon() case is possible too. I think the
code should handle this case correctly anyway, but it seems it doesn't.
Not only page_add_anon_rmap() needs the locked page, I am not not sure
page_add_anon_rmap() is fine for write_opcode() which allocates the new
page. LRU? SetPageSwapBacked?

And you seem to miss my point. I think page_add_file_rmap() is always wrong.
I mean, no matter what is the page_mapping(old_page), the new page should be
mapped anonymously.

> I would leave it for vm experts to decide what the right thing to do.

Sure.

> > 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.

OK, I missed this, thanks.

> 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.

OK.

I must admit, I don't understand the usage of the lockless get_pte() in
write_opcode(). replace_page() checks orig_pte, yes. But how this check
can help write_opcode and why it is needed? I do not think it can prevent
any race, pte can be changed even before write_opcode() calls get_pte().
I guess this is only done because replace_page() requires this argument?

Oleg.