Re: [PATCH] NETLABEL: Fix an RCU warning

[Eric Dumazet <eric.dumazet@gmail.com>]

Le jeudi 25 mars 2010 à 11:37 +0000, David Howells a écrit :
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > Sorry this is not the right fix.
> > 
> > Fix is to change the dereference check to take into account the lock
> > owned here.
> 
> Then the comments on netlbl_unlhsh_hash(), netlbl_unlhsh_search_iface(),
> netlbl_unlhsh_search_iface_def() and netlbl_unlhsh_add_iface() are all wrong,
> for all of them say:
> 
> 	* The caller is responsible for calling the rcu_read_[un]lock()
> 	* functions.
> 
> Furthermore, netlabel_unlhsh_add() and netlabel_unlhsh_remove() _do_ wrap the
> calls to those functions in rcu_read_lock'd sections.

Current code is probably fine.
Comments are not up to date (as many other comments BTW)

Only the dereference check is bad, as it assumes the rcu_read lock is
held.

Its not the case, we own a spinlock.

You suggest adding a surrounding rcu lock, but this surrounding lock
adds overhead on normal kernels, to correct checker warnings only.

If a mutex was protecting existing code, instead of a spinlock, then
adding rcu_read_lock() would be no correct anyway (existing code would
not be allowed to call a might_sleep function)

Please take a look at rcu_dereference_check() in :

__sk_free() (file net/core/sock.c) 
__in6_dev_get() (file include/net/addrconf.h)
rcu_dereference_check_fdtable (file include/linux/fdtable.h)
task_subsys_state() (file include/linux/cgroup.h)
rcu_dereference_check_sched_domain (file kernel/sched.c)
...

for examples of proper checks.

Yes, its more difficult, but its the right thing.