From: Ming Lei <ming.lei@redhat.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>,
linux-block@vger.kernel.org, ming.lei@redhat.com
Subject: Re: [bug report] zram: avoid race between zram_remove and disksize_store
Date: Fri, 5 Nov 2021 08:23:26 +0800 [thread overview]
Message-ID: <YYR5fmwmfvfQzWuZ@T590> (raw)
In-Reply-To: <20211104114830.GA4962@kili>
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
next prev parent reply other threads:[~2021-11-05 0:23 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2021-11-05 9:18 ` Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YYR5fmwmfvfQzWuZ@T590 \
--to=ming.lei@redhat.com \
--cc=dan.carpenter@oracle.com \
--cc=linux-block@vger.kernel.org \
--cc=senozhatsky@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox