linux-kernel - Re: [RFC][PATCH 0/1] kmemleak: Fix false positive with alias

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: <1284729255.26542.39.camel@e102109-lin.cambridge.arm.com>
Date:	Fri, 17 Sep 2010 14:14:15 +0100
From:	Catalin Marinas <catalin.marinas@....com>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	Hiroshi DOYU <Hiroshi.DOYU@...ia.com>,
	linux-kernel@...r.kernel.org, ext-phil.2.carmody@...ia.com,
	linux-omap@...r.kernel.org
Subject: Re: [RFC][PATCH 0/1] kmemleak: Fix false positive with alias

On Mon, 2010-08-30 at 13:30 -0700, Paul E. McKenney wrote:
> On Fri, Aug 27, 2010 at 09:12:24AM +0300, Hiroshi DOYU wrote:
> > > +static struct kmemleak_object *find_and_get_object(unsigned long ptr, int alias)
> > > +{
> > > +	struct kmemleak_object *object;
> > > +
> > > +	rcu_read_lock();
> > > +	object = __find_and_get_object(ptr, alias);
> > > +	rcu_read_unlock();
> > > +
> > > +	return object;
> > > +}
> > > +
> > > +static struct kmemleak_object *__find_and_get_alias(unsigned long ptr, int alias)
> > > +{
> > > +	struct kmemleak_object *object = NULL;
> > > +	struct kmemleak_alias *ao = NULL;
> > > +	struct prio_tree_node *node;
> > > +	struct prio_tree_iter iter;
> > > +	unsigned long flags;
> > > +
> > > +	read_lock_irqsave(&kmemleak_lock, flags);
> 
> If we hold this readlock, how is RCU helping us?  Or are there updates
> that do not write-hold kmemleak_lock?

These kind of constructs are already in the existing kmemleak code.
Kmemleak uses the slab allocator itself but it also gets called from the
slab allocator for other memory allocation/freeing. To avoid a race on
the kfree path, kmemleak objects are freed later via he RCU.

The read_lock is used for the prio_tree access/modifications but the
rcu_lock protects an additional get_object() call.

> > > +	prio_tree_iter_init(&iter, &alias_tree_root, ptr, ptr);
> > > +	node = prio_tree_next(&iter);
> > > +	if (node) {
> > > +		ao = prio_tree_entry(node, struct kmemleak_alias, tree_node);
> > > +		if (!alias && to_address(ao) != ptr) {
> > > +			kmemleak_warn("Found object by alias");
> > > +			ao = NULL;
> > > +		}
> > > +	}
> > > +
> > > +	read_unlock_irqrestore(&kmemleak_lock, flags);
> > > +
> > > +	if (ao && get_object(ao->object))
> > > +		object = ao->object;
> > > +
> > > +	return object;
> > > +}
[...]
> > > +static void __delete_alias_object(struct kmemleak_object *object)
> > > +{
> > > +	prio_tree_remove(&alias_tree_root, &object->alias->tree_node);
> > > +	list_del_rcu(&object->alias->alias_list);
> 
> Don't we need an RCU grace period here, based on either synchronize_rcu()
> or call_rcu()?  Perhaps calling free_object_rcu(), though perhaps it frees
> up more than you would like.

You may be right here. The kmemleak objects (not the aliases introduced
by this patch) are freed later by the put_object() call via a
call_rcu().

Anyway, I don't think its worth pursuing this approach for handling
address aliases (e.g. virt_to_phys) because of the performance hit
Hiroshi is seeing.

-- 
Catalin

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