Re: Patch [1/3] Userspace probes new interfaces

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

Hi Yanmin,

Thanks for reviewing and giving your feedback.

> >>	/* to hold path/dentry etc. */
> >>	struct nameidata nd;
> nameidata is too big. I still think a pointer to dentry is enough and it's very to use dentry.
> 

Given the current usage of dentry to lock down the path, could you
please elaborate on how to implement your suggestion?

> >>	p.kp.addr = (kprobe_opcode_t *)0x080484d4;
> Why should we assign a value to p.kp.addr in the example?

Please check get_kprobe_user(), p.kp.addr it is used to distinguish 
between kernel and user space probes.

> >>+	spin_lock(&mm->page_table_lock);
> >>+	vma = find_vma(mm, (unsigned long)addr);
> Find_vma is protected by mm->mmap_sem instead of mm->page_table_lock.

yes, this will be taken care in the next release of patches.

> >>+int __kprobes insert_kprobe_user(struct uprobe *uprobe, unsigned long *address,
> >>+				struct page *page, struct vm_area_struct *vma)
> Parameter vma is not needed because flush_vma will do the flush for all vma which maps the page.
> process_uprobe_func_t could also delete parameter vma. Same thing of function remove_kprobe_user.
> 
> Another reason to delete the parameter is it might introduce a race between find_get_vma and map_uprobe_page. Vma might be changed or release just after returning from find_get_vma. 

This will be taken care in the next release of patches.

> >>+static struct vm_area_struct __kprobes *find_get_vma(struct uprobe *uprobe,
> >>+			struct page *page, struct address_space *mapping)
> I would like to suggest you to delete this function, because it might introduce race.

This will be taken care.

> >>+	vma_prio_tree_foreach(vma, &iter, head, offset, offset) {
> >>+		mm = vma->vm_mm;
> >>+
> >>+		spin_lock(&mm->page_table_lock);
> vma->vm_start and vma->vm_end are protected by mm->mmap_sem. So the lock becomes complicated because spinlock and sema are used together.

I will check if this works and incorporate the changes in the next
release of patches.

> >>+		spin_lock(&mm->page_table_lock);
> Like above, vma->vm_start and vma->vm_end are protected by mm->mmap_sem.

Yes, this will be done.

> >>+static int __kprobes get_inode_ops(struct uprobe *uprobe,
> >>+				   struct uprobe_module *umodule)
> The function looks incomplete and unclean.

Sorry for the confusion here, I could not split this routine completely,
Please apply the second patch(readpage hooks) to get a complete picture.

> >>+ */
> >>+static inline int get_uprobe_inode(const char *pathname, struct inode **inode)
> I suggest to delete this function because there is a race condition. Inode might be released by another

There are two scenerios we must consider here,
1. Probes might be already existing on this application(inode).
2. Probes might not be existing on this application.

In 1st case, we already have decremented the writecount 
(inode->i_writecount) so that inode does not go away.

In the secound case we lookup_pathname() again and then decrement the
writecount (inode->i_writecount).

I dont see race conditions here. Please apply the second patch(readpage
hooks) to get the complete understanding.

> thread just this thread returns from get_uprobe_inode. This function is simple and could be moved
> to its caller, register_uprobe.

yes, this can be done.

> >>+	if ((error = path_lookup(pathname, LOOKUP_FOLLOW, &nd))) {
> >>+		path_release(&nd);
> If error, still need call patch_release?

yes, path_release() is not required here.

> >>+		arch_disarm_uprobe(p, (kprobe_opcode_t *)address);
> >>+		flush_icache_user_range(vma, page, (unsigned long)address,
> >>+						sizeof(kprobe_opcode_t));
> flush_icache_user_range is not needed here because later flush_vma would do so.

This will be taken care in the next release.

> >>+	if (cleanup_p) {
> >>+		if (p != old_p) {
> >>+			list_del_rcu(&p->list);
> Should this list_del_rcu be moved before synchronize_sched? Suggest to move it to 

It does not have to be before synchronize_sched(), since we are just
unlinking the last kprobe structure on the aggregate probe list and the
aggregate probe itself is no longer on the kprobes hash list.

> >>+		error = path_lookup(uprobe->pathname, LOOKUP_FOLLOW,
> >>+								&umodule->nd);
> >>+		if (error) {
> >>+			path_release(&umodule->nd);
> If error, still need call patch_release? 

Yes, path_release() is not required here.

> >>+}
> 1) When to assign a value to uprobe->kp->addr?

Please check get_kprobe_user(), p.kp.addr it is used to distinguish 
between kernel and user space probes.

> 2) As for the flush_icache when register/unregister uprobe, I am still hesitating if we should flush icache just for the current cpu or all the cpus which run in the address spaces (vma->mm) of all related vma mapping the uprobed same page. Could guys discuss it on the conference call?  

I am sure if flush_icache() flush icaches for all the cpus running in the same address
spaces. Surely we will discuss this on the conference call.

I am planning to the release the take-2 of this patches soon.

Thanks
Prasanna

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