netdev - Re: [PATCH] NETLABEL: Fix an RCU warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 25 Mar 2010 14:32:20 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	David Howells <dhowells@...hat.com>
Cc:	paul.moore@...com, netdev@...r.kernel.org,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH] NETLABEL: Fix an RCU warning

Le jeudi 25 mars 2010 à 11:37 +0000, David Howells a écrit :
> Eric Dumazet <eric.dumazet@...il.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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html