netdev - Re: [RFC PATCH v1 2/6] netlabel: Replace protocol/NetLabel linking with refrerence counts

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: <200808082211.32951.paul.moore@hp.com>
Date:	Fri, 8 Aug 2008 22:11:32 -0400
From:	Paul Moore <paul.moore@...com>
To:	paulmck@...ux.vnet.ibm.com
Cc:	selinux@...ho.nsa.gov, linux-security-module@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [RFC PATCH v1 2/6] netlabel: Replace protocol/NetLabel linking with refrerence counts

On Friday 08 August 2008 6:37:16 pm Paul E. McKenney wrote:
> On Fri, Aug 08, 2008 at 04:53:01PM -0400, Paul Moore wrote:
> >  struct cipso_v4_doi *cipso_v4_doi_getdef(u32 doi)
> >  {
> > -	return cipso_v4_doi_search(doi);
> > +	struct cipso_v4_doi *doi_def;
> > +
> > +	rcu_read_lock();
> > +	doi_def = cipso_v4_doi_search(doi);
> > +	if (doi_def)
>
> Suppose that the doi_def element is removed by some other CPU at
> this point.  The reference-count check would pass (so that the
> deletion function would decline to error out with -EBUSY), and the
> removal would proceed normally.  (Right?)
>
> So we then acquire the reference count on an element that will be
> freed after an RCU grace period, despite the fact that the reference
> count might still be held at that point.
>
> Or am I missing something?  (Wouldn't be a surprise, as it is not
> like I am familiar with this code.)

Hi Paul,

Thanks for taking a look, your point sounds reasonable to me.

> If I am correct, the usual resolution is to combine the reference
> count and the "valid" flag, so that a zero reference counter implies
> "not valid", allowing the atomic_inc() below to become
> atomic_inc_not_zero(), allowing you to simply return NULL should the
> race with removal be detected.  There are other approaches as well...

Combining the valid and refcount fields seems reasonable to me.  I took 
your advice and made the following changes (as well as they other 
changes to replace the valid check with atomic_read(refcount) > 0) ...

struct cipso_v4_doi *cipso_v4_doi_getdef(u32 doi)
{
	struct cipso_v4_doi *doi_def;

	rcu_read_lock();
	doi_def = cipso_v4_doi_search(doi);
	if (doi_def == NULL)
		goto doi_getdef_return;
	if (!atomic_inc_not_zero(&doi_def->refcount))
		doi_def = NULL;

doi_getdef_return:
	rcu_read_unlock();
	return doi_def;
}

int cipso_v4_doi_remove(u32 doi,
			struct netlbl_audit *audit_info,
			void (*callback) (struct rcu_head * head))
{
	struct cipso_v4_doi *doi_def;

	spin_lock(&cipso_v4_doi_list_lock);
	doi_def = cipso_v4_doi_search(doi);
	if (doi_def == NULL) {
		spin_unlock(&cipso_v4_doi_list_lock);
		return -ENOENT;
	}
	if (!atomic_dec_and_test(&doi_def->refcount)) {
		spin_unlock(&cipso_v4_doi_list_lock);
		return -EBUSY;
	}
	list_del_rcu(&doi_def->list);
	spin_unlock(&cipso_v4_doi_list_lock);

	cipso_v4_cache_invalidate();
	call_rcu(&doi_def->rcu, callback);

	return 0;
}

Does that look better?

-- 
paul moore
linux @ hp
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html