linux-kernel - Re: [PATCH v2 2.6.38-rc8-tip 5/20] 5: Uprobes: register/unregister probes.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.00.1103151439400.2787@localhost6.localdomain6>
Date:	Tue, 15 Mar 2011 15:28:04 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
cc:	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...e.hu>,
	Steven Rostedt <rostedt@...dmis.org>,
	Linux-mm <linux-mm@...ck.org>,
	Arnaldo Carvalho de Melo <acme@...radead.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Christoph Hellwig <hch@...radead.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Oleg Nesterov <oleg@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>,
	SystemTap <systemtap@...rces.redhat.com>,
	Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
	Roland McGrath <roland@...k.frob.com>,
	Andi Kleen <andi@...stfloor.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH v2 2.6.38-rc8-tip 5/20] 5: Uprobes: register/unregister
 probes.

On Mon, 14 Mar 2011, Srikar Dronamraju wrote:
> +/* Returns 0 if it can install one probe */
> +int register_uprobe(struct inode *inode, loff_t offset,
> +				struct uprobe_consumer *consumer)
> +{
> +	struct prio_tree_iter iter;
> +	struct list_head tmp_list;
> +	struct address_space *mapping;
> +	struct mm_struct *mm, *tmpmm;
> +	struct vm_area_struct *vma;
> +	struct uprobe *uprobe;
> +	int ret = -1;
> +
> +	if (!inode || !consumer || consumer->next)
> +		return -EINVAL;
> +	uprobe = uprobes_add(inode, offset);

Does uprobes_add() always succeed ?

> +	INIT_LIST_HEAD(&tmp_list);
> +	mapping = inode->i_mapping;
> +
> +	mutex_lock(&uprobes_mutex);
> +	if (uprobe->consumers) {
> +		ret = 0;
> +		goto consumers_add;
> +	}
> +
> +	spin_lock(&mapping->i_mmap_lock);
> +	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, 0, 0) {
> +		loff_t vaddr;
> +
> +		if (!atomic_inc_not_zero(&vma->vm_mm->mm_users))
> +			continue;
> +
> +		mm = vma->vm_mm;
> +		if (!valid_vma(vma)) {
> +			mmput(mm);
> +			continue;
> +		}
> +
> +		vaddr = vma->vm_start + offset;
> +		vaddr -= vma->vm_pgoff << PAGE_SHIFT;
> +		if (vaddr > ULONG_MAX) {
> +			/*
> +			 * We cannot have a virtual address that is
> +			 * greater than ULONG_MAX
> +			 */
> +			mmput(mm);
> +			continue;
> +		}
> +		mm->uprobes_vaddr = (unsigned long) vaddr;
> +		list_add(&mm->uprobes_list, &tmp_list);
> +	}
> +	spin_unlock(&mapping->i_mmap_lock);
> +
> +	if (list_empty(&tmp_list)) {
> +		ret = 0;
> +		goto consumers_add;
> +	}
> +	list_for_each_entry_safe(mm, tmpmm, &tmp_list, uprobes_list) {
> +		down_read(&mm->mmap_sem);
> +		if (!install_uprobe(mm, uprobe))
> +			ret = 0;

Installing it once is success ?

> +		list_del(&mm->uprobes_list);

Also the locking rules for mm->uprobes_list want to be
documented. They are completely non obvious.

> +		up_read(&mm->mmap_sem);
> +		mmput(mm);
> +	}
> +
> +consumers_add:
> +	add_consumer(uprobe, consumer);
> +	mutex_unlock(&uprobes_mutex);
> +	put_uprobe(uprobe);

Why do we drop the refcount here?

> +	return ret;
> +}

> +	/*
> +	 * There could be other threads that could be spinning on
> +	 * treelock; some of these threads could be interested in this
> +	 * uprobe.  Give these threads a chance to run.
> +	 */
> +	synchronize_sched();

This makes no sense at all. We are not holding treelock, we are about
to acquire it. Also what does it matter when they spin on treelock and
are interested in this uprobe. Either they find it before we remove it
or not. So why synchronize_sched()? I find the lifetime rules of
uprobe utterly confusing. Could you explain please ?

> +	spin_lock_irqsave(&treelock, flags);
> +	rb_erase(&uprobe->rb_node, &uprobes_tree);
> +	spin_unlock_irqrestore(&treelock, flags);
> +	iput(uprobe->inode);
> +
> +put_unlock:
> +	mutex_unlock(&uprobes_mutex);
> +	put_uprobe(uprobe);
> +}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/