lkml.org 
[lkml]   [2004]   [Aug]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: RCU issue with SELinux (Re: SELINUX performance issues)
From
Date
On Wed, 2004-08-25 at 05:51, Kaigai Kohei wrote:
> The attached take3-patch is modified as follows:
> - avc_node_dual was eliminated by Paul E.McKenny's suggestion.
> avc_update_node() calls kmalloc() and may return -ENOMEM.
> (But, I think this effect is so limited.)
> - All list_for_each_entry() were replaced by list_for_each_entry_rcu().
> - All spin_lock()/spin_unlock() were replaced by spin_lock_irqsave()
> /spin_unlock_restore().
> - In avc_node_insert(), if an entry with the same ssid/tsid/tclass as new
> one exists, the older entry is replaced by the new one.
>
> Thanks. I want to make it the last edition hopefully. :)

I haven't tracked down the cause yet, but a kernel built with all three
patches (list_replace_rcu, atomic_inc_return, and selinux.rcu take3) on
x86 doesn't allow an enforcing boot; it begins auditing denials _before_
the initial policy load (which should never happen, as
security_compute_av allows everything until the policy is loaded), and
prevents /sbin/init from loading the policy.

A few other comments on the patch:

+ new = kmalloc(sizeof(struct avc_node), GFP_ATOMIC);
+ if (!new)
+ return NULL;

Dynamically allocating the nodes at runtime (rather than pre-allocating
them and then just reclaiming them as necessary as in the current AVC)
worries me, as it introduces a new failure case for avc_has_perm.
Denying permission to a resource due to transient memory shortage is not
good for robustness. And changing the GFP_ATOMIC is not an option, as
calling context may not allow blocking. Hence, pre-allocation seems
desirable, regardless of the locking scheme.

+static int avc_latest_notif_update(int seqno, int is_insert)
+{
+ int ret = 0;
+ static spinlock_t notif_lock = SPIN_LOCK_UNLOCKED;
+ unsigned long flag;
+
+ spin_lock_irqsave(&notif_lock, flag);
+ if (seqno < avc_cache.latest_notif) {
+ if (is_insert) {
+ printk(KERN_WARNING "avc: seqno %d < latest_notif %d\n",
+ seqno, avc_cache.latest_notif);
+ ret = -EAGAIN;
+ } else {
+ avc_cache.latest_notif = seqno;
+ }
+ }
+ spin_unlock_irqrestore(&notif_lock, flag);
+ return ret;
}

In trying to merge the logic related to latest_notif, you've introduced
a bug - latest_notif should only be increased, never decreased. See the
original logic from avc_control and avc_ss_reset prior to your patch.
Those functions update the latest notif based on a policy change event.
In the insert case, you are checking that the entry is not stale, i.e.
has a smaller seqno than the latest notification due to an interleaving
with a policy change event.

+ if (node->ae.avd.allowed != (node->ae.avd.allowed|requested))
+ avc_update_node(AVC_CALLBACK_GRANT
+ ,requested,ssid,tsid,tclass);
}

The test seems unnecessary, as the function has already determined that
not all of the requested permissions were granted, so you should be able
to just unconditionally call avc_update_node here, and you only need to
pass it the denied set that has already been computed, as any other
permissions in requested were already allowed.

--
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:05    [W:0.054 / U:3.300 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site