RE: Review patches of user space kprobe

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

Thanks for your info. I go through the patches 1 by 1. Perhaps later patches already fix the problems in prior patches, so my comments might be incorrect. :)

Yanmin

>>-----Original Message-----
>>From: systemtap-owner@sourceware.org [mailto:systemtap-owner@sourceware.org] On Behalf Of Vara Prasad
>>Sent: 2005年12月22日 13:41
>>To: Zhang, Yanmin
>>Cc: prasanna@in.ibm.com; systemtap@sources.redhat.com; Keshavamurthy, Anil S; Mao, Bibo
>>Subject: Re: Review patches of user space kprobe
>>
>>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;
>>>>>+}
>>>>>+
>>>>>
>>>>>
>>>
>>>
>>>
>>>
>>