Messages in this thread | | | From | "Kaigai Kohei" <> | Subject | Re: RCU issue with SELinux (Re: SELINUX performance issues) | Date | Wed, 25 Aug 2004 18:51:53 +0900 |
| |
Hi Paul, thanks for your comments.
> > I modified the following points: > > - We hold the lock for hash backet when avc_insert() and avc_ss_reset() are > > called for safety. > > - list_for_each_rcu() and list_entry() are replaced by list_for_entry(). > > One subtlety here... > > The traversals that are protected by rcu_read_lock() (rather than an > update-side spinlock) need to be list_for_each_entry_rcu() rather than > list_for_each_entry(). The "_rcu()" is required in order to work > reliably on Alpha, and has the added benefit of calling out exactly > which traversals are RCU-protected. > > Update-side code remains list_for_each_entry().
It was a simple misconception. I fixed them in the take3-patch.
> > - avc_node_dual structure which contains two avc_node objects is defined. > > It allows to do avc_update_node() without kmalloc() or any locks. > > What happens when you have two consecutive updates to the same object? > Don't you have to defer the second update until a grace period has > elapsed since the first update in order to avoid confusing readers that > are still accessing the original version?
I didn't imagine such a situation. Indeed, such thing may happen.
> One way to do this would be to set a "don't-touch-me" bit that is > cleared by an RCU callback. An update to an element with the > "don't-touch-me" bit set would block until the bit clears. There > are probably better ways...
I think we can't apply this approach for the implementation of avc_update_node(), because execution context isn't permitted to block.
I changed my opinion and implementation of avc_update_node(). If kmalloc() returns NULL in avc_update_node(), it returns -ENOMEM.
But this effect of changing the prototype is limited, because only avc_has_perm_noaudit() and avc_update_cache() call avc_update_node().
Even if avc_update_node() return -ENOMEM to avc_has_perm_noaudit(), avc_has_perm_noaudit() can ignore it, because the purpose is only to control the audit-log floods. This adverse effect is only that audit-logs are printed twice.
Nobody calls avc_update_cache(), which is only defined.
Some other trivial fixes are as follows: - 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.
Thank you for the opinion as a specialist of RCU! -------- Kai Gai <kaigai@ak.jp.nec.com>
- 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/
| |