Skip to contentSkip to conversation

netlabel: Fix several rcu_dereference() calls used without RCU read locks

PATCH Applied Paul Moore 3 participants 3 messages Apr 1, 2010 – Apr 2, 2010

Subsystems (3)
lore
Share & tools
lore

Paul Moore

author
#0 lore raw

PATCH 1/1 netlabel: Fix several rcu_dereference() calls used without RCU read locks

The recent changes to add RCU lock verification to rcu_dereference() calls caught out a problem with netlbl_unlhsh_hash(), see below.

===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
net/netlabel/netlabel_unlabeled.c:246 invoked rcu_dereference_check() without protection!

This patch fixes this problem as well as others like it in the NetLabel code. Also included in this patch is the identification of future work to eliminate the RCU read lock in netlbl_domhsh_add(), but in the interest of getting this patch out quickly that work will happen in another patch to be finished later.

Thanks to Eric Dumazet and Paul McKenney for their help in understanding the recent RCU changes.

---
 net/netlabel/netlabel_domainhash.c |   28 ++++++++++-----
 net/netlabel/netlabel_unlabeled.c  |   66 ++++++++++--------------------------
 2 files changed, 37 insertions(+), 57 deletions(-)
net/netlabel/netlabel_domainhash.c +18 -10
index 0bfeaab..06ab41b 100644
@@ -50,9 +50,12 @@ struct netlbl_domhsh_tbl {
 };
 
 /* Domain hash table */
-/* XXX - updates should be so rare that having one spinlock for the entire
- * hash table should be okay */
+/* updates should be so rare that having one spinlock for the entire hash table
+ * should be okay */
 static DEFINE_SPINLOCK(netlbl_domhsh_lock);
+#define netlbl_domhsh_rcu_deref(p) \
+	rcu_dereference_check(p, rcu_read_lock_held() || \
+				 lockdep_is_held(&netlbl_domhsh_lock))
 static struct netlbl_domhsh_tbl *netlbl_domhsh = NULL;
 static struct netlbl_dom_map *netlbl_domhsh_def = NULL;
 
@@ -106,7 +109,8 @@ static void netlbl_domhsh_free_entry(struct rcu_head *entry)
  * Description:
  * This is the hashing function for the domain hash table, it returns the
  * correct bucket number for the domain.  The caller is responsibile for
- * calling the rcu_read_[un]lock() functions.
+ * ensuring that the hash table is protected with either a RCU read lock or the
+ * hash table lock.
  *
  */
 static u32 netlbl_domhsh_hash(const char *key)
@@ -120,7 +124,7 @@ static u32 netlbl_domhsh_hash(const char *key)
 
 	for (iter = 0, val = 0, len = strlen(key); iter < len; iter++)
 		val = (val << 4 | (val >> (8 * sizeof(u32) - 4))) ^ key[iter];
-	return val & (rcu_dereference(netlbl_domhsh)->size - 1);
+	return val & (netlbl_domhsh_rcu_deref(netlbl_domhsh)->size - 1);
 }
 
 /**
@@ -130,7 +134,8 @@ static u32 netlbl_domhsh_hash(const char *key)
  * Description:
  * Searches the domain hash table and returns a pointer to the hash table
  * entry if found, otherwise NULL is returned.  The caller is responsibile for
- * the rcu hash table locks (i.e. the caller much call rcu_read_[un]lock()).
+ * ensuring that the hash table is protected with either a RCU read lock or the
+ * hash table lock.
  *
  */
 static struct netlbl_dom_map *netlbl_domhsh_search(const char *domain)
