* [bug report] zram: avoid race between zram_remove and disksize_store
@ 2021-11-04 11:48 Dan Carpenter
2021-11-04 11:49 ` Dan Carpenter
2021-11-05 0:23 ` Ming Lei
0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2021-11-04 11:48 UTC (permalink / raw)
To: ming.lei; +Cc: Sergey Senozhatsky, linux-block
Hello Ming Lei,
The patch 5a4b653655d5: "zram: avoid race between zram_remove and
disksize_store" from Oct 25, 2021, leads to the following Smatch
static checker warning:
drivers/block/zram/zram_drv.c:2044 zram_remove()
warn: 'zram->mem_pool' double freed
drivers/block/zram/zram_drv.c
2002 static int zram_remove(struct zram *zram)
2003 {
2004 struct block_device *bdev = zram->disk->part0;
2005 bool claimed;
2006
2007 mutex_lock(&bdev->bd_disk->open_mutex);
2008 if (bdev->bd_openers) {
2009 mutex_unlock(&bdev->bd_disk->open_mutex);
2010 return -EBUSY;
2011 }
2012
2013 claimed = zram->claim;
2014 if (!claimed)
2015 zram->claim = true;
2016 mutex_unlock(&bdev->bd_disk->open_mutex);
2017
2018 zram_debugfs_unregister(zram);
2019
2020 if (claimed) {
2021 /*
2022 * If we were claimed by reset_store(), del_gendisk() will
2023 * wait until reset_store() is done, so nothing need to do.
2024 */
2025 ;
2026 } else {
2027 /* Make sure all the pending I/O are finished */
2028 sync_blockdev(bdev);
2029 zram_reset_device(zram);
^^^^^^^^^^^^^^^^^^^^^^^^
This frees zram->mem_pool in zram_meta_free().
2030 }
2031
2032 pr_info("Removed device: %s\n", zram->disk->disk_name);
2033
2034 del_gendisk(zram->disk);
2035
2036 /* del_gendisk drains pending reset_store */
2037 WARN_ON_ONCE(claimed && zram->claim);
2038
2039 /*
2040 * disksize_store() may be called in between zram_reset_device()
2041 * and del_gendisk(), so run the last reset to avoid leaking
2042 * anything allocated with disksize_store()
2043 */
--> 2044 zram_reset_device(zram);
This double frees it.
2045
2046 blk_cleanup_disk(zram->disk);
2047 kfree(zram);
2048 return 0;
2049 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] zram: avoid race between zram_remove and disksize_store
2021-11-04 11:48 [bug report] zram: avoid race between zram_remove and disksize_store Dan Carpenter
@ 2021-11-04 11:49 ` Dan Carpenter
2021-11-05 0:23 ` Ming Lei
1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2021-11-04 11:49 UTC (permalink / raw)
To: ming.lei; +Cc: Sergey Senozhatsky, linux-block
On Thu, Nov 04, 2021 at 02:48:30PM +0300, Dan Carpenter wrote:
> Hello Ming Lei,
>
> The patch 5a4b653655d5: "zram: avoid race between zram_remove and
> disksize_store" from Oct 25, 2021, leads to the following Smatch
> static checker warning:
>
> drivers/block/zram/zram_drv.c:2044 zram_remove()
> warn: 'zram->mem_pool' double freed
>
> drivers/block/zram/zram_drv.c
> 2002 static int zram_remove(struct zram *zram)
> 2003 {
> 2004 struct block_device *bdev = zram->disk->part0;
> 2005 bool claimed;
> 2006
> 2007 mutex_lock(&bdev->bd_disk->open_mutex);
> 2008 if (bdev->bd_openers) {
> 2009 mutex_unlock(&bdev->bd_disk->open_mutex);
> 2010 return -EBUSY;
> 2011 }
> 2012
> 2013 claimed = zram->claim;
> 2014 if (!claimed)
> 2015 zram->claim = true;
> 2016 mutex_unlock(&bdev->bd_disk->open_mutex);
> 2017
> 2018 zram_debugfs_unregister(zram);
> 2019
> 2020 if (claimed) {
> 2021 /*
> 2022 * If we were claimed by reset_store(), del_gendisk() will
> 2023 * wait until reset_store() is done, so nothing need to do.
> 2024 */
> 2025 ;
> 2026 } else {
> 2027 /* Make sure all the pending I/O are finished */
> 2028 sync_blockdev(bdev);
> 2029 zram_reset_device(zram);
> ^^^^^^^^^^^^^^^^^^^^^^^^
> This frees zram->mem_pool in zram_meta_free().
>
> 2030 }
> 2031
> 2032 pr_info("Removed device: %s\n", zram->disk->disk_name);
> 2033
> 2034 del_gendisk(zram->disk);
> 2035
> 2036 /* del_gendisk drains pending reset_store */
> 2037 WARN_ON_ONCE(claimed && zram->claim);
> 2038
> 2039 /*
> 2040 * disksize_store() may be called in between zram_reset_device()
> 2041 * and del_gendisk(), so run the last reset to avoid leaking
> 2042 * anything allocated with disksize_store()
> 2043 */
> --> 2044 zram_reset_device(zram);
>
> This double frees it.
I should have included all three warnings:
drivers/block/zram/zram_drv.c:2044 zram_remove() warn: 'zram->mem_pool' double freed
drivers/block/zram/zram_drv.c:2044 zram_remove() warn: 'zram->mem_pool->name' double freed
drivers/block/zram/zram_drv.c:2044 zram_remove() warn: 'zram->table' double freed
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] zram: avoid race between zram_remove and disksize_store
2021-11-04 11:48 [bug report] zram: avoid race between zram_remove and disksize_store Dan Carpenter
2021-11-04 11:49 ` Dan Carpenter
@ 2021-11-05 0:23 ` Ming Lei
2021-11-05 9:18 ` Dan Carpenter
1 sibling, 1 reply; 4+ messages in thread
From: Ming Lei @ 2021-11-05 0:23 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Sergey Senozhatsky, linux-block, ming.lei
Hello Dan,
On Thu, Nov 04, 2021 at 02:48:30PM +0300, Dan Carpenter wrote:
> Hello Ming Lei,
>
> The patch 5a4b653655d5: "zram: avoid race between zram_remove and
> disksize_store" from Oct 25, 2021, leads to the following Smatch
> static checker warning:
>
> drivers/block/zram/zram_drv.c:2044 zram_remove()
> warn: 'zram->mem_pool' double freed
>
> drivers/block/zram/zram_drv.c
> 2002 static int zram_remove(struct zram *zram)
> 2003 {
> 2004 struct block_device *bdev = zram->disk->part0;
> 2005 bool claimed;
> 2006
> 2007 mutex_lock(&bdev->bd_disk->open_mutex);
> 2008 if (bdev->bd_openers) {
> 2009 mutex_unlock(&bdev->bd_disk->open_mutex);
> 2010 return -EBUSY;
> 2011 }
> 2012
> 2013 claimed = zram->claim;
> 2014 if (!claimed)
> 2015 zram->claim = true;
> 2016 mutex_unlock(&bdev->bd_disk->open_mutex);
> 2017
> 2018 zram_debugfs_unregister(zram);
> 2019
> 2020 if (claimed) {
> 2021 /*
> 2022 * If we were claimed by reset_store(), del_gendisk() will
> 2023 * wait until reset_store() is done, so nothing need to do.
> 2024 */
> 2025 ;
> 2026 } else {
> 2027 /* Make sure all the pending I/O are finished */
> 2028 sync_blockdev(bdev);
> 2029 zram_reset_device(zram);
> ^^^^^^^^^^^^^^^^^^^^^^^^
> This frees zram->mem_pool in zram_meta_free().
>
> 2030 }
> 2031
> 2032 pr_info("Removed device: %s\n", zram->disk->disk_name);
> 2033
> 2034 del_gendisk(zram->disk);
> 2035
> 2036 /* del_gendisk drains pending reset_store */
> 2037 WARN_ON_ONCE(claimed && zram->claim);
> 2038
> 2039 /*
> 2040 * disksize_store() may be called in between zram_reset_device()
> 2041 * and del_gendisk(), so run the last reset to avoid leaking
> 2042 * anything allocated with disksize_store()
> 2043 */
> --> 2044 zram_reset_device(zram);
>
> This double frees it.
No.
Inside zram_reset_device(), if init_done()(zram->disksize) is zero, zram_reset_device()
returns immediately, otherwise zram->disksize is cleared and zram_meta_free()
is run in zram_reset_device(). Meantime zram->init_lock protects the
reset and disksize_store().
The 2nd zram_reset_device() can only reset device if disksize_store() sets new
zram->disksize and allocates new meta after the 1st zram_reset_device().
Seems smatch static checker need to be improved to cover this case?
Thanks,
Ming
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] zram: avoid race between zram_remove and disksize_store
2021-11-05 0:23 ` Ming Lei
@ 2021-11-05 9:18 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2021-11-05 9:18 UTC (permalink / raw)
To: Ming Lei; +Cc: Sergey Senozhatsky, linux-block
On Fri, Nov 05, 2021 at 08:23:26AM +0800, Ming Lei wrote:
> > --> 2044 zram_reset_device(zram);
> >
> > This double frees it.
>
> No.
>
> Inside zram_reset_device(), if init_done()(zram->disksize) is zero, zram_reset_device()
> returns immediately, otherwise zram->disksize is cleared and zram_meta_free()
> is run in zram_reset_device(). Meantime zram->init_lock protects the
> reset and disksize_store().
>
> The 2nd zram_reset_device() can only reset device if disksize_store() sets new
> zram->disksize and allocates new meta after the 1st zram_reset_device().
>
> Seems smatch static checker need to be improved to cover this case?
Yep. It's a bug in Smatch. It's supposed to parse this correctly.
I will investigate. Thanks!
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-05 9:18 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-11-04 11:48 [bug report] zram: avoid race between zram_remove and disksize_store Dan Carpenter
2021-11-04 11:49 ` Dan Carpenter
2021-11-05 0:23 ` Ming Lei
2021-11-05 9:18 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).