Hi,
Jonathan E Brassow wrote:
> I believe this also fixes Jun'ichi's issue (*[dm-devel] [PATCH]
> 2.6.12-rc6: fix __rh_alloc()/rh_update_states() race in dm-raid1.c)
Unfortunately, it doesn't eliminate the possibility of the region
being freed during lock conversion (though the possibility becomes
very very low) and also it will cause other problems.
I modified his patch with fixes below
- spin_lock should be spin_lock_irq to avoid deadlock,
- needs to check pending count to avoid moving dirty region to clean list,
and added retry code.
This one should work. How about this?
However, I think we need to find smarter fix not to depend on retry.
Thanks,
Jonathan E Brassow wrote:
I believe this also fixes Jun'ichi's issue (*[dm-devel] [PATCH]
2.6.12-rc6: fix __rh_alloc()/rh_update_states() race in dm-raid1.c)
brassow
*
On Jun 16, 2005, at 9:21 PM, Zhao Qian wrote:
after write_unlock_irq and just before read_lock, there's a small
window which enables a race causing deletion of the region struct in
function rh_update_states(). then in rh_dec(), the __rh_lookup()
will return null, causing kernel panic.
diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -232,6 +232,7 @@ static struct region *__rh_alloc(struct
{
struct region *reg, *nreg;
+retry:
read_unlock(&rh->hash_lock);
nreg = mempool_alloc(rh->region_pool, GFP_NOIO);
nreg->state = rh->log->type->in_sync(rh->log, region, 1) ?
@@ -252,15 +253,19 @@ static struct region *__rh_alloc(struct
else {
__rh_insert(rh, nreg);
- if (nreg->state == RH_CLEAN) {
- spin_lock(&rh->region_lock);
- list_add(&nreg->list, &rh->clean_regions);
- spin_unlock(&rh->region_lock);
- }
reg = nreg;
}
write_unlock_irq(&rh->hash_lock);
read_lock(&rh->hash_lock);
+ /* rare, but it's possible the region is freed by other cpu */
+ if (reg != __rh_lookup(rh, region))
+ goto retry;
+ if (reg->state == RH_CLEAN) {
+ spin_lock_irq(&rh->region_lock);
+ if (!atomic_read(®->pending) && list_empty(®->list))
+ list_add(®->list, &rh->clean_regions);
+ spin_unlock_irq(&rh->region_lock);
+ }
return reg;
}