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
