linux-kernel - Re: [PATCH v7 3.2-rc2 4/30] uprobes: Define hooks for mmap/munmap.

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: <1322567326.2921.226.camel@twins>
Date:	Tue, 29 Nov 2011 12:48:46 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux-mm <linux-mm@...ck.org>, Ingo Molnar <mingo@...e.hu>,
	Andi Kleen <andi@...stfloor.org>,
	Christoph Hellwig <hch@...radead.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Roland McGrath <roland@...k.frob.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Arnaldo Carvalho de Melo <acme@...radead.org>,
	Anton Arapov <anton@...hat.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
	Stephen Wilson <wilsons@...rt.ca>, tulasidhard@...il.com
Subject: Re: [PATCH v7 3.2-rc2 4/30] uprobes: Define hooks for mmap/munmap.

On Tue, 2011-11-29 at 14:03 +0530, Srikar Dronamraju wrote:


> install_breakpoints cannot have !consumers to be true when called from
> register_uprobe. (Since unregister_uprobe() which does the removal of
> consumer cannot race with register_uprobe().)

Right, that's the easy case ;-)

> Now lets consider mmap_uprobe() being called from vm_adjust(), the
> preceding unmap_uprobe() has already decremented the count but left the
> count intact.
> 
> if consumers is NULL, unregister_uprobes() has kicked already in, so
> there is no point in inserting the probe, Hence we return EEXIST. The
> following unregister_uprobe() (or the munmap_uprobe() which might race
> before unregister_uprobe) is also going to decrement the count.  So we
> have a case where the same breakpoint is accounted as removed twice. To
> offset this, we pretend as if the breakpoint is around by incrementing
> the count.

There's 2 main cases, 
	A) vma_adjust() vs unregister_uprobe() and 
	B) mmap() vs unregister_uprobe().

The result of A should be -1 reference in total, since we're removing
the one probe. The result of B should be 0 since we're removing the
probe and we shouldn't be installing new ones.

A1)
	vma_adjust()
	  munmap_uprobe()
				unregister_uprobe()
	  mmap_uprobe()
				  delete_uprobe()


	munmap will to -1, mmap will do +1, __unregister_uprobe() which is
serialized against vma_adjust() will do -1 on either the old or new vma,
resulting in a grand total of: -1+1-1=-1, OK

A2) breakpoint is in old, not in new, again two cases:

A2a) __unregister_uprobe() sees old

	munmap -1, __unregister_uprobe -1, mmap 0: -2 FAIL

A2b) __unregister_uprobe() sees new

	munmap -1, __unregister_uprobe 0, mmap 0: -1 OK

A3) breakpoint is in new, not in old, again two cases:

A3a) __unregister_uprobe() sees old

	munmap 0, __unregister_uprobe 0, mmap: 1: 1 FAIL

A3b) __unregister_uprobe() seed new

	munmap 0, __unregister_uprobe -1, mmap: 1: 0 FAIL

B1)
				unregister_uprobe()
	mmap()
	  mmap_uprobe()
				  __unregister_uprobe()
				  delete_uprobe()

	mmap +1, __unregister_uprobe() -1: 0 OK

B2)
				unregister_uprobe()
	mmap()
				  __unregister_uprobe()
	  mmap_uprobe()
				  delete_uprobe()

	mmap +1, __unregister_uprobe() 0: +1 FAIL


> Would it help if I add an extra check in mmap_uprobe?
> 
> int mmap_uprobe(...) {
> ....
> 	       ret = install_breakpoint(vma->vm_mm, uprobe);
> 	       if (ret == -EEXIST) {
> 			if (!read_opcode(vma->vm_mm, vaddr, &opcode) &&
> 					(opcode == UPROBES_BKPT_INSN))
> 			       atomic_inc(&vma->vm_mm->mm_uprobes_count);
> 		       ret = 0;
> 	       } 
> ....
> }

> The extra read_opcode check will tell us if the breakpoint is still
> around and then only increment the count. (As in it will distinguish if
> the mmap_uprobe is from vm_adjust).

No, I don't see that fixing A2a for example.

Could be I confused myself above, but like said, this stuff hurt brain.

It might just be easiest not to optimize munmap and leave fancy stuff
for later.
--
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/