Re: Review patches of user space kprobe

[Vara Prasad <prasadav@us.ibm.com>]

Hi Yanmin,

I appreciate your comments. Prasanna the author of the patch is on 
vacation until the month end, he will address your comments once he is 
back in Jan. Please feel free to finish the review of the 3rd patch as 
well in the mean time.

Thanks,
Vara Prasad

Zhang, Yanmin wrote:

>Below inline are the comments for patch 2/3.
>
>Yanmin
>
>  
>
>>>Signed-of-by: Prasanna S Panchamukhi <prasanna@in.ibm.com>
>>>
>>>
>>>---
>>>
>>>linux-2.6.13-prasanna/include/linux/kprobes.h |    2 
>>>linux-2.6.13-prasanna/kernel/kprobes.c        |  113
>>>      
>>>
>++++++++++++++++++++++++++
>  
>
>>>2 files changed, 115 insertions(+)
>>>
>>>diff -puN kernel/kprobes.c~kprobes_userspace_probes-readpages
>>>      
>>>
>kernel/kprobes.c
>  
>
>>>--- linux-2.6.13/kernel/kprobes.c~kprobes_userspace_probes-readpages
>>>      
>>>
>2005-09-14 11:01:18.495513696 +0530
>  
>
>>>+++ linux-2.6.13-prasanna/kernel/kprobes.c	2005-09-14
>>>      
>>>
>11:01:18.550505336 +0530
>  
>
>>>@@ -652,6 +652,109 @@ static struct uprobe_module *get_module_
>>>}
>>>
>>>/*
>>>+ * Check if the given offset lies within given page range.
>>>+ */
>>>+static int find_page_probe(unsigned long offset, unsigned long
>>>      
>>>
>page_start)
>  
>
>>>+{
>>>+      unsigned long page_end = page_start + PAGE_SIZE;
>>>+	if (offset >= page_start && offset < page_end)
>>>+		return 1;
>>>+	return 0;
>>>+}
>>>+
>>>+/*
>>>+ * This function handles the readpages of all modules that have
>>>      
>>>
>active probes
>  
>
>>>+ * in them. Here, we first call the original readpages() of this
>>>+ * inode / address_space to actually read the pages into memory.
>>>      
>>>
>Then, we will
>  
>
>>>+ * insert all the probes that are specified in this pages before
>>>      
>>>
>returning.
>  
>
>>>+ */
>>>+static int up_readpages(struct file *file, struct address_space
>>>      
>>>
>*mapping,
>  
>
>>>+			struct list_head *pages, unsigned nr_pages)
>>>+{
>>>+	int retval = 0;
>>>+	struct page *page;
>>>+	struct uprobe_module *m;
>>>+	struct uprobe *up = NULL;
>>>+	struct hlist_node *node;
>>>+
>>>+	m = get_module_by_inode(file->f_dentry->d_inode);
>>>      
>>>
>There is a race condition between unregister_userspace_probe and here.
>If a thread jumps to the beginning of function up_readpages while
>another thread calls unregister_userspace_probe to delete the um, the
>first thread might return error.
>
>
>  
>
>>>+	if (!m) {
>>>+		printk("up_readpages: major problem. we don't  \
>>>+						have mod for this
>>>      
>>>
>!!!\n");
>  
>
>>>+		return -EINVAL;
>>>+	}
>>>+
>>>+	/* call original readpages() */
>>>+	retval = m->ori_a_ops->readpages(file, mapping, pages,
>>>      
>>>
>nr_pages);
>  
>
>>>+	if (retval >= 0) {
>>>+		hlist_for_each_entry(up, node, &m->ulist_head, ulist) {
>>>+			/*
>>>+			 * TODO: Walk through readpages page list and
>>>      
>>>
>get
>  
>
>>>+			 * pages with probes instead of find_get_page().
>>>+			 */
>>>+			if ((page = find_get_page(mapping,
>>>+				up->offset >> PAGE_CACHE_SHIFT)) !=
>>>      
>>>
>NULL) {
>  
>
>>>+				if (find_page_probe
>>>+				    (up->offset >> PAGE_CACHE_SHIFT,
>>>+				     page->index << PAGE_CACHE_SHIFT)) {
>>>+					up->page = page;
>>>+					if (!map_uprobe_page(up, 0)) {
>>>+						lock_page(up->page);
>>>      
>>>
>The first patch doesn't do lock_page before calling insert_probe_page.
>Why does this patch do so? It's inconsistent.
>
>
>  
>
>>>+						insert_probe_page(up);
>>>+						unmap_uprobe_page(up);
>>>+						unlock_page(up->page);
>>>+					}
>>>+				}
>>>+				page_cache_release(up->page);
>>>+			}
>>>+		}
>>>+	}
>>>+	return retval;
>>>+}
>>>+
>>>+/*
>>>+ * This function handles the readpage of all modules that have active
>>>      
>>>
>probes
>  
>
>>>+ * in them. Here, we first call the original readpage() of this
>>>+ * inode / address_space to actually read the page into memory. Then,
>>>      
>>>
>we will
>  
>
>>>+ * insert all the probes that are specified in this page before
>>>      
>>>
>returning.
>  
>
>>>+ */
>>>+int up_readpage(struct file *file, struct page *page)
>>>+{
>>>+	int retval = 0;
>>>+	struct uprobe_module *m;
>>>+	struct uprobe *up = NULL;
>>>+	int kprobe_page_mapped = 0;
>>>+	struct hlist_node *node;
>>>+
>>>+	m = get_module_by_inode(file->f_dentry->d_inode);
>>>      
>>>
>The same race condition like above function.
>
>
>  
>
>>>+	if (!m) {
>>>+		printk("up_readpage: major problem. we don't have mod
>>>      
>>>
>for this !!!\n");
>  
>
>>>+		return -EINVAL;
>>>+	}
>>>+
>>>+	/* call original readpage() */
>>>+	retval = m->ori_a_ops->readpage(file, page);
>>>+	if (retval >= 0) {
>>>+		hlist_for_each_entry(up, node, &m->ulist_head, ulist) {
>>>+			if (find_page_probe (up->offset >>
>>>      
>>>
>PAGE_CACHE_SHIFT,
>  
>
>>>+					page->index <<
>>>      
>>>
>PAGE_CACHE_SHIFT)) {
>  
>
>>>+				up->page = page;
>>>+				if (!map_uprobe_page(up,
>>>      
>>>
>kprobe_page_mapped)) {
>  
>
>>>+					lock_page(up->page);
>>>      
>>>
>Same inconsistent issue.
>
>
>  
>
>>>+					kprobe_page_mapped = 1;
>>>+					retval = insert_probe_page(up);
>>>+				}
>>>+			}
>>>+		}
>>>+		if (kprobe_page_mapped) {
>>>      
>>>
>The logic here is incorrect. If there are many uprobes at the same page,
>up just points to the last one. How about others? 
>
>
>
>  
>
>>>+			unmap_uprobe_page(up);
>>>+			unlock_page(up->page);
>>>+		}
>>>+	}
>>>+	return retval;
>>>+}
>>>+
>>>      
>>>
>
>
>  
>