linux-kernel - Re: [PATCH v5 3.1.0-rc4-tip 4/26] 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]

Date:	Tue, 27 Sep 2011 13:41:21 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc:	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>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Hugh Dickins <hughd@...gle.com>,
	Christoph Hellwig <hch@...radead.org>,
	Jonathan Corbet <corbet@....net>,
	Thomas Gleixner <tglx@...utronix.de>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Oleg Nesterov <oleg@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
	Roland McGrath <roland@...k.frob.com>,
	Andi Kleen <andi@...stfloor.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 3.1.0-rc4-tip 4/26]   uprobes: Define hooks for
 mmap/munmap.

On Mon, 2011-09-26 at 21:14 +0530, Srikar Dronamraju wrote:
> > Why not something like:
> > 
> > 
> > +static struct uprobe *__find_uprobe(struct inode * inode, loff_t offset,
> >                                       bool inode_only)
> > +{
> >         struct uprobe u = { .inode = inode, .offset = inode_only ? 0 : offset };
> > +       struct rb_node *n = uprobes_tree.rb_node;
> > +       struct uprobe *uprobe;
> >       struct uprobe *ret = NULL;
> > +       int match;
> > +
> > +       while (n) {
> > +               uprobe = rb_entry(n, struct uprobe, rb_node);
> > +               match = match_uprobe(&u, uprobe);
> > +               if (!match) {
> >                       if (!inode_only)
> >                              atomic_inc(&uprobe->ref);
> > +                       return uprobe;
> > +               }
> >               if (inode_only && uprobe->inode == inode)
> >                       ret = uprobe;
> > +               if (match < 0)
> > +                       n = n->rb_left;
> > +               else
> > +                       n = n->rb_right;
> > +
> > +       }
> >         return ret;
> > +}
> > 
> 
> I am not comfortable with this change.
> find_uprobe() was suppose to return back a uprobe if and only if
> the inode and offset match,

And it will, because find_uprobe() will never expose that third
argument.

>  However with your approach, we end up
> returning a uprobe that isnt matching and one that isnt refcounted.
> Moreover if even if we have a matching uprobe, we end up sending a
> unrefcounted uprobe back. 

Because the matching isn't the important part, you want to return the
leftmost node matching the specified inode. Also, in that case you
explicitly don't want the ref, since the first thing you do on the
call-site is drop the ref if there was a match. You don't care about
inode:0 in particular, you want a place to start iterating all of
inode:*.


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