RE: Review patches of user space kprobe

["Zhang, Yanmin" <yanmin.zhang@intel.com>]

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;
>>+}
>>+