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: "Vara Prasad" <prasadav at us dot ibm dot com>
- Cc: <prasanna at in dot ibm dot com>, <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 14:06:54 +0800
- Subject: RE: Review patches of user space kprobe
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;
>>>>>+}
>>>>>+
>>>>>
>>>>>
>>>
>>>
>>>
>>>
>>