| Message ID | 20241022071354.778749-1-mtsai@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mikulas Patocka |
| Headers | show |
| Series | dm cache: fix OOB and error handling in cache creation and resizing | expand |
ack On Tue, Oct 22, 2024 at 8:14 AM Ming-Hung Tsai <mtsai@redhat.com> wrote: > > Out-of-bounds access occurs if the fast device is expanded unexpectedly > before the first-time resume of the cache table. This happens because > expanding the fast device requires reloading the cache table for > cache_create to allocate new in-core data structures that fit the new > size, and the check in cache_preresume is not performed during the > first resume, leading to the issue. > > Reproduce steps: > > 1. prepare component devices: > > dmsetup create cmeta --table "0 8192 linear /dev/sdc 0" > dmsetup create cdata --table "0 65536 linear /dev/sdc 8192" > dmsetup create corig --table "0 524288 linear /dev/sdc 262144" > dd if=/dev/zero of=/dev/mapper/cmeta bs=4k count=1 oflag=direct > > 2. load a cache table of 512 cache blocks, and deliberately expand the > fast device before resuming the cache, making the in-core data > structures inadequate. > > dmsetup create cache --notable > dmsetup reload cache --table "0 524288 cache /dev/mapper/cmeta \ > /dev/mapper/cdata /dev/mapper/corig 128 2 metadata2 writethrough smq 0" > dmsetup reload cdata --table "0 131072 linear /dev/sdc 8192" > dmsetup resume cdata > dmsetup resume cache > > 3. suspend the cache to write out the in-core dirty bitset and hint > array, leading to out-of-bounds access to the dirty bitset at offset > 0x40: > > dmsetup suspend cache > > KASAN reports: > > BUG: KASAN: vmalloc-out-of-bounds in is_dirty_callback+0x2b/0x80 > Read of size 8 at addr ffffc90000085040 by task dmsetup/90 > > (...snip...) > The buggy address belongs to the virtual mapping at > [ffffc90000085000, ffffc90000087000) created by: > cache_ctr+0x176a/0x35f0 > > (...snip...) > Memory state around the buggy address: > ffffc90000084f00: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 > ffffc90000084f80: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 > >ffffc90000085000: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 f8 f8 f8 f8 > ^ > ffffc90000085080: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 > ffffc90000085100: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 > > Fix by checking the size change on the first resume. > > Signed-off-by: Ming-Hung Tsai <mtsai@redhat.com> > Fixes: f494a9c6b1b6 ("dm cache: cache shrinking support") > --- > drivers/md/dm-cache-target.c | 37 ++++++++++++++++-------------------- > 1 file changed, 16 insertions(+), 21 deletions(-) > > diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c > index fa8ef2c32af8..40709310e327 100644 > --- a/drivers/md/dm-cache-target.c > +++ b/drivers/md/dm-cache-target.c > @@ -2901,24 +2901,24 @@ static dm_cblock_t get_cache_dev_size(struct cache *cache) > static bool can_resize(struct cache *cache, dm_cblock_t new_size) > { > if (from_cblock(new_size) > from_cblock(cache->cache_size)) { > - if (cache->sized) { > - DMERR("%s: unable to extend cache due to missing cache table reload", > - cache_device_name(cache)); > - return false; > - } > + DMERR("%s: unable to extend cache due to missing cache table reload", > + cache_device_name(cache)); > + return false; > } > > /* > * We can't drop a dirty block when shrinking the cache. > */ > - new_size = to_cblock(find_next_bit(cache->dirty_bitset, > - from_cblock(cache->cache_size), > - from_cblock(new_size))); > - if (new_size != cache->cache_size) { > - DMERR("%s: unable to shrink cache; cache block %llu is dirty", > - cache_device_name(cache), > - (unsigned long long) from_cblock(new_size)); > - return false; > + if (cache->loaded_mappings) { > + new_size = to_cblock(find_next_bit(cache->dirty_bitset, > + from_cblock(cache->cache_size), > + from_cblock(new_size))); > + if (new_size != cache->cache_size) { > + DMERR("%s: unable to shrink cache; cache block %llu is dirty", > + cache_device_name(cache), > + (unsigned long long) from_cblock(new_size)); > + return false; > + } > } > > return true; > @@ -2949,20 +2949,15 @@ static int cache_preresume(struct dm_target *ti) > /* > * Check to see if the cache has resized. > */ > - if (!cache->sized) { > - r = resize_cache_dev(cache, csize); > - if (r) > - return r; > - > - cache->sized = true; > - > - } else if (csize != cache->cache_size) { > + if (!cache->sized || csize != cache->cache_size) { > if (!can_resize(cache, csize)) > return -EINVAL; > > r = resize_cache_dev(cache, csize); > if (r) > return r; > + > + cache->sized = true; > } > > if (!cache->loaded_mappings) { > -- > 2.47.0 >
diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c index fa8ef2c32af8..40709310e327 100644 --- a/drivers/md/dm-cache-target.c +++ b/drivers/md/dm-cache-target.c @@ -2901,24 +2901,24 @@ static dm_cblock_t get_cache_dev_size(struct cache *cache) static bool can_resize(struct cache *cache, dm_cblock_t new_size) { if (from_cblock(new_size) > from_cblock(cache->cache_size)) { - if (cache->sized) { - DMERR("%s: unable to extend cache due to missing cache table reload", - cache_device_name(cache)); - return false; - } + DMERR("%s: unable to extend cache due to missing cache table reload", + cache_device_name(cache)); + return false; } /* * We can't drop a dirty block when shrinking the cache. */ - new_size = to_cblock(find_next_bit(cache->dirty_bitset, - from_cblock(cache->cache_size), - from_cblock(new_size))); - if (new_size != cache->cache_size) { - DMERR("%s: unable to shrink cache; cache block %llu is dirty", - cache_device_name(cache), - (unsigned long long) from_cblock(new_size)); - return false; + if (cache->loaded_mappings) { + new_size = to_cblock(find_next_bit(cache->dirty_bitset, + from_cblock(cache->cache_size), + from_cblock(new_size))); + if (new_size != cache->cache_size) { + DMERR("%s: unable to shrink cache; cache block %llu is dirty", + cache_device_name(cache), + (unsigned long long) from_cblock(new_size)); + return false; + } } return true; @@ -2949,20 +2949,15 @@ static int cache_preresume(struct dm_target *ti) /* * Check to see if the cache has resized. */ - if (!cache->sized) { - r = resize_cache_dev(cache, csize); - if (r) - return r; - - cache->sized = true; - - } else if (csize != cache->cache_size) { + if (!cache->sized || csize != cache->cache_size) { if (!can_resize(cache, csize)) return -EINVAL; r = resize_cache_dev(cache, csize); if (r) return r; + + cache->sized = true; } if (!cache->loaded_mappings) {