Re: suspicious RCU usage in security/selinux/netnode.c

From: Eric Paris
Date: Tue May 15 2012 - 10:52:10 EST


On Tue, May 15, 2012 at 10:46 AM, Paul E. McKenney
<paulmck@xxxxxxxxxxxxxxxxxx> wrote:
> On Tue, May 15, 2012 at 10:24:23AM -0400, Eric Paris wrote:
>> On Tue, May 15, 2012 at 1:16 AM, Paul E. McKenney
>> <paulmck@xxxxxxxxxxxxxxxxxx> wrote:
>> > On Tue, May 15, 2012 at 12:41:45AM -0400, Dave Jones wrote:
>> >> I just triggered this on Linus' current tree.
>> >
>> > This is a bare:
>> >
>> >        rcu_dereference(sel_netnode_hash[idx].list.prev)
>> >
>> > which needs to be in an RCU read-side critical section.  Alternatively,
>> > the above should instead be something like:
>> >
>> >        rcu_dereference_check(sel_netnode_hash[idx].list.prev,
>> >                              lockdep_is_held(&sel_netnode_lock));
>>
>> Right, but that 'bare' dereference comes from
>> list_for_each_entry_rcu(), [from sel_netnode_sid_slow()] which I don't
>> see how to easily annotate with the lock.  Nor do I think it's within
>> my brain power (or my willingness to maintain such in the future) to
>> want to open code that logic.
>
> You lost me on this one.  The lockdep splat called out the
> rcu_dereference() above, not a list_for_each_entry_rcu().  Besides which,
> the list_for_each_entry_rcu() does not do the checking -- at the time,
> I was not willing to explode the API that much.

Ohhhh, ok. I assumed we needed to annotate list_for_each_entry_rcu()
under the spinlock as well as the bare dereference in the insert code.
Ok, should be very easy to fix, although the list running code is
still going to be un-annotated in any way. Thanks

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/