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: Mon, 4 May 2015 11:20:08 +0900 From: Minchan Kim <minchan@...nel.org> To: Sergey Senozhatsky <sergey.senozhatsky.work@...il.com> Cc: Andrew Morton <akpm@...ux-foundation.org>, Nitin Gupta <ngupta@...are.org>, linux-kernel@...r.kernel.org, Sergey Senozhatsky <sergey.senozhatsky@...il.com> Subject: Re: [PATCHv3 9/9] zram: add dynamic device add/remove functionality Hello Sergey, On Thu, Apr 30, 2015 at 03:51:12PM +0900, Sergey Senozhatsky wrote: > On (04/30/15 15:44), Minchan Kim wrote: > > > > I think the problem of deadlock is that you are trying to remove sysfs file > > > > in sysfs handler. > > > > > > > > #> echo 1 > /sys/xxx/zram_remove > > > > > > > > kernfs_fop_write - hold s_active > > > > -> zram_remove_store > > > > -> zram_remove > > > > -> sysfs_remove_group - hold s_active *again* > > > > > > > > Right? > > > > > > > > > > are those same s_active locks? > > > > > > > > > we hold (s_active#163) and (&bdev->bd_mutex) and want to acquire (s_active#162) > > > > Thanks for sharing the message. > > You're right. It's another lock so it shouldn't be a reason. > > Okay, I will review it. Please give me time. > > > > sure, no problem and no rush. thanks! I had a time to think over it. I think your patch is rather tricky so someone cannot see sysfs although he already opened /dev/zram but after a while he can see sysfs. It's weired. I want to fix it more generic way. Othewise, we might have trouble with locking problem sometime. We already have experieced it with init_lock although we finally fixed it. I think we can fix it with below patch I hope it's more general and right approach. It's based on your [zram: return zram device_id from zram_add()] What do you think about? >From e943df5407b880f9262ef959b270226fdc81bc9f Mon Sep 17 00:00:00 2001 From: Minchan Kim <minchan@...nel.org> Date: Mon, 4 May 2015 08:36:07 +0900 Subject: [PATCH 1/2] zram: close race by open overriding [1] introduced bdev->bd_mutex to protect a race between mount and reset. At that time, we don't have dynamic zram-add/remove feature so it was okay. However, as we introduce dynamic device feature, bd_mutex became trouble. CPU 0 echo 1 > /sys/block/zram<id>/reset -> kernfs->s_active(A) -> zram:reset_store->bd_mutex(B) CPU 1 echo <id> > /sys/class/zram/zram-remove ->zram:zram_remove: bd_mutex(B) -> sysfs_remove_group -> kernfs->s_active(A) IOW, AB -> BA deadlock The reason we are holding bd_mutex for zram_remove is to prevent any incoming open /dev/zram[0-9]. Otherwise, we could remove zram others already have opened. But it causes above deadlock problem. To fix the problem, this patch overrides block_device.open and it returns -EBUSY if zram asserts he claims zram to reset so any incoming open will be failed so we don't need to hold bd_mutex for zram_remove ayn more. This patch is to prepare for zram-add/remove feature. [1] ba6b17: zram: fix umount-reset_store-mount race condition Signed-off-by: Minchan Kim <minchan@...nel.org> --- drivers/block/zram/zram_drv.c | 42 ++++++++++++++++++++++++++++++++---------- drivers/block/zram/zram_drv.h | 4 ++++ 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 3df4394..7fb72dc 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -1074,13 +1074,6 @@ static ssize_t reset_store(struct device *dev, if (!bdev) return -ENOMEM; - mutex_lock(&bdev->bd_mutex); - /* Do not reset an active device! */ - if (bdev->bd_openers) { - ret = -EBUSY; - goto out; - } - ret = kstrtou16(buf, 10, &do_reset); if (ret) goto out; @@ -1090,23 +1083,52 @@ static ssize_t reset_store(struct device *dev, goto out; } + mutex_lock(&bdev->bd_mutex); + /* Do not reset an active device or claimed device */ + if (bdev->bd_openers || zram->claim) { + ret = -EBUSY; + mutex_unlock(&bdev->bd_mutex); + goto out; + } + + /* From now on, anyone can't open /dev/zram[0-9] */ + zram->claim = true; + mutex_unlock(&bdev->bd_mutex); + /* Make sure all pending I/O is finished */ fsync_bdev(bdev); zram_reset_device(zram); - mutex_unlock(&bdev->bd_mutex); revalidate_disk(zram->disk); bdput(bdev); - return len; + mutex_lock(&bdev->bd_mutex); + zram->claim = false; + mutex_unlock(&bdev->bd_mutex); + return len; out: - mutex_unlock(&bdev->bd_mutex); bdput(bdev); return ret; } +static int zram_open(struct block_device *bdev, fmode_t mode) +{ + int ret = 0; + struct zram *zram; + + WARN_ON(!mutex_is_locked(&bdev->bd_mutex)); + + zram = bdev->bd_disk->private_data; + /* zram was claimed to reset so open request fails */ + if (zram->claim) + ret = -EBUSY; + + return ret; +} + static const struct block_device_operations zram_devops = { + .open = zram_open, .swap_slot_free_notify = zram_slot_free_notify, .rw_page = zram_rw_page, .owner = THIS_MODULE diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h index 042994e..6dbe2df 100644 --- a/drivers/block/zram/zram_drv.h +++ b/drivers/block/zram/zram_drv.h @@ -115,5 +115,9 @@ struct zram { */ u64 disksize; /* bytes */ char compressor[10]; + /* + * zram is claimed so open request will be failed + */ + bool claim; /* Protected by bdev->bd_mutex */ }; #endif -- 1.9.3 -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists