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

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

I just read the patch 1, and reply this response firstly. :)

>>-----Original Message-----
>>From: systemtap-owner@sourceware.org [mailto:systemtap-owner@sourceware.org] On Behalf Of Prasanna S Panchamukhi
>>Sent: 2006年1月25日 19:47
>>To: Zhang, Yanmin
>>Cc: systemtap@sources.redhat.com; Keshavamurthy, Anil S; Mao, Bibo
>>Subject: Re: Patch [1/3] Userspace probes new interfaces
>>
>>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?
Maybe I misunderstand your idea. Do you mean the path need to be locked, including mnt and dentry?
 If not including mnt, I think we could just do a dget(nd->dentry). I need double-check it.


>>
>>> >>	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.
I know get_kprobe_user uses it, but should the uprobe implementation assign an appropriate value to p.kp.addr? It's not good to ask the caller of register_uprobe to do so because the page might be not in memory or not been mapped into the address space of the process.



>>
>>> >>+	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).
[YM] Just before lookup_pathname() again, if the inode is released. So I suggest to delete this function and in its caller, we could call lookup_patchname once, does some check, then copy the temp nameidata to uprobe_module->nd if necessary. There will be no race.



>>
>>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.
[YM] You are right.


>>
>>> >>+		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.
[YM] Yes. I still suggest to assign it in uprobe implementation. A fixed address in user space is enough.