Re: RCU issue with SELinux (Re: SELINUX performance issues)

["Kaigai Kohei" <kaigai@ak.jp.nec.com>]

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>


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.