Re: [1/3 PATCH] Kprobes: User space probes support- base interface

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

On Mon, Mar 20, 2006 at 02:42:48AM -0800, Andrew Morton wrote:
> Prasanna S Panchamukhi <prasanna@in.ibm.com> wrote:
> >
> > This patch provides two interfaces to insert and remove
> > user space probes. Each probe is uniquely identified by
> > inode and offset within that executable/library file.
> > Insertion of a probe involves getting the code page for
> > a given offset, mapping it into the memory and then insert
> > the breakpoint at the given offset. Also the probe is added
> > to the uprobe_table hash list. A uprobe_module data strcture
> > is allocated for every probed application/library image on disk.
> > Removal of a probe involves getting the code page for a given
> > offset, mapping that page into the memory and then replacing
> > the breakpoint instruction with a the original opcode.
> > This patch also provides aggrigate probe handler feature,
> > where user can define multiple handlers per probe.
> > 
> > +/**
> > + * Finds a uprobe at the specified user-space address in the current task.
> > + * Points current_uprobe at that uprobe and returns the corresponding kprobe.
> > + */
> > +struct kprobe __kprobes *get_uprobe(void *addr)
> > +{
> > +	struct mm_struct *mm = current->mm;
> > +	struct vm_area_struct *vma;
> > +	struct inode *inode;
> > +	unsigned long offset;
> > +	struct kprobe *p, *kpr;
> > +	struct uprobe *uprobe;
> > +
> > +	vma = find_vma(mm, (unsigned long)addr);
> > +
> > +	BUG_ON(!vma);	/* this should not happen, not in our memory map */
> > +
> > +	offset = (unsigned long)addr - (vma->vm_start +
> > +						(vma->vm_pgoff << PAGE_SHIFT));
> > +	if (!vma->vm_file)
> > +		return NULL;
> 
> All this appears to be happening without mmap_sem held?

Yes, I will make the changes to hold the mmap_sem.

> 
> > +/**
> > + * Wait for the page to be unlocked if someone else had locked it,
> > + * then map the page and insert or remove the breakpoint.
> > + */
> > +static int __kprobes map_uprobe_page(struct page *page, struct uprobe *uprobe,
> > +				     process_uprobe_func_t process_kprobe_user)
> > +{
> > +	int ret = 0;
> > +	kprobe_opcode_t *uprobe_address;
> > +
> > +	if (!page)
> > +		return -EINVAL; /* TODO: more suitable errno */
> > +
> > +	wait_on_page_locked(page);
> > +	/* could probably retry readpage here. */
> > +	if (!PageUptodate(page))
> > +		return -EINVAL; /* TODO: more suitable errno */
> > +
> > +	lock_page(page);
> 
> That's a lot of fuss and might be racy with truncate.
> 
> Why not just run lock_page()?

Yes, I will do that.

> 
> > +	uprobe_address = (kprobe_opcode_t *)kmap(page);
> > +	uprobe_address = (kprobe_opcode_t *)((unsigned long)uprobe_address +
> > +						(uprobe->offset & ~PAGE_MASK));
> > +	ret = (*process_kprobe_user)(uprobe, uprobe_address);
> > +	kunmap(page);
> 
> kmap_atomic() is preferred.
> 

Agreed.

> > +/**
> > + * Gets exclusive write access to the given inode to ensure that the file
> > + * on which probes are currently applied does not change. Use the function,
> > + * deny_write_access_to_inode() we added in fs/namei.c.
> > + */
> > +static inline int ex_write_lock(struct inode *inode)
> > +{
> > +	return deny_write_access_to_inode(inode);
> > +}
> 
> hm, this code likes to poke around in VFS internals.  It would be nice to
> have an overall description of what it's trying to do, why and how.

ok, I should probably separate VFS changes in a different patch with
proper comments. However this is required to ensure that the
executable associated with this inode on which probes are inserted
does not change. deny_write_access_to_inode() just decrements the
inode usage count to -1.

> 
> > +/**
> > + * unregister_uprobe: Disarms the probe, removes the uprobe
> > + * pointers from the hash list and unhooks readpage routines.
> > + */
> > +void __kprobes unregister_uprobe(struct uprobe *uprobe)
> > +{
> > +	struct address_space *mapping;
> > +	struct uprobe_module *umodule;
> > +	struct page *page;
> > +	unsigned long flags;
> > +	int ret = 0;
> > +
> > +	if (!uprobe->inode)
> > +		return;
> > +
> > +	mapping = uprobe->inode->i_mapping;
> > +
> > +	page = find_get_page(mapping, uprobe->offset >> PAGE_CACHE_SHIFT);
> > +
> > +	spin_lock_irqsave(&uprobe_lock, flags);
> > +	ret = remove_uprobe(uprobe);
> > +	spin_unlock_irqrestore(&uprobe_lock, flags);
> > +
> > +	mutex_lock(&uprobe_mutex);
> > +	if (!(umodule = get_module_by_inode(uprobe->inode)))
> > +		goto out;
> > +
> > +	hlist_del(&uprobe->ulist);
> > +	if (hlist_empty(&umodule->ulist_head)) {
> > +		list_del(&umodule->mlist);
> > +		ex_write_unlock(uprobe->inode);
> > +		path_release(&umodule->nd);
> > +		kfree(umodule);
> > +	}
> > +
> > +out:
> > +	mutex_unlock(&uprobe_mutex);
> > +	if (ret)
> > +		ret = map_uprobe_page(page, uprobe, remove_kprobe_user);
> > +
> > +	if (ret == -EINVAL)
> > +		return;
> > +	/*
> > +	 * TODO: unregister_uprobe should not fail, need to handle
> > +	 * if it fails.
> > +	 */
> > +	flush_vma(mapping, page, uprobe);
> > +
> > +	if (page)
> > +		page_cache_release(page);
> > +}
> 
> That's some pretty awkward coding.  Buggy too.  It doesn't drop the
> refcount on the page if map_uprobe_page() returned -EINVAL because it's
> assuming that EINVAL meant "there was no page".  But there are multiple
> reasons for map_uprobe_page() to return -EINVAL.  If that page isn't
> uptodate, we leak a ref.
> 
> This function should be doing the checking for a find_get_page() failure,
> not map_uprobe_page().

Yes, that is buggy, will fix.


Thanks
Prasanna


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