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
| ||
|
Date: Fri, 16 Jan 2015 11:08:42 +0800 From: Ying Xue <ying.xue@...driver.com> To: <tgraf@...g.ch> CC: <davem@...emloft.net>, <sergei.shtylyov@...entembedded.com>, <netdev@...r.kernel.org> Subject: Re: [PATCH net-next v4] rhashtable: Fix race in rhashtable_destroy() and use regular work_struct On 01/16/2015 10:23 AM, Ying Xue wrote: > When we put our declared work task in the global workqueue with > schedule_delayed_work(), its delay parameter is always zero. > Therefore, we should define a regular work in rhashtable structure > instead of a delayed work. > > By the way, we add a condition to check whether resizing functions > are NULL before cancelling the work, avoiding to cancel an > uninitialized work. > > Lastly, while we wait for all work items we submitted before to run > to completion with cancel_delayed_work(), ht->mutex has been taken in > rhashtable_destroy(). Moreover, cancel_delayed_work() doesn't return > until all work items are accomplished, and when work items are > scheduled, the work's function - rht_deferred_worker() will be called. > However, as rht_deferred_worker() also needs to acquire the lock, > deadlock might happen at the moment as the lock is already held before. > So if the cancel work function is moved out of the lock covered scope, > this will avoid the deadlock. > > Fixes: 97defe1 ("rhashtable: Per bucket locks & deferred expansion/shrinking") > Signed-off-by: Ying Xue <ying.xue@...driver.com> > Cc: Thomas Graf <tgraf@...g.ch> > Signed-off-by: Ying Xue <ying.xue@...driver.com> Sorry, there is a duplicated my "signed-off" but lack of Thomas's "acked-by". So please ignore the version. Regards, Ying > --- > Change log: > v4: > - Add an empty line in rhashtable_destroy() > - Revise commit description > v3: > - Revise patch header description from Thomas's suggestion > v2: > - Move cancel_work_sync() out of ht->mutex lock scope from Thomas's > comments > include/linux/rhashtable.h | 2 +- > lib/rhashtable.c | 12 ++++++------ > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h > index 9570832..a2562ed 100644 > --- a/include/linux/rhashtable.h > +++ b/include/linux/rhashtable.h > @@ -119,7 +119,7 @@ struct rhashtable { > atomic_t nelems; > atomic_t shift; > struct rhashtable_params p; > - struct delayed_work run_work; > + struct work_struct run_work; > struct mutex mutex; > bool being_destroyed; > }; > diff --git a/lib/rhashtable.c b/lib/rhashtable.c > index aca6998..84a78e3 100644 > --- a/lib/rhashtable.c > +++ b/lib/rhashtable.c > @@ -485,7 +485,7 @@ static void rht_deferred_worker(struct work_struct *work) > struct rhashtable *ht; > struct bucket_table *tbl; > > - ht = container_of(work, struct rhashtable, run_work.work); > + ht = container_of(work, struct rhashtable, run_work); > mutex_lock(&ht->mutex); > tbl = rht_dereference(ht->tbl, ht); > > @@ -507,7 +507,7 @@ static void rhashtable_wakeup_worker(struct rhashtable *ht) > if (tbl == new_tbl && > ((ht->p.grow_decision && ht->p.grow_decision(ht, size)) || > (ht->p.shrink_decision && ht->p.shrink_decision(ht, size)))) > - schedule_delayed_work(&ht->run_work, 0); > + schedule_work(&ht->run_work); > } > > static void __rhashtable_insert(struct rhashtable *ht, struct rhash_head *obj, > @@ -903,7 +903,7 @@ int rhashtable_init(struct rhashtable *ht, struct rhashtable_params *params) > get_random_bytes(&ht->p.hash_rnd, sizeof(ht->p.hash_rnd)); > > if (ht->p.grow_decision || ht->p.shrink_decision) > - INIT_DEFERRABLE_WORK(&ht->run_work, rht_deferred_worker); > + INIT_WORK(&ht->run_work, rht_deferred_worker); > > return 0; > } > @@ -921,11 +921,11 @@ void rhashtable_destroy(struct rhashtable *ht) > { > ht->being_destroyed = true; > > - mutex_lock(&ht->mutex); > + if (ht->p.grow_decision || ht->p.shrink_decision) > + cancel_work_sync(&ht->run_work); > > - cancel_delayed_work(&ht->run_work); > + mutex_lock(&ht->mutex); > bucket_table_free(rht_dereference(ht->tbl, ht)); > - > mutex_unlock(&ht->mutex); > } > EXPORT_SYMBOL_GPL(rhashtable_destroy); > -- 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
Powered by blists - more mailing lists