Re: [PATCH]SELinux performance improvement by RCU (Re: RCU issue with SELinux)

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

Hi Stephen, Paul, thanks for your comments.

> > > The attached take-4 patches replace the avc_lock in security/selinux/avc.c
> > > by the lock-less read access with RCU.
> > 
> > Thanks.  Was there a reason you didn't move the rcu_read_lock call after
> > the avc_insert call per the suggestion of Paul McKenney, or was that
> > just an oversight?  No need to send a new patch, just ack whether or not
> > you meant to switch the order there.
> 
> One reason might be because I called it out in the text of my message,
> but failed to put it in my patch.  :-/  Of course, if there is some reason
> why moving the rcu_read_lock() call is bad, I would like to know for
> my own education.

In my understanding, the issue is the Paul's suggestion as follows:

> So I do not believe that avc_insert() needs rcu_read_lock().
> Unless I am missing something, the rcu_read_lock() acquired
> in avc_has_perm_noaudit() should be moved after the call to
> avc_insert().

I don't move the rcu_read_lock() because of the possibility of preemption
between the spin_unlock_irqrestore() in avc_insert() and the rcu_read_lock()
which may be inserted after avc_insert() in avc_has_perm_noaudit().

When it's returning from avc_insert(), we can't ignore the possibility
that execution is preempted in this timing.
Therefore, I didn't move rcu_read_lock() in spite of its redundancy.

If rcu_read_lock() was moved after avc_insert()
[ in avc_insert() ]----------------------------
                :
        spin_lock_irqsave(&avc_cache.slots_lock[hvalue], flag);
        list_for_each_entry(pos, &avc_cache.slots[hvalue], list) {
                :
        }
        list_add_rcu(&node->list, &avc_cache.slots[hvalue]);
found:
        spin_unlock_irqrestore(&avc_cache.slots_lock[hvalue], flag);  ---------
        //  +--> including preempt_enable()                               |
                It has the danger of releasing the 'node'.                V
    }                                                                preemption
out:                                                                     is
    return node;                                                       possible
}
-----------------------------------------------
Because it's legal to hold the rcu_read_lock() twice as Paul says,
we should do it for safety.
It's the reason that I didn't move rcu_read_lock() at this point,
and it might be lack of my explanation, sorry.

Thanks.
--------
Kai Gai <kaigai@ak.jp.nec.com>