@@ -141,7 +146,7 @@ static struct netlbl_dom_map *netlbl_domhsh_search(const char *domain)
 
 	if (domain != NULL) {
 		bkt = netlbl_domhsh_hash(domain);
-		bkt_list = &rcu_dereference(netlbl_domhsh)->tbl[bkt];
+		bkt_list = &netlbl_domhsh_rcu_deref(netlbl_domhsh)->tbl[bkt];
 		list_for_each_entry_rcu(iter, bkt_list, list)
 			if (iter->valid && strcmp(iter->domain, domain) == 0)
 				return iter;
@@ -159,8 +164,8 @@ static struct netlbl_dom_map *netlbl_domhsh_search(const char *domain)
  * Searches the domain hash table and returns a pointer to the hash table
  * entry if an exact match is found, if an exact match is not present in the
  * hash table then the default entry is returned if valid otherwise NULL is
- * returned.  The caller is responsibile for the rcu hash table locks
- * (i.e. the caller much call rcu_read_[un]lock()).
+ * returned.  The caller is responsibile ensuring that the hash table is
+ * protected with either a RCU read lock or the hash table lock.
  *
  */
 static struct netlbl_dom_map *netlbl_domhsh_search_def(const char *domain)
@@ -169,7 +174,7 @@ static struct netlbl_dom_map *netlbl_domhsh_search_def(const char *domain)
 
 	entry = netlbl_domhsh_search(domain);
 	if (entry == NULL) {
-		entry = rcu_dereference(netlbl_domhsh_def);
+		entry = netlbl_domhsh_rcu_deref(netlbl_domhsh_def);
 		if (entry != NULL && !entry->valid)
 			entry = NULL;
 	}
@@ -306,8 +311,11 @@ int netlbl_domhsh_add(struct netlbl_dom_map *entry,
 	struct netlbl_af6list *tmp6;
 #endif /* IPv6 */
 
+	/* XXX - we can remove this RCU read lock as the spinlock protects the
+	 *       entire function, but before we do we need to fixup the
+	 *       netlbl_af[4,6]list RCU functions to do "the right thing" with
+	 *       respect to rcu_dereference() when only a spinlock is held. */
 	rcu_read_lock();
-
 	spin_lock(&netlbl_domhsh_lock);
 	if (entry->domain != NULL)
 		entry_old = netlbl_domhsh_search(entry->domain);
net/netlabel/netlabel_unlabeled.c +19 -47

145 lines not on this page · raw message

Acked-by on v1 1/1
#1 lore raw
Quoted 18 lines · Paul Moore

The recent changes to add RCU lock verification to rcu_dereference() calls caught out a problem with netlbl_unlhsh_hash(), see below.

===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
net/netlabel/netlabel_unlabeled.c:246 invoked rcu_dereference_check() without protection!

This patch fixes this problem as well as others like it in the NetLabel code. Also included in this patch is the identification of future work to eliminate the RCU read lock in netlbl_domhsh_add(), but in the interest of getting this patch out quickly that work will happen in another patch to be finished later.

Thanks to Eric Dumazet and Paul McKenney for their help in understanding the recent RCU changes.

Quoted 9 lines
---
 net/netlabel/netlabel_domainhash.c |   28 ++++++++++-----
 net/netlabel/netlabel_unlabeled.c  |   66 ++++++++++--------------------------
 2 files changed, 37 insertions(+), 57 deletions(-)

Thanks Paul

maintainer Applied
#2 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 01 Apr 2010 22:56:47 +0200

Quoted 29 lines · Paul Moore
Quoted 18 lines

The recent changes to add RCU lock verification to rcu_dereference() calls caught out a problem with netlbl_unlhsh_hash(), see below.

===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
net/netlabel/netlabel_unlabeled.c:246 invoked rcu_dereference_check() without protection!

This patch fixes this problem as well as others like it in the NetLabel code. Also included in this patch is the identification of future work to eliminate the RCU read lock in netlbl_domhsh_add(), but in the interest of getting this patch out quickly that work will happen in another patch to be finished later.

Thanks to Eric Dumazet and Paul McKenney for their help in understanding the recent RCU changes.

Quoted 9 lines
---
 net/netlabel/netlabel_domainhash.c |   28 ++++++++++-----
 net/netlabel/netlabel_unlabeled.c  |   66 ++++++++++--------------------------
 2 files changed, 37 insertions(+), 57 deletions(-)
Quoted 1 line

Applied.