Re: Review patches of user space kprobe

[Prasanna S Panchamukhi <prasanna at in dot ibm dot com>]

Yanmin,

Thanks for your comments, Please see my comments inline below.

>>>		/*inode of the application */
>>>		struct inode *inode;
>How about using a pointer to uprobe_module to locate the inode? Although
>the storage space is same, it's better to keep all the same info of
>uprobes in uprobe_module. Uprobes just need keep its own specific info.

inode and offset combination uniquely identifies individual user space
probe, hence when a probe is hit, combination inode and offset is used to
identify each probe. Keeping inode pointer in struct uprobe clearly 
reflects this concept.


>>		/*offset within the page */
>This comment is incorrect. Functions, such like
>register_userspace_probe, use it as the offset from the beginning of the
>file, not a page of the file. Right? Although your example uses it as an
>offset of page. Frank's reply said this item could be deleted. I don't
>think he is correct because uprobe->kp.addr is used when calling
>register_kprobe.

Next patch release will take care of this, offset is hidden from the user and user need
not initialize it.


>>		unsigned long offset;=20
>>		/*page containing probes */
>>		struct page *page;
>>		/* virtual memory area containing the probes */
>>		struct vm_area_struct *vma;
>Item page and vma are used temporarily. They are not used when the probe
>is hit. To shrink struct uprobe, they could be deleted. A new super
>struct could be defined to include struct uprobe, or always to search
>them when they are needed. Application might be inserted huge uprobes.
>It's better to keep the struct smaller.

If vma's are temporarily defined, then readpage hooks should walk 
through the vma list to get the vma. To reduce the time spent in
readpage/readpages hooks in fastpath, we store it while registeration.



>>	};
>>
>>	struct uprobe_module {
>>		struct hlist_head ulist_head;
>>		/* hlist head containing list of probes within this
module */=20
>>		struct list_head mlist;
>>		/* list of uprobes_modules */
>>		struct inode *inode;
>Item inode could be deleted. Use uprobe_module->nd.dentry->d_inode.

This will be taken care in the next patch release.


>>		/* inode of the application on which probes are
implanted */
>>		struct nameidata nd;
>>		/* to hold path/dentry etc. */
>Struct nameidata looks a little big. A pointer to dentry might be
>enough.

nameidata is allocated per module/application and not for every probe.
At present the reference count to the mount point is held until 
the probes is unregistered. I will relook at this issue.
 
>>+	if (!kernel_text_address((unsigned long)p->addr))
>>+		addr =3D insert_kprobe_user(p);
>After calling insert_kprobe_user, p->addr is still equal to the return
>value of kmap_atomic. So when the uprobe is hit, how to find it from the
>hash table? 

Here it does not return just the mapped address, but inode * offset as the address. 
Each probe is uniquely identified by the combination of inode and offset, kprobe_handler
does the same thing.

>And I didn't see kprobe_handler is changed to call
>get_kprobe_user if uprobe is hit.

kprobe_handler() calls get_uprobe_at() when the probe is hit.

>>+ */
>>+static int map_uprobe_page(struct uprobe *up, int mapped)
>The second parameter seems useless.

Its being used in the readpage patch. 



>>+{
>>+{
>>+	up->kp.addr =3D (kprobe_opcode_t *) ((unsigned long)up->kp.addr +
>>+				 (unsigned long)(up->offset &
~PAGE_MASK));
Does it guarantee later register_kprobe could distinguish it's a user
probe? Perhaps it's better to assign a fixed address in user space, such
like 0.

Yes,register_kprobe() distinguishes user space probes. I could not 
understand what you mean by "assign a fixed address in user space"

>>+	up->kp.opcode =3D 0;
>It's not necessary to initiate kp.opcode as zero here.

This will be taken care in the next patch release.


>>+	list_for_each_entry(um, &uprobe_module_list, mlist) {
>There is no any lock to protect uprobe_module_list. It's not safe under
>multi-task environment.

This will be fixed in the next release .


>>+		path_release(&nd);
>>+		printk("Failed to lookup the path\n");
>>+		return NULL;
>>+	}
>>+	inode =3D nd.dentry->d_inode;
>>+	path_release(&nd);
>>+	return get_module_by_inode(inode);

>It's better to move path_release after calling get_module_by_inode in
>case of potential race condition.

Later in the patch, we increment the writecount on the inode. get_module_by_inode
just walks the uprobe_module list.

>>+
>>+	um =3D kcalloc(1, sizeof(struct uprobe_module), GFP_KERNEL);
>If kcalloc returns error, it should be checked here.

This will be taken care in the next patch release.

>>+	goto out;
>>+
>>+err:
>>+	kfree(um);
>It looks like you forgot "return NULL" here.

It is taken care off now.


>>+	mapping =3D up->inode->i_mapping;
>>+	up->vma =3D find_get_vma(up->offset, mapping);
>>+	up->page =3D find_get_page(mapping, (up->offset >>
PAGE_CACHE_SHIFT));
>>+
>>+	if (!map_uprobe_page(up, 0)) {
>If map_uprobe_page(up,0) !=3D 0, register_userspace_probe still returns 0
>meaning success register. It's incorrect and might confuse users.
>How about change it to:
>if (!(error=3Dmap_uprobe_page(up, 0))) {

Proper return of error value in case of failure is taken care 
in next patch release.

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