| Message ID | Z4XWx5X0doetOJni@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v3] rhashtable: Fix rhashtable_try_insert test | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
From: Herbert Xu <herbert@gondor.apana.org.au> Sent: Monday, January 13, 2025 7:15 PM > > On Fri, Jan 10, 2025 at 06:22:40PM +0000, Michael Kelley wrote: > > > > This patch passes my tests. I'm doing a narrow test to verify that > > the boot failure when opening the Mellanox NIC is no longer occurring. > > I also unloaded/reloaded the mlx5 driver a couple of times. For good > > measure, I then did a full Linux kernel build, and all is good. My testing > > does not broadly verify correct operation of rhashtable except as it > > gets exercised implicitly by these basic tests. > > Thanks for testing! The patch needs one more change though as > moving the atomic_inc outside of the lock was a bad idea on my > part. This could cause atomic_inc/atomic_dec to be reordered > thus resulting in an underflow. > > Thanks, I've tested this version of the fix in the same limited way as before. All is good. I'm still testing against linux-next20250108 from a few days ago, but that should not make any difference. I have *not* reviewed the code change itself. Tested-by: Michael Kelley <mhklinux@outlook.com> > > ---8<--- > The test on whether rhashtable_insert_one did an insertion relies > on the value returned by rhashtable_lookup_one. Unfortunately that > value is overwritten after rhashtable_insert_one returns. Fix this > by moving the test before data gets overwritten. > > Simplify the test as only data == NULL matters. > > Finally move atomic_inc back within the lock as otherwise it may > be reordered with the atomic_dec on the removal side, potentially > leading to an underflow. > > Reported-by: Michael Kelley <mhklinux@outlook.com> > Fixes: e1d3422c95f0 ("rhashtable: Fix potential deadlock by moving schedule_work outside lock") > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/lib/rhashtable.c b/lib/rhashtable.c > index bf956b85455a..0e9a1d4cf89b 100644 > --- a/lib/rhashtable.c > +++ b/lib/rhashtable.c > @@ -611,21 +611,23 @@ static void *rhashtable_try_insert(struct rhashtable *ht, > const void *key, > new_tbl = rht_dereference_rcu(tbl->future_tbl, ht); > data = ERR_PTR(-EAGAIN); > } else { > + bool inserted; > + > flags = rht_lock(tbl, bkt); > data = rhashtable_lookup_one(ht, bkt, tbl, > hash, key, obj); > new_tbl = rhashtable_insert_one(ht, bkt, tbl, > hash, obj, data); > + inserted = data && !new_tbl; > + if (inserted) > + atomic_inc(&ht->nelems); > if (PTR_ERR(new_tbl) != -EEXIST) > data = ERR_CAST(new_tbl); > > rht_unlock(tbl, bkt, flags); > > - if (PTR_ERR(data) == -ENOENT && !new_tbl) { > - atomic_inc(&ht->nelems); > - if (rht_grow_above_75(ht, tbl)) > - schedule_work(&ht->run_work); > - } > + if (inserted && rht_grow_above_75(ht, tbl)) > + schedule_work(&ht->run_work); > } > } while (!IS_ERR_OR_NULL(new_tbl)); >
diff --git a/lib/rhashtable.c b/lib/rhashtable.c index bf956b85455a..0e9a1d4cf89b 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -611,21 +611,23 @@ static void *rhashtable_try_insert(struct rhashtable *ht, const void *key, new_tbl = rht_dereference_rcu(tbl->future_tbl, ht); data = ERR_PTR(-EAGAIN); } else { + bool inserted; + flags = rht_lock(tbl, bkt); data = rhashtable_lookup_one(ht, bkt, tbl, hash, key, obj); new_tbl = rhashtable_insert_one(ht, bkt, tbl, hash, obj, data); + inserted = data && !new_tbl; + if (inserted) + atomic_inc(&ht->nelems); if (PTR_ERR(new_tbl) != -EEXIST) data = ERR_CAST(new_tbl); rht_unlock(tbl, bkt, flags); - if (PTR_ERR(data) == -ENOENT && !new_tbl) { - atomic_inc(&ht->nelems); - if (rht_grow_above_75(ht, tbl)) - schedule_work(&ht->run_work); - } + if (inserted && rht_grow_above_75(ht, tbl)) + schedule_work(&ht->run_work); } } while (!IS_ERR_OR_NULL(new_tbl));