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: <1322492384.2921.143.camel@twins>
Date:	Mon, 28 Nov 2011 15:59:44 +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 Thu, 2011-11-24 at 19:17 +0530, Srikar Dronamraju wrote:
> * Peter Zijlstra <peterz@...radead.org> [2011-11-23 19:10:12]:
> 
> > On Fri, 2011-11-18 at 16:37 +0530, Srikar Dronamraju wrote:
> > > +                       ret = install_breakpoint(vma->vm_mm, uprobe);
> > > +                       if (ret == -EEXIST) {
> > > +                               atomic_inc(&vma->vm_mm->mm_uprobes_count);
> > > +                               ret = 0;
> > > +                       } 
> > 
> > Aren't you double counting that probe position here? The one that raced
> > you to inserting it will also have incremented that counter, no?
> > 
> 
> No we arent.
> Because register_uprobe can never race with mmap_uprobe and register
> before mmap_uprobe registers .(Once we start mmap_region,
> register_uprobe waits for the read_lock of mmap_sem.)
> 
> And we badly need this for mmap_uprobe case.  Because when we do mremap,
> or vma_adjust(), we do a munmap_uprobe() followed by mmap_uprobe() which
> would have decremented the count but not removed it. So when we do a
> mmap_uprobe, we need to increment the count. 

Ok, so I didn't parse that properly last time around.. but it still
doesn't make sense, why would munmap_uprobe() decrement the count but
not uninstall the probe?

install_breakpoint() returning -EEXIST on two different conditions
doesn't help either.

So what I think you're doing is that you're optimizing the unmap case
since the memory is going to be thrown out fixing up the instruction is
a waste of time, but this leads to the asymmetry observed above. But you
fail to mention this in both the changelog or a comment near that
-EEXIST branch in mmap_uprobe.

Worse, you don't explain how the other -EEXIST (!consumers) thing
interacts here, and I just gave up trying to figure that out since it
made my head hurt.

Also, your whole series of patches is still utter crap, the splitup
doesn't work at all, I need to constantly search back and forth between
patches in order to figure out wtf is happening, and your changelogs
only seem to add confusion if anything at all.

Also, you seem to have stuck a whole bunch of random patches at the end
that fix various things without folding them back in to make the series
saner/smaller.

I've now reverted to simply applying all patches and reading the end
result and using git-blame to figure out what patch something came
from :-(

--
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/