This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
RE: Review patches of user space kprobe
- From: "Zhang, Yanmin" <yanmin dot zhang at intel dot com>
- To: <prasanna at in dot ibm dot com>
- Cc: <systemtap at sources dot redhat dot com>, "Keshavamurthy, Anil S" <anil dot s dot keshavamurthy at intel dot com>, "Mao, Bibo" <bibo dot mao at intel dot com>
- Date: Thu, 22 Dec 2005 13:34:01 +0800
- Subject: RE: Review patches of user space kprobe
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;
>>+}
>>+