lkml.org 
[lkml]   [2004]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH]SELinux performance improvement by RCU (Re: RCU issue with SELinux)
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
-
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.039 / U:0.380 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site