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

["Paul E. McKenney" <paulmck@us.ibm.com>]

On Tue, Aug 31, 2004 at 01:33:33PM +0900, Kaigai Kohei wrote:
> 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.

Works for me!  Might be worth adding a comment, though.

							Thanx, Paul