* [PATCH RFC 1/2] dm persistent-data: add btree traversal depth limit to detect metadata corruption
2026-08-20 2:24 [PATCH RFC 0/2] dm persistent-data: detect btree cycles and record space-map errors Ye Bin
@ 2026-08-20 2:24 ` Ye Bin
2026-08-20 2:24 ` [PATCH RFC 2/2] dm persistent-data: record space-map errors in transaction manager Ye Bin
1 sibling, 0 replies; 3+ messages in thread
From: Ye Bin @ 2026-08-20 2:24 UTC (permalink / raw)
To: agk, snitzer, mpatocka, bmarzins, dm-devel; +Cc: yebin, yebin10
From: Ye Bin <yebin10@huawei.com>
The btree traversal loops in dm-persistent-data are unbounded do/while
or for(;;) loops. When metadata is corrupted such that a node's value
points back to an ancestor (or to itself), these loops never terminate,
leaving the kernel hung in an uninterruptible state.
Add a DM_BTREE_MAX_DEPTH (16) counter to all unbounded traversal loops:
- btree_lookup_raw() (read path)
- btree_insert_raw() (insert path)
- __btree_get_overwrite_leaf() (overwrite path)
- remove_raw() (remove path)
- remove_nearest() (remove_leaves path)
- find_key() (find_highest/lowest_key path)
When the depth limit is exceeded, return -ELOOP and log a rate-limited
error indicating possible metadata corruption.
The cursor path (find_leaf) is already protected by
DM_BTREE_CURSOR_MAX_DEPTH in push_node(). The dm_btree_del() path is
already protected by MAX_SPINE_DEPTH (64) and __check_holder().
In dm-thin.c, handle -ELOOP from dm_thin_find_block() in all three
call sites (process_cell, __process_bio_read_only, and thin_bio_map) by
calling metadata_operation_failed() to abort the transaction and
downgrade the pool to read-only mode, preventing further writes to the
corrupted metadata.
16 levels can address well over 200^16 entries, far exceeding any
practical thin pool size, so the limit never affects valid metadata.
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
drivers/md/dm-thin.c | 9 +++++++
drivers/md/persistent-data/dm-btree-remove.c | 14 ++++++++++
drivers/md/persistent-data/dm-btree.c | 28 ++++++++++++++++++++
drivers/md/persistent-data/dm-btree.h | 8 ++++++
4 files changed, 59 insertions(+)
diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 59392de7a477..6492c6c3852d 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -1997,6 +1997,9 @@ static void process_cell(struct thin_c *tc, struct dm_bio_prison_cell *cell)
default:
DMERR_LIMIT("%s: dm_thin_find_block() failed: error = %d",
__func__, r);
+ if (r == -ELOOP)
+ metadata_operation_failed(pool,
+ "btree cycle detected", r);
cell_defer_no_holder(tc, cell);
bio_io_error(bio);
break;
@@ -2065,6 +2068,9 @@ static void __process_bio_read_only(struct thin_c *tc, struct bio *bio,
default:
DMERR_LIMIT("%s: dm_thin_find_block() failed: error = %d",
__func__, r);
+ if (r == -ELOOP)
+ metadata_operation_failed(tc->pool,
+ "btree cycle detected", r);
if (cell)
cell_defer_no_holder(tc, cell);
bio_io_error(bio);
@@ -2802,6 +2808,9 @@ static int thin_bio_map(struct dm_target *ti, struct bio *bio)
* dm_thin_find_block can fail with -EINVAL if the
* pool is switched to fail-io mode.
*/
+ if (r == -ELOOP)
+ metadata_operation_failed(tc->pool,
+ "btree cycle detected", r);
bio_io_error(bio);
cell_defer_no_holder(tc, virt_cell);
return DM_MAPIO_SUBMITTED;
diff --git a/drivers/md/persistent-data/dm-btree-remove.c b/drivers/md/persistent-data/dm-btree-remove.c
index aeec5b9a1dd5..7e0560f71729 100644
--- a/drivers/md/persistent-data/dm-btree-remove.c
+++ b/drivers/md/persistent-data/dm-btree-remove.c
@@ -555,8 +555,15 @@ static int remove_raw(struct shadow_spine *s, struct dm_btree_info *info,
{
int i = *index, r;
struct btree_node *n;
+ unsigned int depth = 0;
for (;;) {
+ if (depth++ >= DM_BTREE_MAX_DEPTH) {
+ DMERR_LIMIT("%s: exceeded max depth (%u), possible metadata corruption",
+ __func__, DM_BTREE_MAX_DEPTH);
+ return -ELOOP;
+ }
+
r = shadow_step(s, root, vt);
if (r < 0)
break;
@@ -649,8 +656,15 @@ static int remove_nearest(struct shadow_spine *s, struct dm_btree_info *info,
{
int i = *index, r;
struct btree_node *n;
+ unsigned int depth = 0;
for (;;) {
+ if (depth++ >= DM_BTREE_MAX_DEPTH) {
+ DMERR_LIMIT("%s: exceeded max depth (%u), possible metadata corruption",
+ __func__, DM_BTREE_MAX_DEPTH);
+ return -ELOOP;
+ }
+
r = shadow_step(s, root, vt);
if (r < 0)
break;
diff --git a/drivers/md/persistent-data/dm-btree.c b/drivers/md/persistent-data/dm-btree.c
index dd02eee4a23c..3004537c75a1 100644
--- a/drivers/md/persistent-data/dm-btree.c
+++ b/drivers/md/persistent-data/dm-btree.c
@@ -346,8 +346,15 @@ static int btree_lookup_raw(struct ro_spine *s, dm_block_t block, uint64_t key,
{
int i, r;
uint32_t flags, nr_entries;
+ unsigned int depth = 0;
do {
+ if (depth++ >= DM_BTREE_MAX_DEPTH) {
+ DMERR_LIMIT("%s: exceeded max depth (%u), possible metadata corruption",
+ __func__, DM_BTREE_MAX_DEPTH);
+ return -ELOOP;
+ }
+
r = ro_step(s, block);
if (r < 0)
return r;
@@ -1095,8 +1102,15 @@ static int btree_insert_raw(struct shadow_spine *s, dm_block_t root,
{
int r, i = *index, top = 1;
struct btree_node *node;
+ unsigned int depth = 0;
for (;;) {
+ if (depth++ >= DM_BTREE_MAX_DEPTH) {
+ DMERR_LIMIT("%s: exceeded max depth (%u), possible metadata corruption",
+ __func__, DM_BTREE_MAX_DEPTH);
+ return -ELOOP;
+ }
+
r = shadow_step(s, root, vt);
if (r < 0)
return r;
@@ -1158,9 +1172,16 @@ static int __btree_get_overwrite_leaf(struct shadow_spine *s, dm_block_t root,
{
int r, i = -1;
struct btree_node *node;
+ unsigned int depth = 0;
*index = 0;
for (;;) {
+ if (depth++ >= DM_BTREE_MAX_DEPTH) {
+ DMERR_LIMIT("%s: exceeded max depth (%u), possible metadata corruption",
+ __func__, DM_BTREE_MAX_DEPTH);
+ return -ELOOP;
+ }
+
r = shadow_step(s, root, &s->info->value_type);
if (r < 0)
return r;
@@ -1342,8 +1363,15 @@ static int find_key(struct ro_spine *s, dm_block_t block, bool find_highest,
{
int i, r;
uint32_t flags;
+ unsigned int depth = 0;
do {
+ if (depth++ >= DM_BTREE_MAX_DEPTH) {
+ DMERR_LIMIT("%s: exceeded max depth (%u), possible metadata corruption",
+ __func__, DM_BTREE_MAX_DEPTH);
+ return -ELOOP;
+ }
+
r = ro_step(s, block);
if (r < 0)
return r;
diff --git a/drivers/md/persistent-data/dm-btree.h b/drivers/md/persistent-data/dm-btree.h
index 1b92acd7823d..617d808ca8dd 100644
--- a/drivers/md/persistent-data/dm-btree.h
+++ b/drivers/md/persistent-data/dm-btree.h
@@ -180,6 +180,14 @@ int dm_btree_walk(struct dm_btree_info *info, dm_block_t root,
/*----------------------------------------------------------------*/
+/*
+ * Maximum depth of btree traversal. Used to detect cycles caused by
+ * metadata corruption (e.g. a node whose value points back to itself).
+ * 16 levels can address well over 200^16 entries, far exceeding any
+ * practical thin pool size.
+ */
+#define DM_BTREE_MAX_DEPTH 16
+
/*
* Cursor API. This does not follow the rolling lock convention. Since we
* know the order that values are required we can issue prefetches to speed
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH RFC 2/2] dm persistent-data: record space-map errors in transaction manager
2026-08-20 2:24 [PATCH RFC 0/2] dm persistent-data: detect btree cycles and record space-map errors Ye Bin
2026-08-20 2:24 ` [PATCH RFC 1/2] dm persistent-data: add btree traversal depth limit to detect metadata corruption Ye Bin
@ 2026-08-20 2:24 ` Ye Bin
1 sibling, 0 replies; 3+ messages in thread
From: Ye Bin @ 2026-08-20 2:24 UTC (permalink / raw)
To: agk, snitzer, mpatocka, bmarzins, dm-devel; +Cc: yebin, yebin10
From: Ye Bin <yebin10@huawei.com>
dm_tm_inc(), dm_tm_dec(), dm_tm_inc_range(), and dm_tm_dec_range() have
void return types, so when the underlying space-map operations
(dm_sm_inc_block/dec_block/inc_blocks/dec_blocks) fail, the error is
silently swallowed. The transaction proceeds and may be committed with
inconsistent reference counts, leading to:
1. data_block_dec failure during device deletion: a data block's
refcount is not decremented, causing a permanent space leak that
is silently persisted on commit.
2. inc_children failure during shadow of a shared node: a child
node's refcount is not incremented. When the old shared parent is
later released, the child's refcount drops to zero prematurely.
The block allocator can then reuse that child block as a shadow
copy of the parent (which still contains a pointer to it), creating
a self-referencing node that causes infinite loops in all btree
traversal paths (mitigated by the depth-limit patch, but the
underlying corruption remains).
3. dm_tm_dec failure during rebalance merge: an internal node is not
freed, causing a metadata space leak.
Fix this without changing any function signatures:
- Add a sticky int sm_error field to struct dm_transaction_manager.
dm_tm_inc/dm_tm_dec/dm_tm_inc_range/dm_tm_dec_range record the first
error in sm_error. Since all kernel error return values are
non-zero, sm_error == 0 unambiguously means "no error".
- dm_tm_pre_commit() and dm_tm_commit() check sm_error and refuse to
commit if it is non-zero, returning the recorded error.
- dm_tm_clear_error() resets sm_error to 0. Called in
__begin_transaction() at the start of each new transaction so a
fresh transaction can proceed after the previous one was aborted.
Similarly, the data space-map operations (data_block_inc/data_block_dec
in dm-thin-metadata.c) call dm_sm_inc_blocks/dm_sm_dec_blocks() directly
via with_runs(), bypassing the transaction manager entirely. Add a
parallel int data_sm_error field to struct dm_pool_metadata:
- with_runs() is changed to return int so errors propagate.
- data_block_inc/data_block_dec record the first error in
data_sm_error.
- __commit_transaction() checks data_sm_error and refuses to commit
if it is non-zero.
- __begin_transaction() and dm_pool_abort_metadata() clear
data_sm_error so a fresh transaction can proceed after an abort.
The sm_error and data_sm_error fields are accessed only under
pmd->root_lock (write-locked for commit/abort, read-locked for
inc/dec during metadata operations), so no additional locking is
needed.
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
drivers/md/dm-thin-metadata.c | 65 +++++++++++++---
.../persistent-data/dm-transaction-manager.c | 74 ++++++++++++++++++-
.../persistent-data/dm-transaction-manager.h | 6 ++
3 files changed, 132 insertions(+), 13 deletions(-)
diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c
index e60e1326376a..e7e7facc8d89 100644
--- a/drivers/md/dm-thin-metadata.c
+++ b/drivers/md/dm-thin-metadata.c
@@ -227,6 +227,16 @@ struct dm_pool_metadata {
*/
__u8 data_space_map_root[SPACE_MAP_ROOT_SIZE];
__u8 metadata_space_map_root[SPACE_MAP_ROOT_SIZE];
+
+ /*
+ * Sticky error flag for data space-map operations (inc/dec)
+ * that were silently swallowed by the void value_type inc/dec
+ * callbacks. A non-zero value indicates the data space map is
+ * inconsistent and the transaction must not be committed.
+ * Checked in __commit_transaction(); cleared in
+ * __begin_transaction() and dm_pool_abort_metadata().
+ */
+ int data_sm_error;
};
struct dm_thin_device {
@@ -324,12 +334,13 @@ static void unpack_block_time(uint64_t v, dm_block_t *b, uint32_t *t)
*/
typedef int (*run_fn)(struct dm_space_map *, dm_block_t, dm_block_t);
-static void with_runs(struct dm_space_map *sm, const __le64 *value_le, unsigned int count, run_fn fn)
+static int with_runs(struct dm_space_map *sm, const __le64 *value_le, unsigned int count, run_fn fn)
{
uint64_t b, begin, end;
uint32_t t;
bool in_run = false;
unsigned int i;
+ int r = 0;
for (i = 0; i < count; i++, value_le++) {
/* We know value_le is 8 byte aligned */
@@ -339,7 +350,9 @@ static void with_runs(struct dm_space_map *sm, const __le64 *value_le, unsigned
if (b == end) {
end++;
} else {
- fn(sm, begin, end);
+ r = fn(sm, begin, end);
+ if (r)
+ return r;
begin = b;
end = b + 1;
}
@@ -351,19 +364,35 @@ static void with_runs(struct dm_space_map *sm, const __le64 *value_le, unsigned
}
if (in_run)
- fn(sm, begin, end);
+ r = fn(sm, begin, end);
+
+ return r;
}
static void data_block_inc(void *context, const void *value_le, unsigned int count)
{
- with_runs((struct dm_space_map *) context,
- (const __le64 *) value_le, count, dm_sm_inc_blocks);
+ struct dm_pool_metadata *pmd = context;
+ int r;
+
+ r = with_runs(pmd->data_sm, (const __le64 *) value_le,
+ count, dm_sm_inc_blocks);
+ if (r && !pmd->data_sm_error) {
+ DMERR_LIMIT("data_block_inc failed: error %d", r);
+ pmd->data_sm_error = r;
+ }
}
static void data_block_dec(void *context, const void *value_le, unsigned int count)
{
- with_runs((struct dm_space_map *) context,
- (const __le64 *) value_le, count, dm_sm_dec_blocks);
+ struct dm_pool_metadata *pmd = context;
+ int r;
+
+ r = with_runs(pmd->data_sm, (const __le64 *) value_le,
+ count, dm_sm_dec_blocks);
+ if (r && !pmd->data_sm_error) {
+ DMERR_LIMIT("data_block_dec failed: error %d", r);
+ pmd->data_sm_error = r;
+ }
}
static int data_block_equal(void *context, const void *value1_le, const void *value2_le)
@@ -485,7 +514,7 @@ static void __setup_btree_details(struct dm_pool_metadata *pmd)
{
pmd->info.tm = pmd->tm;
pmd->info.levels = 2;
- pmd->info.value_type.context = pmd->data_sm;
+ pmd->info.value_type.context = pmd;
pmd->info.value_type.size = sizeof(__le64);
pmd->info.value_type.inc = data_block_inc;
pmd->info.value_type.dec = data_block_dec;
@@ -504,7 +533,7 @@ static void __setup_btree_details(struct dm_pool_metadata *pmd)
pmd->bl_info.tm = pmd->tm;
pmd->bl_info.levels = 1;
- pmd->bl_info.value_type.context = pmd->data_sm;
+ pmd->bl_info.value_type.context = pmd;
pmd->bl_info.value_type.size = sizeof(__le64);
pmd->bl_info.value_type.inc = data_block_inc;
pmd->bl_info.value_type.dec = data_block_dec;
@@ -846,6 +875,15 @@ static int __begin_transaction(struct dm_pool_metadata *pmd)
pmd->data_block_size = le32_to_cpu(disk_super->data_block_size);
dm_bm_unlock(sblock);
+
+ /*
+ * Start the new transaction with a clean error slate. Any
+ * space-map errors from the previous (aborted) transaction
+ * must not carry over.
+ */
+ dm_tm_clear_error(pmd->tm);
+ pmd->data_sm_error = 0;
+
return 0;
}
@@ -899,6 +937,12 @@ static int __commit_transaction(struct dm_pool_metadata *pmd)
if (unlikely(!pmd->in_service))
return 0;
+ if (pmd->data_sm_error) {
+ DMERR("aborting commit due to earlier data space-map error: %d",
+ pmd->data_sm_error);
+ return pmd->data_sm_error;
+ }
+
if (pmd->pre_commit_fn) {
r = pmd->pre_commit_fn(pmd->pre_commit_context);
if (r < 0) {
@@ -976,6 +1020,7 @@ struct dm_pool_metadata *dm_pool_metadata_open(struct block_device *bdev,
pmd->data_block_size = data_block_size;
pmd->pre_commit_fn = NULL;
pmd->pre_commit_context = NULL;
+ pmd->data_sm_error = 0;
r = __create_persistent_data_objects(pmd, format_device);
if (r) {
@@ -1894,6 +1939,8 @@ int dm_pool_abort_metadata(struct dm_pool_metadata *pmd)
r = __open_or_format_metadata(pmd, false);
if (r)
pmd->fail_io = true;
+ else
+ pmd->data_sm_error = 0;
pmd_write_unlock(pmd);
return r;
}
diff --git a/drivers/md/persistent-data/dm-transaction-manager.c b/drivers/md/persistent-data/dm-transaction-manager.c
index 7401add01d12..035b554289cb 100644
--- a/drivers/md/persistent-data/dm-transaction-manager.c
+++ b/drivers/md/persistent-data/dm-transaction-manager.c
@@ -99,6 +99,16 @@ struct dm_transaction_manager {
struct rb_root buckets[DM_HASH_SIZE];
struct prefetch_set prefetches;
+
+ /*
+ * Sticky error flag for space-map operations (inc/dec) that
+ * were silently swallowed by the void tm_inc/tm_dec wrappers.
+ * A non-zero value indicates the transaction is inconsistent
+ * and must not be committed. Checked in tm_pre_commit() and
+ * tm_commit(); cleared by dm_tm_clear_error() at the start of
+ * each new transaction.
+ */
+ int sm_error;
};
/*----------------------------------------------------------------*/
@@ -193,6 +203,7 @@ static struct dm_transaction_manager *dm_tm_create(struct dm_block_manager *bm,
tm->real = NULL;
tm->bm = bm;
tm->sm = sm;
+ tm->sm_error = 0;
spin_lock_init(&tm->lock);
for (i = 0; i < DM_HASH_SIZE; i++)
@@ -236,6 +247,12 @@ int dm_tm_pre_commit(struct dm_transaction_manager *tm)
if (tm->is_clone)
return -EWOULDBLOCK;
+ if (tm->sm_error) {
+ DMERR("aborting pre-commit due to earlier space-map error: %d",
+ tm->sm_error);
+ return tm->sm_error;
+ }
+
r = dm_sm_commit(tm->sm);
if (r < 0)
return r;
@@ -249,6 +266,13 @@ int dm_tm_commit(struct dm_transaction_manager *tm, struct dm_block *root)
if (tm->is_clone)
return -EWOULDBLOCK;
+ if (tm->sm_error) {
+ DMERR("refusing commit due to earlier space-map error: %d",
+ tm->sm_error);
+ dm_bm_unlock(root);
+ return tm->sm_error;
+ }
+
wipe_shadow_table(tm);
dm_bm_unlock(root);
@@ -375,45 +399,87 @@ EXPORT_SYMBOL_GPL(dm_tm_unlock);
void dm_tm_inc(struct dm_transaction_manager *tm, dm_block_t b)
{
+ int r;
+
/*
* The non-blocking clone doesn't support this.
*/
BUG_ON(tm->is_clone);
- dm_sm_inc_block(tm->sm, b);
+ r = dm_sm_inc_block(tm->sm, b);
+ if (r && !tm->sm_error) {
+ DMERR_LIMIT("dm_tm_inc failed for block %llu: error %d",
+ (unsigned long long)b, r);
+ tm->sm_error = r;
+ }
}
EXPORT_SYMBOL_GPL(dm_tm_inc);
void dm_tm_inc_range(struct dm_transaction_manager *tm, dm_block_t b, dm_block_t e)
{
+ int r;
+
/*
* The non-blocking clone doesn't support this.
*/
BUG_ON(tm->is_clone);
- dm_sm_inc_blocks(tm->sm, b, e);
+ r = dm_sm_inc_blocks(tm->sm, b, e);
+ if (r && !tm->sm_error) {
+ DMERR_LIMIT("dm_tm_inc_range failed for blocks %llu..%llu: error %d",
+ (unsigned long long)b, (unsigned long long)e, r);
+ tm->sm_error = r;
+ }
}
EXPORT_SYMBOL_GPL(dm_tm_inc_range);
void dm_tm_dec(struct dm_transaction_manager *tm, dm_block_t b)
{
+ int r;
+
/*
* The non-blocking clone doesn't support this.
*/
BUG_ON(tm->is_clone);
- dm_sm_dec_block(tm->sm, b);
+ r = dm_sm_dec_block(tm->sm, b);
+ if (r && !tm->sm_error) {
+ DMERR_LIMIT("dm_tm_dec failed for block %llu: error %d",
+ (unsigned long long)b, r);
+ tm->sm_error = r;
+ }
}
EXPORT_SYMBOL_GPL(dm_tm_dec);
+/*
+ * Clear the sticky space-map error flag. Called at the start of each
+ * new transaction (in __begin_transaction) so that a fresh transaction
+ * can proceed after the previous one was aborted.
+ */
+void dm_tm_clear_error(struct dm_transaction_manager *tm)
+{
+ if (tm->is_clone)
+ return;
+
+ tm->sm_error = 0;
+}
+EXPORT_SYMBOL_GPL(dm_tm_clear_error);
+
void dm_tm_dec_range(struct dm_transaction_manager *tm, dm_block_t b, dm_block_t e)
{
+ int r;
+
/*
* The non-blocking clone doesn't support this.
*/
BUG_ON(tm->is_clone);
- dm_sm_dec_blocks(tm->sm, b, e);
+ r = dm_sm_dec_blocks(tm->sm, b, e);
+ if (r && !tm->sm_error) {
+ DMERR_LIMIT("dm_tm_dec_range failed for blocks %llu..%llu: error %d",
+ (unsigned long long)b, (unsigned long long)e, r);
+ tm->sm_error = r;
+ }
}
EXPORT_SYMBOL_GPL(dm_tm_dec_range);
diff --git a/drivers/md/persistent-data/dm-transaction-manager.h b/drivers/md/persistent-data/dm-transaction-manager.h
index 61a8d10825ca..f3c50c3f9964 100644
--- a/drivers/md/persistent-data/dm-transaction-manager.h
+++ b/drivers/md/persistent-data/dm-transaction-manager.h
@@ -105,6 +105,12 @@ void dm_tm_inc_range(struct dm_transaction_manager *tm, dm_block_t b, dm_block_t
void dm_tm_dec(struct dm_transaction_manager *tm, dm_block_t b);
void dm_tm_dec_range(struct dm_transaction_manager *tm, dm_block_t b, dm_block_t e);
+/*
+ * Clear the sticky space-map error flag. Should be called at the start
+ * of each new transaction.
+ */
+void dm_tm_clear_error(struct dm_transaction_manager *tm);
+
/*
* Builds up runs of adjacent blocks, and then calls the given fn
* (typically dm_tm_inc/dec). Very useful when you have to perform
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread