RE: Review patches of user space kprobe

["Zhang, Yanmin" <yanmin dot zhang at intel dot com>]

General question:
1) How to insert an uprobe at anonymous page (VMA)? I think there are 2 cases related to the question.
	a) Many applications execute codes produced themselves, such like JIT (Just-In-Time) of JVM.
	b) Some executables include TEXTREL section. When they are loaded into memory and linked dynamically, the text section might be changed, and kernel will do a Copy-On-Write to create a new anonymous page and map the new page to the process address space. So after the process starts, we couldn't insert uprobe on its copied pages.
Should a new interface be added to support it? The parameters could be process id and offset in the process address space. Of course, it could be an enhancement and implemented later.

3) Can function register_userspace_probe do not call register_kprobe? I think it's not necessary. It's just my feeling. It's up to you to make decision. :)

2) Function get_inode_ops should take care of errors and its caller, register_userspace_probe should check if the return value of get_inode_ops is IS_ERR. If so, the error code should be returned instead of a hard-coded -ENOSYS.

Other more comments against patch 1/3 are inline below.

Yanmin

>>-----Original Message-----
>>From: systemtap-owner@sourceware.org [mailto:systemtap-owner@sourceware.org] On Behalf Of Zhang, Yanmin
>>Sent: 2005年12月21日 16:30
>>To: prasanna@in.ibm.com
>>>>+static struct vm_area_struct *find_get_vma(unsigned long offset,
>>>>+					   struct address_space
>>*mapping)
>>>>+{
>>>>+	struct vm_area_struct *vma = NULL;
>>>>+	struct prio_tree_iter iter;
>>>>+	struct prio_tree_root *head = &mapping->i_mmap;
>>>>+	struct mm_struct *mm;
>>>>+	unsigned long start, end;
>>>>+
>>>>+	spin_lock(&mapping->i_mmap_lock);
>>>>+	vma_prio_tree_foreach(vma, &iter, head, offset, offset) {
>>>>+		mm = vma->vm_mm;
>>>>+		spin_lock(&mm->page_table_lock);
>>>>+		start = vma->vm_start - (vma->vm_pgoff << PAGE_SHIFT);
>>>>+		end = vma->vm_end - (vma->vm_pgoff << PAGE_SHIFT);
>>>>+		spin_unlock(&mm->page_table_lock);
>>>>+		if ((start + offset) < end) {
>>>>+			spin_unlock(&mapping->i_mmap_lock);
>>>>+			return vma;
It's not safe to return vma without lock. There is a race condition. If vma is released by another thread, kernel might be crazy when this thread tries to access it later.

If the page is mapped to many vma in different processes, function find_get_vma just returns one vma. It's not enough.

I'd like to suggest to do the flush_icache in the vma_prio_tree_foreach loop.


>>>>+		}
>>>>+	}
>>>>+	spin_unlock(&mapping->i_mmap_lock);
>>>>+	return NULL;
>>>>+}