Re: [PATCH v3.17 .. v3.19] lib/rhashtable: fix race between rhashtable_lookup_compare and hashtable resize

2015-06-30 Thread Konstantin Khlebnikov

+CC Sasha Levin

FYI: this patch fixes race in netlink which leads to hung in glibc
function getaddrinfo() because it doesn't handle errors at all.

On 26.06.2015 13:48, Konstantin Khlebnikov wrote:

Hash value passed as argument into rhashtable_lookup_compare could be
computed using different hash table than rhashtable_lookup_compare sees.

This patch passes key into rhashtable_lookup_compare() instead of hash and
compures hash value right in place using the same table as for lookup.

Also it adds comment for rhashtable_hashfn and rhashtable_obj_hashfn:
user must prevent concurrent insert/remove otherwise returned hash value
could be invalid.

Signed-off-by: Konstantin Khlebnikov khlebni...@yandex-team.ru
Fixes: e341694e3eb5 (netlink: Convert netlink_lookup() to use RCU protected hash 
table)
Link: http://lkml.kernel.org/r/20150514042151.ga5...@gondor.apana.org.au
Cc: Stable sta...@vger.kernel.org (v3.17 .. v3.19)
---
  include/linux/rhashtable.h |2 +-
  lib/rhashtable.c   |   12 
  net/netlink/af_netlink.c   |5 +
  3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index fb298e9d6d3a..84056743780a 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -108,7 +108,7 @@ int rhashtable_expand(struct rhashtable *ht, gfp_t flags);
  int rhashtable_shrink(struct rhashtable *ht, gfp_t flags);

  void *rhashtable_lookup(const struct rhashtable *ht, const void *key);
-void *rhashtable_lookup_compare(const struct rhashtable *ht, u32 hash,
+void *rhashtable_lookup_compare(const struct rhashtable *ht, void *key,
bool (*compare)(void *, void *), void *arg);

  void rhashtable_destroy(const struct rhashtable *ht);
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 624a0b7c05ef..cb22073fe687 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -61,6 +61,8 @@ static u32 __hashfn(const struct rhashtable *ht, const void 
*key,
   * Computes the hash value using the hash function provided in the 'hashfn'
   * of struct rhashtable_params. The returned value is guaranteed to be
   * smaller than the number of buckets in the hash table.
+ *
+ * The caller must ensure that no concurrent table mutations occur.
   */
  u32 rhashtable_hashfn(const struct rhashtable *ht, const void *key, u32 len)
  {
@@ -92,6 +94,8 @@ static u32 obj_hashfn(const struct rhashtable *ht, const void 
*ptr, u32 hsize)
   * 'obj_hashfn' depending on whether the hash table is set up to work with
   * a fixed length key. The returned value is guaranteed to be smaller than
   * the number of buckets in the hash table.
+ *
+ * The caller must ensure that no concurrent table mutations occur.
   */
  u32 rhashtable_obj_hashfn(const struct rhashtable *ht, void *ptr)
  {
@@ -474,7 +478,7 @@ EXPORT_SYMBOL_GPL(rhashtable_lookup);
  /**
   * rhashtable_lookup_compare - search hash table with compare function
   * @ht:   hash table
- * @hash:  hash value of desired entry
+ * @key:   pointer to key
   * @compare:  compare function, must return true on match
   * @arg:  argument passed on to compare function
   *
@@ -486,14 +490,14 @@ EXPORT_SYMBOL_GPL(rhashtable_lookup);
   *
   * Returns the first entry on which the compare function returned true.
   */
-void *rhashtable_lookup_compare(const struct rhashtable *ht, u32 hash,
+void *rhashtable_lookup_compare(const struct rhashtable *ht, void *key,
bool (*compare)(void *, void *), void *arg)
  {
const struct bucket_table *tbl = rht_dereference_rcu(ht-tbl, ht);
struct rhash_head *he;
+   u32 hash;

-   if (unlikely(hash = tbl-size))
-   return NULL;
+   hash = __hashfn(ht, key, ht-p.key_len, tbl-size);

rht_for_each_rcu(he, tbl-buckets[hash], ht) {
if (!compare(rht_obj(ht, he), arg))
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c0a418766f9c..c82b2e37e652 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1003,11 +1003,8 @@ static struct sock *__netlink_lookup(struct 
netlink_table *table, u32 portid,
.net = net,
.portid = portid,
};
-   u32 hash;

-   hash = rhashtable_hashfn(table-hash, portid, sizeof(portid));
-
-   return rhashtable_lookup_compare(table-hash, hash,
+   return rhashtable_lookup_compare(table-hash, portid,
 netlink_compare, arg);
  }





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


Re: [PATCH v3.17 .. v3.19] lib/rhashtable: fix race between rhashtable_lookup_compare and hashtable resize

2015-06-30 Thread Thomas Graf
On 06/26/15 at 01:48pm, Konstantin Khlebnikov wrote:
 Hash value passed as argument into rhashtable_lookup_compare could be
 computed using different hash table than rhashtable_lookup_compare sees.
 
 This patch passes key into rhashtable_lookup_compare() instead of hash and
 compures hash value right in place using the same table as for lookup.
 
 Also it adds comment for rhashtable_hashfn and rhashtable_obj_hashfn:
 user must prevent concurrent insert/remove otherwise returned hash value
 could be invalid.
 
 Signed-off-by: Konstantin Khlebnikov khlebni...@yandex-team.ru
 Fixes: e341694e3eb5 (netlink: Convert netlink_lookup() to use RCU protected 
 hash table)
 Link: http://lkml.kernel.org/r/20150514042151.ga5...@gondor.apana.org.au
 Cc: Stable sta...@vger.kernel.org (v3.17 .. v3.19)

Nice catch.

For reference, this is a partial cherry pick of
8d24c0b43125e (rhashtable: Do hashing inside of rhashtable_lookup_compare())

nft_hash is not affected as it doesn't use the lookup API until 4.0.

Acked-by: Thomas Graf tg...@suug.ch
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3.17 .. v3.19] lib/rhashtable: fix race between rhashtable_lookup_compare and hashtable resize

2015-06-27 Thread Herbert Xu
On Fri, Jun 26, 2015 at 01:48:17PM +0300, Konstantin Khlebnikov wrote:
 Hash value passed as argument into rhashtable_lookup_compare could be
 computed using different hash table than rhashtable_lookup_compare sees.
 
 This patch passes key into rhashtable_lookup_compare() instead of hash and
 compures hash value right in place using the same table as for lookup.
 
 Also it adds comment for rhashtable_hashfn and rhashtable_obj_hashfn:
 user must prevent concurrent insert/remove otherwise returned hash value
 could be invalid.
 
 Signed-off-by: Konstantin Khlebnikov khlebni...@yandex-team.ru
 Fixes: e341694e3eb5 (netlink: Convert netlink_lookup() to use RCU protected 
 hash table)
 Link: http://lkml.kernel.org/r/20150514042151.ga5...@gondor.apana.org.au
 Cc: Stable sta...@vger.kernel.org (v3.17 .. v3.19)

This could indeed explain Eric's problems with those kernels.  Eric,
can you rerun your test to see if this patch makes your problem go
away on the pre-4.0 kernels?

Thanks!
-- 
Email: Herbert Xu herb...@gondor.apana.org.au
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html