Messages in this thread | | | Date | Fri, 23 Apr 2010 20:53:41 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH v2 7/11] Uprobes Implementation |
| |
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.
| |