Re: Review patches of user space kprobe

[Prasanna S Panchamukhi <prasanna@in.ibm.com>]

> >>+	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.

Some locking/semaphore can be used to avoid this. 

> >>+						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.
> 

Next patch release will take care of this.

> >>+	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.
> 

Some locking/semaphore can be used to avoid this. 
> 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? 
> 

readpage() routine reads one page at a time. we map the page one time and walk the 
probes list for this inode, insert all the probes within this page and then unmap it.

Thanks
Prasanna
-- 

Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Email: prasanna@in.ibm.com
Ph: 91-80-25044636