Reviewed-by: thornber@xxxxxxxxxx
On Tue, Dec 3, 2024 at 5:47 PM Ming-Hung Tsai <mtsai@xxxxxxxxxx> wrote:
>
> When dm_bm_read_lock() fails due to locking or checksum errors, it
> releases the faulty block implicitly while leaving an invalid output
> pointer behind. The caller of dm_bm_read_lock() should not operate on
> this invalid dm_block pointer, or it will lead to undefined result.
> For example, the dm_array_cursor incorrectly caches the invalid pointer
> on reading a faulty array block, causing a double release in
> dm_array_cursor_end(), then hitting the BUG_ON in dm-bufio cache_put().
>
> Reproduce steps:
>
> 1. initialize a cache device
>
> 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
> dmsetup create cache --table "0 524288 cache /dev/mapper/cmeta \
> /dev/mapper/cdata /dev/mapper/corig 128 2 metadata2 writethrough smq 0"
>
> 2. wipe the second array block offline
>
> dmsteup remove cache cmeta cdata corig
> mapping_root=$(dd if=/dev/sdc bs=1c count=8 skip=192 \
> 2>/dev/null | hexdump -e '1/8 "%u\n"')
> ablock=$(dd if=/dev/sdc bs=1c count=8 skip=$((4096*mapping_root+2056)) \
> 2>/dev/null | hexdump -e '1/8 "%u\n"')
> dd if=/dev/zero of=/dev/sdc bs=4k count=1 seek=$ablock
>
> 3. try reopen the cache device
>
> 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"
> dmsetup create cache --table "0 524288 cache /dev/mapper/cmeta \
> /dev/mapper/cdata /dev/mapper/corig 128 2 metadata2 writethrough smq 0"
>
> Kernel logs:
>
> (snip)
> device-mapper: array: array_block_check failed: blocknr 0 != wanted 10
> device-mapper: block manager: array validator check failed for block 10
> device-mapper: array: get_ablock failed
> device-mapper: cache metadata: dm_array_cursor_next for mapping failed
> ------------[ cut here ]------------
> kernel BUG at drivers/md/dm-bufio.c:638!
>
> Fix by setting the cached block pointer to NULL on errors.
>
> Signed-off-by: Ming-Hung Tsai <mtsai@xxxxxxxxxx>
> Fixes: fdd1315aa5f0 ("dm array: introduce cursor api")
> ---
> drivers/md/persistent-data/dm-array.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/persistent-data/dm-array.c b/drivers/md/persistent-data/dm-array.c
> index 157c9bd2fed7..c46a374f7459 100644
> --- a/drivers/md/persistent-data/dm-array.c
> +++ b/drivers/md/persistent-data/dm-array.c
> @@ -917,23 +917,28 @@ static int load_ablock(struct dm_array_cursor *c)
> if (c->block)
> unlock_ablock(c->info, c->block);
>
> - c->block = NULL;
> - c->ab = NULL;
> c->index = 0;
>
> r = dm_btree_cursor_get_value(&c->cursor, &key, &value_le);
> if (r) {
> DMERR("dm_btree_cursor_get_value failed");
> dm_btree_cursor_end(&c->cursor);
> + goto out;
>
> } else {
> r = get_ablock(c->info, le64_to_cpu(value_le), &c->block, &c->ab);
> if (r) {
> DMERR("get_ablock failed");
> dm_btree_cursor_end(&c->cursor);
> + goto out;
> }
> }
>
> + return 0;
> +
> +out:
> + c->block = NULL;
> + c->ab = NULL;
> return r;
> }
>
> --
> 2.47.0
>
