* [Devel] [PATCH hci-8.1 v5 01/11] dm-ploop: fix u32 overflow of the delta BAT byte size
2026-10-13 7:55 [Devel] [PATCH hci-8.1 v5 00/11] dm-ploop: give back unused tail of top delta on suspend Alexander Ivanov
@ 2026-10-13 7:55 ` Alexander Ivanov
2026-10-13 8:02 ` no-reply
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 02/11] dm-ploop: fix lost wakeup in ploop_allocator() Alexander Ivanov
` (9 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Alexander Ivanov @ 2026-10-13 7:55 UTC (permalink / raw)
To: den; +Cc: devel
The byte size of header and BAT, (PLOOP_MAP_OFFSET + m_Size) *
sizeof(map_index_t), is computed in u32 in several places. It wraps for
m_Size >= 2^30 - PLOOP_MAP_OFFSET, which a delta with small clusters
may legally have.
Compute the size in u64 and use ploop_nr_bat_clusters() for the number
of BAT clusters everywhere.
Reject a header or a resize whose hb_nr exceeds PLOOP_MAX_HB_NR,
which also keeps DIV_ROUND_UP(hb_nr, 8) and the bitmap size in bits
from wrapping.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-ploop-bat.c | 23 ++++++++++++-----------
drivers/md/dm-ploop-cmd.c | 19 +++++++++----------
drivers/md/dm-ploop.h | 6 ++++--
3 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index a7849de8aaa6..5ddb8ffdff07 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -189,7 +189,6 @@ int ploop_setup_metadata(struct ploop *ploop, struct page *page)
struct ploop_pvd_header *m_hdr = NULL;
u32 bat_clusters, offset_clusters;
struct dm_target *ti = ploop->ti;
- unsigned long size;
int ret;
m_hdr = kmap(page);
@@ -213,8 +212,11 @@ int ploop_setup_metadata(struct ploop *ploop, struct page *page)
ploop->nr_bat_entries = le32_to_cpu(m_hdr->m_Size);
/* Header and BAT-occupied clusters at start of file */
- size = (PLOOP_MAP_OFFSET + ploop->nr_bat_entries) * sizeof(map_index_t);
- bat_clusters = DIV_ROUND_UP(size, CLU_SIZE(ploop));
+ bat_clusters = ploop_nr_bat_clusters(ploop, ploop->nr_bat_entries);
+ if ((u64)bat_clusters + ploop->nr_bat_entries > PLOOP_MAX_HB_NR) {
+ PL_ERR("Too large BAT");
+ goto out;
+ }
/* Clusters from start of file to first data block */
offset_clusters = SEC_TO_CLU(ploop, le32_to_cpu(m_hdr->m_FirstBlockOffset));
@@ -242,7 +244,7 @@ static int ploop_delta_check_header(struct ploop *ploop,
struct rb_root *md_root,
u32 *delta_nr_be_ret)
{
- u32 bytes, delta_nr_be, offset_clusters, bat_clusters;
+ u32 delta_nr_be, offset_clusters, bat_clusters;
struct md_page *md0 = ploop_md_first_entry(md_root);
struct ploop_pvd_header *d_hdr;
int ret = -EPROTO;
@@ -257,8 +259,7 @@ static int ploop_delta_check_header(struct ploop *ploop,
delta_nr_be = le32_to_cpu(d_hdr->m_Size);
offset_clusters = SEC_TO_CLU(ploop, le32_to_cpu(d_hdr->m_FirstBlockOffset));
- bytes = (PLOOP_MAP_OFFSET + delta_nr_be) * sizeof(map_index_t);
- bat_clusters = DIV_ROUND_UP(bytes, CLU_SIZE(ploop));
+ bat_clusters = ploop_nr_bat_clusters(ploop, delta_nr_be);
if (delta_nr_be > ploop->nr_bat_entries ||
bat_clusters != offset_clusters)
@@ -274,13 +275,12 @@ ALLOW_ERROR_INJECTION(ploop_delta_check_header, ERRNO);
static int ploop_convert_bat_entries(struct ploop *ploop, struct rb_root *md_root,
u32 nr_be, u32 nr_pages, loff_t file_size)
{
- u32 i, end, bytes, bat_clusters, page_id, *bat_entries, max_file_clu;
+ u32 i, end, bat_clusters, page_id, *bat_entries, max_file_clu;
struct rb_node *node;
struct md_page *md;
int ret = 0;
- bytes = (PLOOP_MAP_OFFSET + nr_be) * sizeof(map_index_t);
- bat_clusters = DIV_ROUND_UP(bytes, CLU_SIZE(ploop));
+ bat_clusters = ploop_nr_bat_clusters(ploop, nr_be);
max_file_clu = file_size / CLU_SIZE(ploop) - 1;
page_id = 0;
@@ -311,12 +311,13 @@ int ploop_read_delta_metadata(struct ploop *ploop, struct file *file,
struct rb_root *md_root, u32 *delta_nr_be_ret)
{
struct bio_vec bvec_on_stack, *bvec = &bvec_on_stack;
- u32 i, size, delta_nr_be, nr_segs;
+ u32 i, delta_nr_be, nr_segs;
loff_t pos, file_size;
struct iov_iter iter;
struct rb_node *node;
struct md_page *md;
ssize_t len;
+ u64 size;
int ret;
ret = -ENOMEM;
@@ -339,7 +340,7 @@ int ploop_read_delta_metadata(struct ploop *ploop, struct file *file,
if (ret)
goto out;
- size = (PLOOP_MAP_OFFSET + delta_nr_be) * sizeof(map_index_t);
+ size = ((u64)delta_nr_be + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
size = ALIGN(size, PAGE_SIZE); /* file may be open as direct */
nr_segs = size / PAGE_SIZE;
diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index 5fa78e6cff9a..ab17520c2d56 100644
--- a/drivers/md/dm-ploop-cmd.c
+++ b/drivers/md/dm-ploop-cmd.c
@@ -386,7 +386,7 @@ ALLOW_ERROR_INJECTION(ploop_grow_relocate_cluster, ERRNO);
static int ploop_grow_update_header(struct ploop *ploop,
struct ploop_cmd *cmd)
{
- unsigned int size, first_block_off;
+ unsigned int first_block_off;
struct ploop_pvd_header *hdr;
struct ploop_index_wb *piwb;
u32 nr_be, offset, clus;
@@ -413,9 +413,7 @@ static int ploop_grow_update_header(struct ploop *ploop,
}
piwb = md->piwb;
- size = (PLOOP_MAP_OFFSET + cmd->resize.nr_bat_entries);
- size *= sizeof(map_index_t);
- clus = DIV_ROUND_UP(size, CLU_SIZE(ploop));
+ clus = ploop_nr_bat_clusters(ploop, cmd->resize.nr_bat_entries);
first_block_off = CLU_TO_SEC(ploop, clus);
hdr = kmap_local_page(piwb->bat_page);
@@ -597,6 +595,11 @@ static int ploop_resize(struct ploop *ploop, sector_t new_sectors)
}
nr_bat_entries = SEC_TO_CLU(ploop, new_sectors);
+ nr_bat_clusters = ploop_nr_bat_clusters(ploop, nr_bat_entries);
+ if ((u64)nr_bat_clusters + nr_bat_entries > PLOOP_MAX_HB_NR) {
+ DMWARN("resize: too large size is requested");
+ return -EINVAL;
+ }
/* Memory for new md pages */
ret = ploop_prealloc_md_pages(&cmd.resize.md_pages_root,
@@ -604,8 +607,6 @@ static int ploop_resize(struct ploop *ploop, sector_t new_sectors)
if (ret)
return ret;
- size = (PLOOP_MAP_OFFSET + nr_bat_entries) * sizeof(map_index_t);
- nr_bat_clusters = DIV_ROUND_UP(size, CLU_SIZE(ploop));
hb_nr = nr_bat_clusters + nr_bat_entries;
size = round_up(DIV_ROUND_UP(hb_nr, 8), sizeof(unsigned long));
@@ -1010,11 +1011,9 @@ static int process_flip_upper_deltas(struct ploop *ploop)
u8 level = ploop_top_level(ploop) - 1;
struct rb_node *node;
struct md_page *md;
- u64 size;
- size = (PLOOP_MAP_OFFSET + READ_ONCE(ploop->nr_bat_entries))
- * sizeof(map_index_t);
- bat_clusters = DIV_ROUND_UP(size, CLU_SIZE(ploop));
+ bat_clusters = ploop_nr_bat_clusters(ploop,
+ READ_ONCE(ploop->nr_bat_entries));
hb_nr = ploop->hb_nr;
/*
diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
index 23662943d56a..5041fc8f1e13 100644
--- a/drivers/md/dm-ploop.h
+++ b/drivers/md/dm-ploop.h
@@ -12,6 +12,8 @@
#include <linux/bio.h>
#define PLOOP_MAP_OFFSET 16
+/* Keeps DIV_ROUND_UP(hb_nr, 8) and clu + PLOOP_MAP_OFFSET within u32 */
+#define PLOOP_MAX_HB_NR round_down(U32_MAX - PLOOP_MAP_OFFSET, 8)
typedef u32 map_index_t;
#define BAT_ENTRIES_PER_PAGE (PAGE_SIZE / sizeof(map_index_t))
@@ -410,9 +412,9 @@ static inline unsigned int ploop_nr_pages_in_cluster(struct ploop *ploop)
static inline unsigned int ploop_nr_bat_clusters(struct ploop *ploop,
u32 nr_bat_entries)
{
- unsigned long size, bat_clusters;
+ u64 size, bat_clusters;
- size = (PLOOP_MAP_OFFSET + nr_bat_entries) * sizeof(map_index_t);
+ size = ((u64)nr_bat_entries + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
bat_clusters = DIV_ROUND_UP(size, CLU_SIZE(ploop));
return bat_clusters;
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Devel] [PATCH hci-8.1 v5 01/11] dm-ploop: fix u32 overflow of the delta BAT byte size
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 01/11] dm-ploop: fix u32 overflow of the delta BAT byte size Alexander Ivanov
@ 2026-10-13 8:02 ` no-reply
2026-10-13 9:24 ` Alexander Ivanov
0 siblings, 1 reply; 25+ messages in thread
From: no-reply @ 2026-10-13 8:02 UTC (permalink / raw)
To: den, Alexander Ivanov; +Cc: devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-ploop: ploop_bat_clu_to_page_nr() still wraps in size_t on 32-bit
--
commit 101e85108bc98e48077f0a87fa099eedec6fc95d
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
dm-ploop: fix u32 overflow of the delta BAT byte size
This commit computes the byte size of the header and BAT in u64 and uses
ploop_nr_bat_clusters() for the number of BAT clusters everywhere. It also
rejects a header or a resize whose hb_nr exceeds PLOOP_MAX_HB_NR.
Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171
> diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
> --- a/drivers/md/dm-ploop.h
> +++ b/drivers/md/dm-ploop.h
> @@ -12,6 +12,8 @@
> #define PLOOP_MAP_OFFSET 16
> +/* Keeps DIV_ROUND_UP(hb_nr, 8) and clu + PLOOP_MAP_OFFSET within u32 */
> +#define PLOOP_MAX_HB_NR round_down(U32_MAX - PLOOP_MAP_OFFSET, 8)
> typedef u32 map_index_t;
[ ... ]
> @@ -410,9 +412,9 @@ static inline unsigned int ploop_nr_pages_in_cluster(struct ploop *ploop)
> static inline unsigned int ploop_nr_bat_clusters(struct ploop *ploop,
> u32 nr_bat_entries)
> {
> - unsigned long size, bat_clusters;
> + u64 size, bat_clusters;
>
> - size = (PLOOP_MAP_OFFSET + nr_bat_entries) * sizeof(map_index_t);
> + size = ((u64)nr_bat_entries + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
> bat_clusters = DIV_ROUND_UP(size, CLU_SIZE(ploop));
>
> return bat_clusters;
[Severity: Low]
The commit message says the u32 overflow is fixed "everywhere", but the
sibling helper ploop_bat_clu_to_page_nr() in this same header still has
the same pattern:
static inline u32 ploop_bat_clu_to_page_nr(u32 clu)
{
u64 byte;
byte = (clu + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
return byte >> PAGE_SHIFT;
}
Here the multiplication is evaluated at size_t width, and only the
result is widened to u64. On a 32-bit build this wraps for
clu >= 2^30 - PLOOP_MAP_OFFSET, which is the same range the commit
message describes.
DM_PLOOP in drivers/md/Kconfig depends only on BLK_DEV_DM, with no 64BIT
restriction, so a 32-bit build appears possible. PLOOP_MAX_HB_NR is
about 2^32 - 24, so ploop_setup_metadata() and ploop_resize() would still
accept a BAT in that range.
Can ploop_prealloc_md_pages() then under-allocate md pages on 32-bit?
ploop_resize()
ploop_prealloc_md_pages()
new_nr_pages = ploop_bat_clu_to_page_nr(new_nr_bat_entries - 1) + 1;
The same wrapped value would also feed init_be_iter(),
ploop_bat_entries() and the other callers of this helper.
This is probably not reachable in practice, since a BAT of 2^30 entries
needs more than 4 GiB of md pages. Would it make sense to cast clu to
u64 before the multiplication, as is done in ploop_nr_bat_clusters(), so
the helpers stay consistent with the commit message?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013075513.90618-1-alexander.ivanov@virtuozzo.com?part=1
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Devel] [PATCH hci-8.1 v5 02/11] dm-ploop: fix lost wakeup in ploop_allocator()
2026-10-13 7:55 [Devel] [PATCH hci-8.1 v5 00/11] dm-ploop: give back unused tail of top delta on suspend Alexander Ivanov
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 01/11] dm-ploop: fix u32 overflow of the delta BAT byte size Alexander Ivanov
@ 2026-10-13 7:55 ` Alexander Ivanov
2026-10-13 8:00 ` no-reply
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 03/11] dm-ploop: reject a delta header whose BAT does not cover the target Alexander Ivanov
` (8 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Alexander Ivanov @ 2026-10-13 7:55 UTC (permalink / raw)
To: den; +Cc: devel
ploop_allocator() checks prealloc_size and kthread_should_stop() before
setting TASK_INTERRUPTIBLE. If ploop_req_prealloc() sets prealloc_size
and calls wake_up_process() in between, the wakeup finds the task
running and is lost, so the allocator goes to sleep with a pending
preallocation request. Later requests for the same size return early
without waking it up, so the request may stay pending forever and
waiters on dispatcher_wq_prealloc hang. The same applies to
kthread_stop().
Set the task state before checking the conditions, as the canonical
kthread loop does. set_current_state() implies a full barrier, which
pairs with the one in try_to_wake_up().
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
drivers/md/dm-ploop-map.c | 44 ++++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 48466435dd19..92ceadf41b42 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -2347,31 +2347,33 @@ int ploop_allocator(void *data)
struct file *file;
for (;;) {
- /* we do not care about exact number here - risk one extra schedule wrt taking the lock */
- if (ploop->prealloc_size) {
- __set_current_state(TASK_RUNNING);
- file = ploop_top_delta(ploop)->file;
- current->flags |= PLOOP_PFLAGS;
- ret = ploop_preallocate_cluster(ploop, file);
- if (ret) {
- /*
- * threads requesting prealloc must check
- * if size actually changed
- */
- /* ENOSPC is only for fallocate */
- pr_err_ratelimited(
- PL_FMT("failed to allocate space: %d\n"),
- ploop_device_name(ploop), ret);
- }
- wake_up_interruptible(&ploop->dispatcher_wq_prealloc);
- current_restore_flags(old_flags, PLOOP_PFLAGS);
- }
+ set_current_state(TASK_INTERRUPTIBLE);
if (kthread_should_stop()) {
__set_current_state(TASK_RUNNING);
break;
}
- set_current_state(TASK_INTERRUPTIBLE);
- schedule();
+ /* we do not care about exact number here - risk one extra schedule wrt taking the lock */
+ if (!READ_ONCE(ploop->prealloc_size)) {
+ schedule();
+ continue;
+ }
+ __set_current_state(TASK_RUNNING);
+
+ file = ploop_top_delta(ploop)->file;
+ current->flags |= PLOOP_PFLAGS;
+ ret = ploop_preallocate_cluster(ploop, file);
+ if (ret) {
+ /*
+ * threads requesting prealloc must check
+ * if size actually changed
+ */
+ /* ENOSPC is only for fallocate */
+ pr_err_ratelimited(
+ PL_FMT("failed to allocate space: %d\n"),
+ ploop_device_name(ploop), ret);
+ }
+ wake_up_interruptible(&ploop->dispatcher_wq_prealloc);
+ current_restore_flags(old_flags, PLOOP_PFLAGS);
}
return 0;
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread* [Devel] [PATCH hci-8.1 v5 03/11] dm-ploop: reject a delta header whose BAT does not cover the target
2026-10-13 7:55 [Devel] [PATCH hci-8.1 v5 00/11] dm-ploop: give back unused tail of top delta on suspend Alexander Ivanov
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 01/11] dm-ploop: fix u32 overflow of the delta BAT byte size Alexander Ivanov
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 02/11] dm-ploop: fix lost wakeup in ploop_allocator() Alexander Ivanov
@ 2026-10-13 7:55 ` Alexander Ivanov
2026-10-13 8:01 ` no-reply
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 04/11] dm-ploop: reject empty deltas Alexander Ivanov
` (7 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Alexander Ivanov @ 2026-10-13 7:55 UTC (permalink / raw)
To: den; +Cc: devel
ploop_setup_metadata() takes the number of BAT entries from m_Size of
the top delta header, but checks only m_SizeInSectors_v2 against the
target length. A header with a small m_Size (including zero) and a large
enough m_SizeInSectors_v2 passes all checks. Requests past the last BAT
entry then hit WARN_ONCE() in ploop_rq_valid(), which panics the host
with panic_on_warn.
Reject the header if the BAT does not cover the target, taking the
skip_off shift of request positions into account.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-ploop-bat.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 5ddb8ffdff07..7b12b0f9fe81 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -10,6 +10,7 @@
#include <linux/uio.h>
#include <linux/error-injection.h>
#include <linux/mm.h>
+#include <linux/overflow.h>
#include "dm-ploop.h"
struct md_page *ploop_md_page_find(struct ploop *ploop, u32 id)
@@ -189,6 +190,7 @@ int ploop_setup_metadata(struct ploop *ploop, struct page *page)
struct ploop_pvd_header *m_hdr = NULL;
u32 bat_clusters, offset_clusters;
struct dm_target *ti = ploop->ti;
+ u64 end;
int ret;
m_hdr = kmap(page);
@@ -229,6 +231,11 @@ int ploop_setup_metadata(struct ploop *ploop, struct page *page)
PL_ERR("Too short BAT");
goto out;
}
+ if (check_add_overflow((u64)ti->len, ploop->skip_off, &end) ||
+ SEC_TO_CLU(ploop, end - 1) >= ploop->nr_bat_entries) {
+ PL_ERR("BAT does not cover the target");
+ goto out;
+ }
kunmap(page);
m_hdr = NULL;
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread* [Devel] [PATCH hci-8.1 v5 04/11] dm-ploop: reject empty deltas
2026-10-13 7:55 [Devel] [PATCH hci-8.1 v5 00/11] dm-ploop: give back unused tail of top delta on suspend Alexander Ivanov
` (2 preceding siblings ...)
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 03/11] dm-ploop: reject a delta header whose BAT does not cover the target Alexander Ivanov
@ 2026-10-13 7:55 ` Alexander Ivanov
2026-10-13 8:02 ` no-reply
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 05/11] dm-ploop: set delta is_raw before applying its mappings Alexander Ivanov
` (6 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Alexander Ivanov @ 2026-10-13 7:55 UTC (permalink / raw)
To: den; +Cc: devel
ploop_delta_check_header() accepts a header with m_Size == 0, and
ploop_add_delta() accepts a raw delta of zero size. The BAT iterators
can't handle nr_be == 0: ploop_bat_clu_to_page_nr(nr_be - 1) wraps to
page 0, so md pages past the first one are iterated whole and the
clu == nr_be - 1 stop condition never matches. A non-raw empty delta
has a single md page, so ploop_apply_delta_mappings() and
notify_delta_merged() (and ploop_check_delta_before_flip() with
PLOOP_DEBUG) walk past it and dereference a NULL d_md on a device with
more than one md page. A raw delta is not handled as raw yet, as
ploop_add_delta() sets is_raw only after applying its mappings; once it
is, an empty raw delta would get all the clusters mapped to it.
Reject empty deltas in ploop_delta_check_header(), which covers every
caller reading a delta header, and in ploop_add_delta() for raw deltas.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-ploop-bat.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 7b12b0f9fe81..4fc99400a0a4 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -268,7 +268,7 @@ static int ploop_delta_check_header(struct ploop *ploop,
offset_clusters = SEC_TO_CLU(ploop, le32_to_cpu(d_hdr->m_FirstBlockOffset));
bat_clusters = ploop_nr_bat_clusters(ploop, delta_nr_be);
- if (delta_nr_be > ploop->nr_bat_entries ||
+ if (!delta_nr_be || delta_nr_be > ploop->nr_bat_entries ||
bat_clusters != offset_clusters)
goto out;
@@ -503,6 +503,9 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
goto out;
} else {
nr_be = POS_TO_CLU(ploop, file_size);
+ ret = -EPROTO;
+ if (!nr_be)
+ goto out;
}
ret = -EBADSLT; /* Lower delta can't be bigger then upper */
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread* [Devel] [PATCH hci-8.1 v5 05/11] dm-ploop: set delta is_raw before applying its mappings
2026-10-13 7:55 [Devel] [PATCH hci-8.1 v5 00/11] dm-ploop: give back unused tail of top delta on suspend Alexander Ivanov
` (3 preceding siblings ...)
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 04/11] dm-ploop: reject empty deltas Alexander Ivanov
@ 2026-10-13 7:55 ` Alexander Ivanov
2026-10-13 8:03 ` no-reply
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 06/11] dm-ploop: factor out ploop_read_delta() from ploop_add_delta() Alexander Ivanov
` (5 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Alexander Ivanov @ 2026-10-13 7:55 UTC (permalink / raw)
To: den; +Cc: devel
ploop_apply_delta_mappings() takes is_raw from deltas[level], but
ploop_add_delta() sets deltas[level].is_raw only after the mappings
are applied. So a raw delta is handled as a non-raw one: its md_root
is empty, ploop_md_first_entry() returns NULL and d_md->kmpage
dereferences it.
This has been the case since the driver was added in commit
6ddad5a4d226 ("dm-ploop: Add ploop target driver"), i.e. raw deltas
have never worked here; this is not a regression.
Set is_raw before applying the mappings.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
drivers/md/dm-ploop-bat.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 4fc99400a0a4..3427d1a90d81 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -512,6 +512,7 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
if (level != ploop_top_level(ploop) && nr_be > deltas[level + 1].nr_be)
goto out;
+ deltas[level].is_raw = is_raw;
ploop_apply_delta_mappings(ploop, deltas, level, &md_root, nr_be);
deltas[level].file = file;
@@ -532,7 +533,6 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
deltas[level].file_size = file_size;
deltas[level].file_preallocated_area_start = file_size;
deltas[level].nr_be = nr_be;
- deltas[level].is_raw = is_raw;
ret = 0;
out:
ploop_free_md_pages_tree(&md_root);
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread* [Devel] [PATCH hci-8.1 v5 06/11] dm-ploop: factor out ploop_read_delta() from ploop_add_delta()
2026-10-13 7:55 [Devel] [PATCH hci-8.1 v5 00/11] dm-ploop: give back unused tail of top delta on suspend Alexander Ivanov
` (4 preceding siblings ...)
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 05/11] dm-ploop: set delta is_raw before applying its mappings Alexander Ivanov
@ 2026-10-13 7:55 ` Alexander Ivanov
2026-10-13 8:04 ` no-reply
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 07/11] dm-ploop: take bat_lock around md page lookup in ploop_llseek_hole() Alexander Ivanov
` (4 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Alexander Ivanov @ 2026-10-13 7:55 UTC (permalink / raw)
To: den; +Cc: devel
Move reading of delta BAT and applying its mappings to
ploop->bat_entries into a separate function ploop_read_delta(). It
will be used to reload metadata of already added deltas.
ploop_read_delta() takes is_raw from deltas[level], so ploop_add_delta()
sets it before the call.
Replace the stale comment of ploop_add_delta(): it claimed deltas are
added from oldest to newest, while ploop_add_deltas_stack() adds them
from top to base, which ploop_read_delta() relies on.
No functional changes.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
drivers/md/dm-ploop-bat.c | 43 +++++++++++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 8 deletions(-)
diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 3427d1a90d81..5a89771a2e02 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -481,23 +481,27 @@ int ploop_check_delta_length(struct ploop *ploop, struct file *file, loff_t *fil
ALLOW_ERROR_INJECTION(ploop_check_delta_length, ERRNO);
/*
- * @fd refers to a new delta, which is placed right before top_delta.
- * So, userspace has to populate deltas stack from oldest to newest.
+ * Read BAT of delta @level and apply its mappings to ploop->bat_entries.
+ * deltas[@level].is_raw and nr_be of upper deltas must be already set.
+ *
+ * Deltas must be read from top to base: mappings of upper deltas take
+ * precedence (already populated BAT entries are not overwritten), and
+ * size of the delta is checked against nr_be of the upper one.
*/
-int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_raw)
+static int ploop_read_delta(struct ploop *ploop, u32 level, struct file *file,
+ loff_t *file_size_ret, u32 *nr_be_ret)
{
struct ploop_delta *deltas = ploop->deltas;
struct rb_root md_root = RB_ROOT;
- struct file *cloned_file = NULL;
loff_t file_size;
u32 nr_be;
- int ret, i;
+ int ret;
ret = ploop_check_delta_length(ploop, file, &file_size);
if (ret)
goto out;
- if (!is_raw) {
+ if (!deltas[level].is_raw) {
ret = ploop_read_delta_metadata(ploop, file, &md_root, &nr_be);
if (ret)
goto out;
@@ -512,9 +516,33 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
if (level != ploop_top_level(ploop) && nr_be > deltas[level + 1].nr_be)
goto out;
- deltas[level].is_raw = is_raw;
ploop_apply_delta_mappings(ploop, deltas, level, &md_root, nr_be);
+ *file_size_ret = file_size;
+ *nr_be_ret = nr_be;
+ ret = 0;
+out:
+ ploop_free_md_pages_tree(&md_root);
+ return ret;
+}
+
+/*
+ * Add @file as delta @level. Deltas must be added from top to base,
+ * see ploop_read_delta().
+ */
+int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_raw)
+{
+ struct ploop_delta *deltas = ploop->deltas;
+ struct file *cloned_file = NULL;
+ loff_t file_size;
+ u32 nr_be;
+ int ret, i;
+
+ deltas[level].is_raw = is_raw;
+ ret = ploop_read_delta(ploop, level, file, &file_size, &nr_be);
+ if (ret)
+ goto out;
+
deltas[level].file = file;
deltas[level].mtfile = kcalloc(ploop->nkt_runners, sizeof(*file),
GFP_KERNEL);
@@ -535,7 +563,6 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
deltas[level].nr_be = nr_be;
ret = 0;
out:
- ploop_free_md_pages_tree(&md_root);
return ret;
}
ALLOW_ERROR_INJECTION(ploop_add_delta, ERRNO);
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread* [Devel] [PATCH hci-8.1 v5 07/11] dm-ploop: take bat_lock around md page lookup in ploop_llseek_hole()
2026-10-13 7:55 [Devel] [PATCH hci-8.1 v5 00/11] dm-ploop: give back unused tail of top delta on suspend Alexander Ivanov
` (5 preceding siblings ...)
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 06/11] dm-ploop: factor out ploop_read_delta() from ploop_add_delta() Alexander Ivanov
@ 2026-10-13 7:55 ` Alexander Ivanov
2026-10-13 8:06 ` no-reply
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 08/11] dm-ploop: use wake_up() for inflight_md_wq and dispatcher_wq_prealloc Alexander Ivanov
` (3 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Alexander Ivanov @ 2026-10-13 7:55 UTC (permalink / raw)
To: den; +Cc: devel
ploop_llseek_hole() walks the md pages tree without any lock, while
resize inserts new md pages into it in ploop_add_md_pages(). A lookup
racing with rb-tree rebalancing may follow a stale pointer or miss the
page and return -EINVAL.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-ploop-map.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 92ceadf41b42..691284a239df 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -2772,7 +2772,11 @@ loff_t ploop_llseek_hole(struct dm_target *ti, loff_t offset, int whence)
while (clu < READ_ONCE(ploop->nr_bat_entries)) {
if (id != ploop_bat_clu_to_page_nr(clu)) {
id = ploop_bat_clu_to_page_nr(clu);
+
+ spin_lock_irq(&ploop->bat_lock);
md = ploop_md_page_find(ploop, id);
+ spin_unlock_irq(&ploop->bat_lock);
+
if (!md)
return -EINVAL;
}
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread* [Devel] [PATCH hci-8.1 v5 08/11] dm-ploop: use wake_up() for inflight_md_wq and dispatcher_wq_prealloc
2026-10-13 7:55 [Devel] [PATCH hci-8.1 v5 00/11] dm-ploop: give back unused tail of top delta on suspend Alexander Ivanov
` (6 preceding siblings ...)
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 07/11] dm-ploop: take bat_lock around md page lookup in ploop_llseek_hole() Alexander Ivanov
@ 2026-10-13 7:55 ` Alexander Ivanov
2026-10-13 8:04 ` no-reply
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 09/11] dm-ploop: fix use-after-free on the last inflight_md decrement Alexander Ivanov
` (2 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Alexander Ivanov @ 2026-10-13 7:55 UTC (permalink / raw)
To: den; +Cc: devel
wake_up_interruptible() wakes only TASK_INTERRUPTIBLE waiters. All the
current waiters on inflight_md_wq and dispatcher_wq_prealloc sleep
interruptibly, but one of the next patches adds uninterruptible ones on
postsuspend, which would never be woken. Use wake_up(), which wakes
both kinds.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-ploop-map.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 691284a239df..c313f303fff9 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -926,7 +926,7 @@ static void ploop_put_piwb(struct ploop_index_wb *piwb)
*/
if (atomic_dec_and_test(&piwb->ploop->inflight_md))
- wake_up_interruptible(&piwb->ploop->inflight_md_wq);
+ wake_up(&piwb->ploop->inflight_md_wq);
ploop_free_piwb(piwb);
}
}
@@ -1106,7 +1106,7 @@ void ploop_break_bat_update(struct ploop *ploop, struct md_page *md,
spin_unlock_irqrestore(&ploop->bat_lock, flags);
if (atomic_dec_and_test(&ploop->inflight_md))
- wake_up_interruptible(&ploop->inflight_md_wq);
+ wake_up(&ploop->inflight_md_wq);
ploop_free_piwb(piwb);
}
@@ -2372,7 +2372,7 @@ int ploop_allocator(void *data)
PL_FMT("failed to allocate space: %d\n"),
ploop_device_name(ploop), ret);
}
- wake_up_interruptible(&ploop->dispatcher_wq_prealloc);
+ wake_up(&ploop->dispatcher_wq_prealloc);
current_restore_flags(old_flags, PLOOP_PFLAGS);
}
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread* [Devel] [PATCH hci-8.1 v5 09/11] dm-ploop: fix use-after-free on the last inflight_md decrement
2026-10-13 7:55 [Devel] [PATCH hci-8.1 v5 00/11] dm-ploop: give back unused tail of top delta on suspend Alexander Ivanov
` (7 preceding siblings ...)
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 08/11] dm-ploop: use wake_up() for inflight_md_wq and dispatcher_wq_prealloc Alexander Ivanov
@ 2026-10-13 7:55 ` Alexander Ivanov
2026-10-13 8:11 ` no-reply
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 10/11] dm-ploop: reload metadata on resume Alexander Ivanov
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 11/11] dm-ploop: truncate unused tail of top delta on suspend Alexander Ivanov
10 siblings, 1 reply; 25+ messages in thread
From: Alexander Ivanov @ 2026-10-13 7:55 UTC (permalink / raw)
To: den; +Cc: devel
The last piwb reference is often dropped from the BAT write completion
in aio context. ploop_bat_write_finish() completes the data pios there,
and once the last original request is done, suspend can finish and a
table reload or remove can free the ploop in ploop_destroy(). The
completing CPU still decrements inflight_md, calls wake_up() on
inflight_md_wq and frees the piwb into ploop->pio_pool, so it can lock
a freed waitqueue or free into a destroyed mempool.
Nothing on the suspend or destroy path waits for inflight_md, and the
one waiter that exists, ploop_suspend_submitting_pios(), checks the
counter without a lock. It can see zero and return before wake_up() and
ploop_free_piwb() are done with the ploop, so the message can finish
and the table can be destroyed under the waker.
Free the piwb first, then decrement inflight_md and wake the waiters
under inflight_md_wq.lock as the last access to the ploop. Have the
waiters take the same lock once they see zero, so they cannot proceed
until the waker has released it, and wait for it in ploop_destroy()
before freeing anything.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-ploop-cmd.c | 6 ++++--
drivers/md/dm-ploop-map.c | 22 ++++++++++++++++++----
drivers/md/dm-ploop-target.c | 7 +++++++
3 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index ab17520c2d56..977b6920a5b4 100644
--- a/drivers/md/dm-ploop-cmd.c
+++ b/drivers/md/dm-ploop-cmd.c
@@ -154,8 +154,10 @@ static int ploop_suspend_submitting_pios(struct ploop *ploop)
if (ret)
goto out;
- ret = wait_event_interruptible(ploop->inflight_md_wq,
- !atomic_read(&ploop->inflight_md));
+ spin_lock_irq(&ploop->inflight_md_wq.lock);
+ ret = wait_event_interruptible_locked_irq(ploop->inflight_md_wq,
+ !atomic_read(&ploop->inflight_md));
+ spin_unlock_irq(&ploop->inflight_md_wq.lock);
out:
if (ret)
ploop_resume_submitting_pios(ploop);
diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index c313f303fff9..e2b70cb3c142 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -909,9 +909,25 @@ static void ploop_free_piwb(struct ploop_index_wb *piwb)
static void ploop_bat_write_finish(struct pio *pio, void *piwb_ptr,
blk_status_t bi_status);
+/*
+ * Waiters do not act on zero inflight_md without holding inflight_md_wq.lock,
+ * so the ploop stays valid while we hold it. Once we drop it, the ploop may
+ * be freed, so this must be the last access to it.
+ */
+static void ploop_dec_inflight_md(struct ploop *ploop)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&ploop->inflight_md_wq.lock, flags);
+ if (atomic_dec_and_test(&ploop->inflight_md))
+ wake_up_locked(&ploop->inflight_md_wq);
+ spin_unlock_irqrestore(&ploop->inflight_md_wq.lock, flags);
+}
+
static void ploop_put_piwb(struct ploop_index_wb *piwb)
{
if (atomic_dec_and_test(&piwb->count)) {
+ struct ploop *ploop = piwb->ploop;
ploop_bat_write_finish(piwb->pio, piwb, piwb->bi_status);
@@ -925,9 +941,8 @@ static void ploop_put_piwb(struct ploop_index_wb *piwb)
* zero keeps it as is
*/
- if (atomic_dec_and_test(&piwb->ploop->inflight_md))
- wake_up(&piwb->ploop->inflight_md_wq);
ploop_free_piwb(piwb);
+ ploop_dec_inflight_md(ploop);
}
}
@@ -1105,9 +1120,8 @@ void ploop_break_bat_update(struct ploop *ploop, struct md_page *md,
spin_unlock(&md->md_lock);
spin_unlock_irqrestore(&ploop->bat_lock, flags);
- if (atomic_dec_and_test(&ploop->inflight_md))
- wake_up(&ploop->inflight_md_wq);
ploop_free_piwb(piwb);
+ ploop_dec_inflight_md(ploop);
}
static void ploop_bat_page_zero_cluster(struct ploop *ploop,
diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
index 3b589579741c..dc9410212831 100644
--- a/drivers/md/dm-ploop-target.c
+++ b/drivers/md/dm-ploop-target.c
@@ -192,6 +192,13 @@ static void ploop_destroy(struct ploop *ploop)
if (ploop->kt_allocator)
kthread_stop(ploop->kt_allocator->task);
+ /* BAT writes may still be completing from aio context */
+ wait_event(ploop->inflight_md_wq,
+ !atomic_read(&ploop->inflight_md));
+ /* Wait for ploop_dec_inflight_md() to drop the lock */
+ spin_lock_irq(&ploop->inflight_md_wq.lock);
+ spin_unlock_irq(&ploop->inflight_md_wq.lock);
+
WARN_ON(!llist_empty(&ploop->pios[PLOOP_LIST_PREPARE]));
WARN_ON(!llist_empty(&ploop->llresubmit_pios));
WARN_ON(!llist_empty(&ploop->enospc_pios));
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Devel] [PATCH hci-8.1 v5 09/11] dm-ploop: fix use-after-free on the last inflight_md decrement
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 09/11] dm-ploop: fix use-after-free on the last inflight_md decrement Alexander Ivanov
@ 2026-10-13 8:11 ` no-reply
0 siblings, 0 replies; 25+ messages in thread
From: no-reply @ 2026-10-13 8:11 UTC (permalink / raw)
To: Alexander Ivanov, den; +Cc: devel
> The last piwb reference is often dropped from the BAT write completion
> in aio context. ploop_bat_write_finish() completes the data pios there,
> and once the last original request is done, suspend can finish and a
> table reload or remove can free the ploop in ploop_destroy(). The
> completing CPU still decrements inflight_md, calls wake_up() on
> inflight_md_wq and frees the piwb into ploop->pio_pool, so it can lock
> a freed waitqueue or free into a destroyed mempool.
>
> Nothing on the suspend or destroy path waits for inflight_md, and the
> one waiter that exists, ploop_suspend_submitting_pios(), checks the
> counter without a lock. It can see zero and return before wake_up() and
> ploop_free_piwb() are done with the ploop, so the message can finish
> and the table can be destroyed under the waker.
>
> Free the piwb first, then decrement inflight_md and wake the waiters
> under inflight_md_wq.lock as the last access to the ploop. Have the
> waiters take the same lock once they see zero, so they cannot proceed
> until the waker has released it, and wait for it in ploop_destroy()
> before freeing anything.
>
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-ploop: ploop target driver
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013075513.90618-1-alexander.ivanov@virtuozzo.com?part=9
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Devel] [PATCH hci-8.1 v5 10/11] dm-ploop: reload metadata on resume
2026-10-13 7:55 [Devel] [PATCH hci-8.1 v5 00/11] dm-ploop: give back unused tail of top delta on suspend Alexander Ivanov
` (8 preceding siblings ...)
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 09/11] dm-ploop: fix use-after-free on the last inflight_md decrement Alexander Ivanov
@ 2026-10-13 7:55 ` Alexander Ivanov
2026-10-13 8:16 ` no-reply
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 11/11] dm-ploop: truncate unused tail of top delta on suspend Alexander Ivanov
10 siblings, 1 reply; 25+ messages in thread
From: Alexander Ivanov @ 2026-10-13 7:55 UTC (permalink / raw)
To: den; +Cc: devel
On "dmsetup reload" the new ploop reads image metadata in ploop_ctr(),
but IO keeps going through the old ploop until suspend, and userspace
may modify images of a suspended device (merge zeroes BAT entries of
the base delta, snapshot creation trims the top delta). So after resume
the new ploop may work with a stale BAT and file sizes, e.g. allocate
clusters already in use.
Reload all metadata in ploop_preresume(): the top delta header, BAT of
every delta and delta file sizes, and rebuild the BAT tree and
holes_bitmap. On error the old metadata is kept and resume fails. DM
has already swapped the new table in by then, so the device stays
suspended and the next resume retries. Userspace must check the result
and must not resume the previous instance on the same images.
Skip the in-use signature check on reload, it is done in ploop_ctr().
Take the signature from disk: libploop marks the image closed on the
suspended device before umount, and an in-memory IN_USE written over
it makes umount trim the image and lose CBT. After flip_upper_deltas
keep the in-memory signature, as userspace relies on it being written
to the new top delta, otherwise the next load fails with -ENOLCK after
online merge.
If metadata is dirty after suspend, keep it, as md pages may still be
referenced. Data bios complete before BAT writeback, so wait for
inflight_md in postsuspend.
Request preallocation in ploop_preresume() after the reload instead of
in ploop_ctr(), so the new ploop doesn't truncate the top delta back to
a stale size before resume. Wait for pending preallocation in
postsuspend, killably, as the allocator may hang on a dead file. Don't
wait in standby, so that dm_replace() of libploop can suspend the
device to recover it. Skip the reload and prealloc on resume while
preallocation is pending, as the allocator changes the top delta size
under bat_lock only. Skip the reload in standby too, as the images may
be inaccessible.
Track standby per ploop instead of the queue bit. The queue is shared
by all tables, and ploop_ctr() of a new table clears the bit before the
old table is suspended, so the old ploop would wait for its stuck
allocator in postsuspend.
ploop_llseek_hole() needs no locking against the reload:
dm_llseek_hole() calls it under md->io_barrier and returns -EAGAIN on
a suspended device.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-ploop-bat.c | 146 ++++++++++++++++++++++++++++++++++-
drivers/md/dm-ploop-cmd.c | 1 +
drivers/md/dm-ploop-map.c | 9 ++-
drivers/md/dm-ploop-target.c | 87 +++++++++++++++------
drivers/md/dm-ploop.h | 13 +++-
5 files changed, 227 insertions(+), 29 deletions(-)
diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 5a89771a2e02..c060afdd8d3e 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -185,7 +185,8 @@ static int ploop_setup_holes_bitmap(struct ploop *ploop, u32 bat_clusters)
}
ALLOW_ERROR_INJECTION(ploop_setup_holes_bitmap, ERRNO);
-int ploop_setup_metadata(struct ploop *ploop, struct page *page)
+int ploop_setup_metadata(struct ploop *ploop, struct page *page,
+ bool check_in_use)
{
struct ploop_pvd_header *m_hdr = NULL;
u32 bat_clusters, offset_clusters;
@@ -200,7 +201,8 @@ int ploop_setup_metadata(struct ploop *ploop, struct page *page)
goto out;
ret = -ENOLCK;
- if (m_hdr->m_DiskInUse != cpu_to_le32(SIGNATURE_DISK_IN_USE) &&
+ if (check_in_use &&
+ m_hdr->m_DiskInUse != cpu_to_le32(SIGNATURE_DISK_IN_USE) &&
!ploop_is_ro(ploop) && !ignore_signature_disk_in_use)
goto out;
@@ -567,6 +569,146 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
}
ALLOW_ERROR_INJECTION(ploop_add_delta, ERRNO);
+static bool ploop_md_pages_are_clean(struct ploop *ploop)
+{
+ struct rb_node *node;
+ struct md_page *md;
+
+ if (!llist_empty(&ploop->wb_batch_llist) ||
+ atomic_read(&ploop->inflight_md))
+ return false;
+
+ ploop_for_each_md_page(ploop, md, node) {
+ if (test_bit(MD_DIRTY, &md->status) ||
+ test_bit(MD_WRITEBACK, &md->status) ||
+ test_bit(MD_UPDATING, &md->status) ||
+ md->piwb || !llist_empty(&md->wait_llist))
+ return false;
+ }
+
+ return true;
+}
+
+/*
+ * Build in-memory metadata from scratch: re-read header of top delta,
+ * BAT of every delta and sizes of delta files. This is called on resume
+ * of suspended device, since after "dmsetup reload" the metadata of the
+ * new ploop may be outdated: the images could be changed by the old
+ * ploop after the new one was constructed. On error the old metadata
+ * is kept.
+ */
+int ploop_reload_metadata(struct ploop *ploop)
+{
+ struct rb_root old_bat_entries = ploop->bat_entries;
+ void *old_holes_bitmap = ploop->holes_bitmap;
+ u32 old_nr_bat_entries = ploop->nr_bat_entries;
+ u32 old_hb_nr = ploop->hb_nr;
+ u32 old_m_Type = ploop->m_Type;
+ u32 old_m_Sectors = ploop->m_Sectors;
+ struct ploop_pvd_header *hdr, *old_hdr;
+ u8 old_m_Sig[sizeof(ploop->m_Sig)];
+ struct ploop_delta *delta, *old_deltas;
+ struct page *page;
+ loff_t file_size;
+ u32 nr_be;
+ int level, ret;
+
+ /*
+ * Metadata of the device that has already been running is up to
+ * date, so we only need to reload clean metadata. Dirty metadata
+ * must not appear after suspend, as all IO and BAT writeback are
+ * completed (see ploop_postsuspend()), but we can't free md pages,
+ * which are still in use, in any case.
+ */
+ if (WARN_ON_ONCE(!ploop_md_pages_are_clean(ploop)))
+ return 0;
+
+ memcpy(old_m_Sig, ploop->m_Sig, sizeof(old_m_Sig));
+
+ old_deltas = kmemdup(ploop->deltas,
+ sizeof(*old_deltas) * ploop->nr_deltas,
+ GFP_KERNEL);
+ page = alloc_page(GFP_KERNEL);
+ ret = -ENOMEM;
+ if (!old_deltas || !page)
+ goto out;
+
+ ret = ploop_rw_page_sync(READ, ploop_top_delta(ploop)->file, 0, page);
+ if (ret)
+ goto out;
+
+ ploop->bat_entries = RB_ROOT;
+ ploop->holes_bitmap = NULL;
+
+ /* Image is already checked for being in use in ploop_ctr() */
+ ret = ploop_setup_metadata(ploop, page, false);
+ if (ret)
+ goto restore;
+
+ ret = ploop_prealloc_md_pages(&ploop->bat_entries, 0,
+ ploop->nr_bat_entries);
+ if (ret)
+ goto restore;
+
+ /*
+ * Same order as in ploop_add_deltas_stack(): from top to base.
+ * ploop_read_delta() relies on it, see the comment there. nr_be of
+ * the delta is updated right away, as the lower delta is checked
+ * against it on the next iteration.
+ */
+ for (level = ploop_top_level(ploop); level >= 0; level--) {
+ delta = &ploop->deltas[level];
+
+ ret = ploop_read_delta(ploop, level, delta->file,
+ &file_size, &nr_be);
+ if (ret)
+ goto restore;
+
+ delta->file_size = file_size;
+ delta->file_preallocated_area_start = file_size;
+ delta->nr_be = nr_be;
+ }
+
+ /*
+ * Header of top delta is written with BAT page 0. Userspace doesn't
+ * mark the delta, which becomes top after flip_upper_deltas, as in
+ * use, and relies that the in-use signature of the old top delta
+ * will be written to it. So keep the signature we had. In other
+ * cases take it from disk: userspace may have changed it on the
+ * suspended device (e.g. marked the image closed on umount).
+ */
+ if (ploop->top_flipped) {
+ old_hdr = ploop_md_first_entry(&old_bat_entries)->kmpage;
+ hdr = ploop_md_first_entry(&ploop->bat_entries)->kmpage;
+ hdr->m_DiskInUse = old_hdr->m_DiskInUse;
+ ploop->top_flipped = false;
+ }
+
+ ploop_free_md_pages_tree(&old_bat_entries);
+ kvfree(old_holes_bitmap);
+ goto out;
+restore:
+ ploop_free_md_pages_tree(&ploop->bat_entries);
+ kvfree(ploop->holes_bitmap);
+ ploop->bat_entries = old_bat_entries;
+ ploop->holes_bitmap = old_holes_bitmap;
+ ploop->nr_bat_entries = old_nr_bat_entries;
+ ploop->hb_nr = old_hb_nr;
+ memcpy(ploop->m_Sig, old_m_Sig, sizeof(ploop->m_Sig));
+ ploop->m_Type = old_m_Type;
+ ploop->m_Sectors = old_m_Sectors;
+ memcpy(ploop->deltas, old_deltas,
+ sizeof(*old_deltas) * ploop->nr_deltas);
+out:
+ if (page)
+ put_page(page);
+ kfree(old_deltas);
+ if (ret)
+ PL_ERR("Failed to reload metadata: %d", ret);
+ return ret;
+}
+ALLOW_ERROR_INJECTION(ploop_reload_metadata, ERRNO);
+
void ploop_put_delta(struct ploop *ploop, struct ploop_delta *delta)
{
int i;
diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index 977b6920a5b4..1590af23360f 100644
--- a/drivers/md/dm-ploop-cmd.c
+++ b/drivers/md/dm-ploop-cmd.c
@@ -1049,6 +1049,7 @@ static int process_flip_upper_deltas(struct ploop *ploop)
/* FIXME */
swap(ploop->deltas[level], ploop->deltas[level+1]);
+ ploop->top_flipped = true;
return 0;
}
diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index e2b70cb3c142..5733b7d28196 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -1247,7 +1247,7 @@ void ploop_should_prealloc(struct ploop *ploop)
spin_unlock_irqrestore(&ploop->bat_lock, flags);
}
-static int ploop_no_pending_prealloc(struct ploop *ploop)
+int ploop_no_pending_prealloc(struct ploop *ploop)
{
int ret;
unsigned long flags;
@@ -1259,6 +1259,12 @@ static int ploop_no_pending_prealloc(struct ploop *ploop)
return ret;
}
+void ploop_wait_prealloc(struct ploop *ploop)
+{
+ wait_event_killable(ploop->dispatcher_wq_prealloc,
+ ploop_no_pending_prealloc(ploop));
+}
+
static int ploop_allocate_cluster(struct ploop *ploop, u32 *dst_clu, struct file *file)
{
struct ploop_delta *top = ploop_top_delta(ploop);
@@ -1446,6 +1452,7 @@ static void ploop_check_standby_mode(struct ploop *ploop, long res)
spin_lock_irqsave(&q->queue_lock, flags);
prev = test_and_set_bit(QUEUE_FLAG_STANDBY, &q->queue_flags);
spin_unlock_irqrestore(&q->queue_lock, flags);
+ WRITE_ONCE(ploop->standby, true);
if (!prev)
PL_INFO("was switched into the standby mode");
diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
index dc9410212831..3a24769d0a8f 100644
--- a/drivers/md/dm-ploop-target.c
+++ b/drivers/md/dm-ploop-target.c
@@ -21,6 +21,7 @@
#include <linux/uio.h>
#include <linux/timer.h>
#include <linux/error-injection.h>
+#include <linux/sched/mm.h>
#include "dm-ploop.h"
#define DM_MSG_PREFIX "ploop"
@@ -295,7 +296,7 @@ static int ploop_check_top_delta(struct ploop *ploop, struct file *file)
/* flush dirty pages before changing direct IO */
vfs_fsync(file, 1);
- ret = ploop_setup_metadata(ploop, page);
+ ret = ploop_setup_metadata(ploop, page, true);
if (ret)
goto out;
@@ -426,8 +427,7 @@ static struct ploop_worker *ploop_worker_create(struct ploop *ploop,
* suspended and resumed. This behavior is intentional to allow for atomic
* updates to the mapping table without disrupting ongoing I/O operations.
*
- * But dm-ploop is not fully compatible with this design. Ploop does not do a
- * "metadata" reload on resume. So there can be situations like:
+ * So there can be situations like:
*
* 1. dmsetup reload is called, and new ploop is created with ploop_ctr for
* the same delta image file as an old ploop and ploop_add_deltas_stack reads
@@ -438,23 +438,19 @@ static struct ploop_worker *ploop_worker_create(struct ploop *ploop,
* the new one, new ploop has outdated metadata and thus may corrupt the
* images by relying on it.
*
- * For instance we thought to truncate the preallocations to the delta image
- * file on ploop_dtr, but that can't be done as ploop_dtr is called on the old
- * ploop after the new one is resumed, so we can't possibly modify the image
- * from the old ploop context at this point, as new ploop may be already doing
- * its own IO or preallocation.
+ * To handle this, ploop_preresume reloads all the metadata from the images
+ * (see ploop_reload_metadata), so that the new ploop "sees" all previous
+ * updates from the old one. For the same reason the new ploop must not
+ * modify the images before resume, so preallocation is started only after
+ * the metadata reload, and the old ploop waits for its preallocation to
+ * complete on postsuspend.
*
- * The better way to do this truncation is on ploop_*suspend, and then on resume
- * to reload the metadata in the new ploop so that the new ploop "sees" all
- * previous updates from the old one.
- *
- * For now we can leave with it, as we chose not to do this truncation on
- * destroy and we rely that userspace always does dmsetup reload on already
- * suspended device.
- *
- * FIXME: We must implement metadata reload on resume. After that we can
- * implement preallocation truncation on suspend (see dm-qcow2 as it already
- * implements that in qcow2_parse_metadata and qcow2_truncate_preallocations).
+ * Still, we can't truncate the preallocations to the delta image file on
+ * ploop_dtr, as ploop_dtr is called on the old ploop after the new one is
+ * resumed, so we can't possibly modify the image from the old ploop context
+ * at this point, as new ploop may be already doing its own IO or
+ * preallocation. This truncation should be done on ploop_*suspend (see
+ * dm-qcow2 as it already implements that in qcow2_truncate_preallocations).
*/
static int ploop_ctr(struct dm_target *ti, unsigned int argc, char **argv)
{
@@ -644,9 +640,6 @@ static int ploop_ctr(struct dm_target *ti, unsigned int argc, char **argv)
ti->num_discard_bios = 1;
ti->discards_supported = true;
- if (!ploop_is_ro(ploop) && ploop->nr_deltas > 0)
- ploop_should_prealloc(ploop);
-
return 0;
err:
@@ -726,6 +719,11 @@ static void ploop_set_suspended(struct dm_target *ti, bool suspended)
up_write(&ploop->ctl_rwsem);
}
+static bool ploop_is_standby(struct ploop *ploop)
+{
+ return READ_ONCE(ploop->standby);
+}
+
static void ploop_presuspend(struct dm_target *ti)
{
struct ploop *ploop = ti->private;
@@ -750,19 +748,58 @@ static void ploop_presuspend_undo(struct dm_target *ti)
}
static void ploop_postsuspend(struct dm_target *ti)
{
+ struct ploop *ploop = ti->private;
+
+ /*
+ * Let the next resumed ploop (maybe of reloaded table)
+ * see final size of the top delta. Don't wait in standby
+ * mode: the allocator may be stuck on the dead file, while
+ * dm_replace() of libploop must be able to suspend the device
+ * to recover it. The wait is killable for the same reason.
+ * If the wait is skipped or killed, a ploop of a reloaded
+ * table may see a stale size, and a late truncate of our
+ * allocator may shrink the file under it.
+ */
+ if (!ploop_is_standby(ploop))
+ ploop_wait_prealloc(ploop);
+ /*
+ * Data bios are completed before BAT writeback is finished
+ * (see ploop_put_piwb()), so wait for it explicitly to make
+ * md pages clean for ploop_reload_metadata().
+ */
+ wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
ploop_set_suspended(ti, true);
}
static int ploop_preresume(struct dm_target *ti)
{
struct ploop *ploop = ti->private;
+ unsigned int noio_flags;
+ bool reload = false;
int ret = 0;
- down_read(&ploop->ctl_rwsem);
- if (ploop->noresume)
+ down_write(&ploop->ctl_rwsem);
+ if (ploop->noresume) {
ret = -EAGAIN;
- up_read(&ploop->ctl_rwsem);
+ } else if (!ploop_is_standby(ploop) &&
+ ploop_no_pending_prealloc(ploop)) {
+ reload = true;
+ /*
+ * The device is suspended, so reclaim must not wait
+ * for IO to it. Otherwise we deadlock with ourselves.
+ */
+ noio_flags = memalloc_noio_save();
+ ret = ploop_reload_metadata(ploop);
+ memalloc_noio_restore(noio_flags);
+ }
+ up_write(&ploop->ctl_rwsem);
if (ret == 0) {
+ /*
+ * Preallocate only after the metadata is reloaded,
+ * as prealloc relies on the actual top delta size.
+ */
+ if (reload && !ploop_is_ro(ploop))
+ ploop_should_prealloc(ploop);
/*
* We are singleton target. There will be
* no more reasons to break resume.
diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
index 5041fc8f1e13..5750eafc8a1b 100644
--- a/drivers/md/dm-ploop.h
+++ b/drivers/md/dm-ploop.h
@@ -245,6 +245,13 @@ struct ploop {
/* Resume is prohibited */
bool noresume;
+ /* Top delta was swapped by flip_upper_deltas */
+ bool top_flipped;
+ /*
+ * This ploop switched the queue into standby. Unlike the queue
+ * bit, it is not cleared by ploop_ctr() of a new table.
+ */
+ bool standby;
/* Device is suspended */
bool suspended;
/* Device wants suspend */
@@ -633,7 +640,9 @@ extern struct pio *ploop_alloc_pio_with_pages(struct ploop *ploop);
extern void ploop_free_pio_with_pages(struct ploop *ploop, struct pio *pio);
extern void ploop_pio_prepare_offsets(struct ploop *ploop, struct pio *pio, u32 clu);
-extern int ploop_setup_metadata(struct ploop *ploop, struct page *page);
+extern int ploop_setup_metadata(struct ploop *ploop, struct page *page,
+ bool check_in_use);
+extern int ploop_reload_metadata(struct ploop *ploop);
extern int ploop_read_delta_metadata(struct ploop *ploop, struct file *file,
struct rb_root *md_root, u32 *delta_nr_be);
extern void ploop_index_wb_init(struct ploop_index_wb *piwb,
@@ -651,6 +660,8 @@ extern void ploop_disable_writeback_delay(struct ploop *ploop);
extern void ploop_enable_writeback_delay(struct ploop *ploop);
extern void ploop_should_prealloc(struct ploop *ploop);
+extern void ploop_wait_prealloc(struct ploop *ploop);
+extern int ploop_no_pending_prealloc(struct ploop *ploop);
extern void ploop_resubmit_enospc_pios(struct ploop *ploop);
#endif /* __DM_PLOOP_H */
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Devel] [PATCH hci-8.1 v5 10/11] dm-ploop: reload metadata on resume
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 10/11] dm-ploop: reload metadata on resume Alexander Ivanov
@ 2026-10-13 8:16 ` no-reply
0 siblings, 0 replies; 25+ messages in thread
From: no-reply @ 2026-10-13 8:16 UTC (permalink / raw)
To: Alexander Ivanov, den; +Cc: devel
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] dm-ploop: ploop_postsuspend() waits uninterruptibly for BAT writeback, even in standby
- [Medium] dm-ploop: new ploop keeps suspended=false after failed preresume on a suspended DM device
- [Medium] dm-ploop: postsuspend ignores killed prealloc wait, old allocator can resize top delta after reload
- [Low] dm-ploop: per-instance standby flag is never cleared, so reload is skipped
--
commit fe4b80dd50a64ca3e5385df8c28c40c9de854171
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
dm-ploop: reload metadata on resume
This patch reloads the top delta header, the BAT of every delta and the
delta file sizes in ploop_preresume(), and rebuilds the BAT tree and
holes_bitmap. It also moves preallocation to after the reload, tracks
standby per ploop instance, and makes ploop_postsuspend() wait for
pending preallocation and for inflight BAT writeback.
Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171
> diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
> --- a/drivers/md/dm-ploop-map.c
> +++ b/drivers/md/dm-ploop-map.c
[ ... ]
> +void ploop_wait_prealloc(struct ploop *ploop)
> +{
> + wait_event_killable(ploop->dispatcher_wq_prealloc,
> + ploop_no_pending_prealloc(ploop));
> +}
[Severity: Medium]
ploop_wait_prealloc() discards the return value of wait_event_killable().
If the suspending task is killed while the old allocator is still inside
ploop_preallocate_cluster(), ploop_postsuspend() carries on and marks
the device suspended:
ploop_postsuspend()
ploop_wait_prealloc() /* -ERESTARTSYS dropped */
wait_event(inflight_md_wq, ...)
ploop_set_suspended(ti, true);
A reloaded table can then resume. Its ploop_preresume() only checks
ploop_no_pending_prealloc() on the new ploop, so it cannot see the old
instance's in-flight allocation. When the old allocator finishes, it
runs:
ploop_preallocate_cluster()
new_len = top->file_size + ploop->prealloc_in_progress;
...
ploop_truncate_prealloc_safe(ploop, top->file_size, new_len, ...)
new_len was computed from the old, stale file_size. Can this set the
file size below what the new ploop has already reloaded and extended,
cutting off clusters the new ploop allocated?
The comment in ploop_postsuspend() acknowledges this ("a late truncate of
our allocator may shrink the file under it"), but nothing seems to handle
it. Should the wait result be propagated, or the old allocator otherwise
fenced?
> diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
> --- a/drivers/md/dm-ploop-target.c
> +++ b/drivers/md/dm-ploop-target.c
[ ... ]
> static void ploop_postsuspend(struct dm_target *ti)
> {
> + struct ploop *ploop = ti->private;
> +
> + /*
> + * Let the next resumed ploop (maybe of reloaded table)
> [ ... ]
> + */
> + if (!ploop_is_standby(ploop))
> + ploop_wait_prealloc(ploop);
> + /*
> + * Data bios are completed before BAT writeback is finished
> + * (see ploop_put_piwb()), so wait for it explicitly to make
> + * md pages clean for ploop_reload_metadata().
> + */
> + wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
> ploop_set_suspended(ti, true);
> }
[Severity: Medium]
The comment above says the prealloc wait is skipped in standby and made
killable so that dm_replace() of libploop can suspend a device whose
backing file is dead. Is the new wait_event() on inflight_md_wq missing
the same escape?
inflight_md is raised in ploop_prepare_bat_update():
piwb->type = type;
atomic_inc(&ploop->inflight_md);
and is only dropped through ploop_dec_inflight_md(), from
ploop_put_piwb() or ploop_break_bat_update(). The BAT write is queued
from ploop_index_wb_submit() via ploop_runners_add_work() and completes
only when the backing file completes the I/O.
ploop_check_standby_mode() only reacts to -EBUSY, -ENOTCONN or -EIO, so
entering standby does not guarantee that a pending BAT write completes.
If the backing file stalls without returning an error, can
ploop_postsuspend() sleep forever in D state here?
ploop_presuspend() waits only on kt_worker->inflight_pios, so it does not
drain inflight_md. A BAT write with no attached data pio would not be
covered by that wait.
Should this wait be skipped in standby and made killable, like the
prealloc wait just above it?
Also, the later patch "dm-ploop: truncate unused tail of top delta on
suspend" keeps this wait_event() and moves it inside
down_write(&ploop->ctl_rwsem). In that state a stuck wait also blocks
ploop_message() and ploop_preresume() behind ctl_rwsem.
[ ... ]
> static int ploop_preresume(struct dm_target *ti)
> {
[ ... ]
> + down_write(&ploop->ctl_rwsem);
> + if (ploop->noresume) {
> ret = -EAGAIN;
> + } else if (!ploop_is_standby(ploop) &&
> + ploop_no_pending_prealloc(ploop)) {
> + reload = true;
[ ... ]
> + ret = ploop_reload_metadata(ploop);
> + memalloc_noio_restore(noio_flags);
> + }
> + up_write(&ploop->ctl_rwsem);
>
> if (ret == 0) {
[ ... ]
> ploop_set_suspended(ti, false);
> ploop_set_wants_suspend(ti, false);
> }
> return ret;
> }
[Severity: Medium]
On a reloaded table, the new ploop never runs ploop_postsuspend(), and
ploop_ctr() never initializes ->suspended, so it stays false from
kzalloc. If ploop_reload_metadata() fails (-ENOMEM, -EIO, -EBADSLT) or
noresume is set, ploop_preresume() returns an error and skips
ploop_set_suspended(ti, false), while DM keeps the device suspended.
do_resume() then still runs dm_table_destroy(old_map) even though
dm_resume() failed. That destroys the old ploop together with its
top_flipped state and in-memory IN_USE signature.
The live ploop now reports suspended == false while the DM device is
suspended. The flip_upper_deltas message handler in dm-ploop-cmd.c
checks:
if (!ploop->suspended || !ploop->noresume || ploop->maintaince)
return -EBUSY;
so it would return -EBUSY until a resume succeeds and the device is
suspended again. Is that intended? I have not checked how libploop
uses flip_upper_deltas after a failed resume, so this may be harmless in
practice. Should the new ploop's suspended flag be synchronized with DM
state on this path?
> diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
> --- a/drivers/md/dm-ploop-map.c
> +++ b/drivers/md/dm-ploop-map.c
> @@ -1446,6 +1452,7 @@ static void ploop_check_standby_mode(struct ploop *ploop, long res)
> spin_lock_irqsave(&q->queue_lock, flags);
> prev = test_and_set_bit(QUEUE_FLAG_STANDBY, &q->queue_flags);
> spin_unlock_irqrestore(&q->queue_lock, flags);
> + WRITE_ONCE(ploop->standby, true);
[Severity: Low]
This is the only place ploop->standby is written, and nothing in
drivers/md/dm-ploop-*.c clears it. The queue bit, by contrast, is
cleared in ploop_ctr():
if (blk_queue_standby(ploop_blk_queue(ploop))) {
blk_queue_flag_clear(QUEUE_FLAG_STANDBY, ...);
so the two can diverge. Once set, ploop_preresume() permanently skips
ploop_reload_metadata() and the following ploop_should_prealloc() for
this instance:
} else if (!ploop_is_standby(ploop) &&
ploop_no_pending_prealloc(ploop)) {
and ploop_postsuspend() permanently skips ploop_wait_prealloc().
Consider this sequence: ploop_check_standby_mode() sets ->standby, a new
table is loaded and ploop_ctr() clears the queue bit, the new table is
abandoned, and userspace resumes the old instance. The queue is no
longer in standby, but the old ploop skips the reload and runs on
possibly stale BAT and file sizes.
The commit message says userspace must not resume the previous instance
on the same images, but the driver does not enforce that. Should the
flag be cleared when the instance leaves standby, for example on a
successful resume?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013075513.90618-1-alexander.ivanov@virtuozzo.com?part=10
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Devel] [PATCH hci-8.1 v5 11/11] dm-ploop: truncate unused tail of top delta on suspend
2026-10-13 7:55 [Devel] [PATCH hci-8.1 v5 00/11] dm-ploop: give back unused tail of top delta on suspend Alexander Ivanov
` (9 preceding siblings ...)
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 10/11] dm-ploop: reload metadata on resume Alexander Ivanov
@ 2026-10-13 7:55 ` Alexander Ivanov
2026-10-13 8:20 ` no-reply
10 siblings, 1 reply; 25+ messages in thread
From: Alexander Ivanov @ 2026-10-13 7:55 UTC (permalink / raw)
To: den; +Cc: devel
Preallocated space and clusters at the end of the top delta freed by
discard are never given back, so the file size never shrinks. Now that
ploop reloads metadata on resume, we can truncate the top delta on
suspend: the next resumed ploop (maybe of reloaded table) picks up the
new file size.
Add ploop_calc_used_end(), which returns the end of the last cluster
used by the delta: either BAT area, the last mapped cluster or format
extension. Metadata is read from disk rather than taken from memory:
the next table load checks BAT entries against the file size (-EPROTO
otherwise), while in-memory BAT may differ from on-disk one after
failed BAT writeback. Skip it if md pages are not clean, as on-disk BAT
may miss allocated clusters then.
Userspace writes format extension (CBT) behind the last data cluster
of suspended device on umount, resumes it, and then removes the device,
which suspends it again. So keep the area up to the end of format
extension header, which is the last cluster of the extension, as
userspace does. Name the corresponding header field properly instead
of m_Reserved. The field was never interpreted before, so if it is not
cluster-aligned or out of range, don't truncate anything: unaligned
truncation would make the next resume fail with -EPROTO.
Truncate the top delta to the used end in ploop_postsuspend() after
pending preallocation is completed. Do it under ctl_rwsem, so that
ploop messages (e.g. resize) don't change the image in parallel. Skip
read-only devices and raw top delta. On error just leave the file as is.
Skip devices in standby mode as well: the top delta file is dead then,
and the truncation may hang with md->suspend_lock held, so dm_replace()
of libploop could neither replace nor remove the device.
Also skip it if the file size differs from top->file_size. Then another
ploop on the same images writes to the top delta (e.g. a live one left
by libploop error path), and on-disk BAT may miss clusters it has just
allocated, so the truncation would cut its data.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-ploop-bat.c | 56 +++++++++++++++++++++++++++++++
drivers/md/dm-ploop-target.c | 64 +++++++++++++++++++++++++++++++++---
drivers/md/dm-ploop.h | 3 +-
3 files changed, 118 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index c060afdd8d3e..7bab746a3707 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -709,6 +709,62 @@ int ploop_reload_metadata(struct ploop *ploop)
}
ALLOW_ERROR_INJECTION(ploop_reload_metadata, ERRNO);
+/*
+ * Returns the end of the last cluster of delta @file used by BAT area,
+ * mapped clusters or format extension, according to on-disk metadata.
+ * Must be called on suspended device.
+ */
+loff_t ploop_calc_used_end(struct ploop *ploop, struct file *file)
+{
+ u32 i, end, nr_be, dst_clu, used_clus, *bat_entries;
+ struct rb_root md_root = RB_ROOT;
+ struct ploop_pvd_header *hdr;
+ struct rb_node *node;
+ struct md_page *md;
+ u64 ext_off, ext_clu;
+ loff_t used_end;
+ int ret;
+
+ /* BAT on disk may miss allocated clusters until writeback is done */
+ if (WARN_ON_ONCE(!ploop_md_pages_are_clean(ploop)))
+ return -EBUSY;
+
+ ret = ploop_read_delta_metadata(ploop, file, &md_root, &nr_be);
+ if (ret)
+ return ret;
+
+ used_clus = ploop_nr_bat_clusters(ploop, nr_be);
+ rb_root_for_each_md_page(&md_root, md, node) {
+ bat_entries = md->kmpage;
+ init_be_iter(nr_be, md->id, &i, &end);
+ for (; i <= end; i++) {
+ dst_clu = READ_ONCE(bat_entries[i]);
+ if (dst_clu != BAT_ENTRY_NONE && dst_clu >= used_clus)
+ used_clus = dst_clu + 1;
+ }
+ }
+
+ /* The header is the last cluster, see delta_save_optional_header() */
+ hdr = ploop_md_first_entry(&md_root)->kmpage;
+ ext_off = le64_to_cpu(hdr->m_FormatExtensionOffset);
+ if (ext_off) {
+ ext_clu = SEC_TO_CLU(ploop, ext_off);
+ if (CLU_TO_SEC(ploop, ext_clu) != ext_off ||
+ ext_clu >= BAT_ENTRY_NONE) {
+ used_end = -EPROTO;
+ goto out;
+ }
+ if (ext_clu >= used_clus)
+ used_clus = ext_clu + 1;
+ }
+
+ used_end = CLU_TO_POS(ploop, used_clus);
+out:
+ ploop_free_md_pages_tree(&md_root);
+ return used_end;
+}
+ALLOW_ERROR_INJECTION(ploop_calc_used_end, ERRNO);
+
void ploop_put_delta(struct ploop *ploop, struct ploop_delta *delta)
{
int i;
diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
index 3a24769d0a8f..f7ead7fb396a 100644
--- a/drivers/md/dm-ploop-target.c
+++ b/drivers/md/dm-ploop-target.c
@@ -449,8 +449,9 @@ static struct ploop_worker *ploop_worker_create(struct ploop *ploop,
* ploop_dtr, as ploop_dtr is called on the old ploop after the new one is
* resumed, so we can't possibly modify the image from the old ploop context
* at this point, as new ploop may be already doing its own IO or
- * preallocation. This truncation should be done on ploop_*suspend (see
- * dm-qcow2 as it already implements that in qcow2_truncate_preallocations).
+ * preallocation. So this truncation is done on ploop_postsuspend (see
+ * ploop_truncate_unused_tail), and the new ploop picks up the new size
+ * of the top delta on metadata reload.
*/
static int ploop_ctr(struct dm_target *ti, unsigned int argc, char **argv)
{
@@ -746,10 +747,59 @@ static void ploop_presuspend_undo(struct dm_target *ti)
{
ploop_set_wants_suspend(ti, false);
}
+
+/*
+ * Truncate preallocations and the tail of the top delta, which became
+ * unused after discard. The next resumed ploop (maybe of reloaded
+ * table) re-reads the size of the top delta in ploop_preresume().
+ */
+static void ploop_truncate_unused_tail(struct ploop *ploop)
+{
+ struct ploop_delta *top = ploop_top_delta(ploop);
+ unsigned long flags;
+ loff_t end;
+ int ret;
+
+ lockdep_assert_held_write(&ploop->ctl_rwsem);
+
+ if (ploop_is_ro(ploop) || top->is_raw)
+ return;
+ if (ploop_is_standby(ploop))
+ return;
+ if (!ploop_no_pending_prealloc(ploop))
+ return;
+ if (i_size_read(file_inode(top->file)) != top->file_size) {
+ PL_ERR("Top delta is changed by someone else, don't truncate it");
+ return;
+ }
+
+ end = ploop_calc_used_end(ploop, top->file);
+ if (end < 0) {
+ PL_ERR("Can't find used end of top delta: %lld", end);
+ return;
+ }
+ if (end >= top->file_size)
+ return;
+
+ ret = do_ftruncate(top->file, end, 0);
+ if (ret) {
+ PL_ERR("Can't truncate unused tail of top delta: %d", ret);
+ return;
+ }
+
+ spin_lock_irqsave(&ploop->bat_lock, flags);
+ top->file_size = end;
+ if (top->file_preallocated_area_start > end)
+ top->file_preallocated_area_start = end;
+ spin_unlock_irqrestore(&ploop->bat_lock, flags);
+}
+
static void ploop_postsuspend(struct dm_target *ti)
{
struct ploop *ploop = ti->private;
+ unsigned int noio_flags;
+ down_write(&ploop->ctl_rwsem);
/*
* Let the next resumed ploop (maybe of reloaded table)
* see final size of the top delta. Don't wait in standby
@@ -765,10 +815,16 @@ static void ploop_postsuspend(struct dm_target *ti)
/*
* Data bios are completed before BAT writeback is finished
* (see ploop_put_piwb()), so wait for it explicitly to make
- * md pages clean for ploop_reload_metadata().
+ * md pages clean for ploop_reload_metadata() and
+ * ploop_calc_used_end().
*/
wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
- ploop_set_suspended(ti, true);
+ /* The device is suspended, see ploop_preresume() */
+ noio_flags = memalloc_noio_save();
+ ploop_truncate_unused_tail(ploop);
+ memalloc_noio_restore(noio_flags);
+ ploop->suspended = true;
+ up_write(&ploop->ctl_rwsem);
}
static int ploop_preresume(struct dm_target *ti)
{
diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
index 5750eafc8a1b..bdc8033c9cc5 100644
--- a/drivers/md/dm-ploop.h
+++ b/drivers/md/dm-ploop.h
@@ -37,7 +37,7 @@ struct ploop_pvd_header {
__u32 m_DiskInUse; /* Disk in use */
__u32 m_FirstBlockOffset; /* First data block offset (in sectors) */
__u32 m_Flags; /* Misc flags */
- __u8 m_Reserved[8]; /* Reserved */
+ __u64 m_FormatExtensionOffset; /* Format extension offset (in sectors) */
};
#pragma pack(pop)
@@ -643,6 +643,7 @@ extern void ploop_pio_prepare_offsets(struct ploop *ploop, struct pio *pio, u32
extern int ploop_setup_metadata(struct ploop *ploop, struct page *page,
bool check_in_use);
extern int ploop_reload_metadata(struct ploop *ploop);
+extern loff_t ploop_calc_used_end(struct ploop *ploop, struct file *file);
extern int ploop_read_delta_metadata(struct ploop *ploop, struct file *file,
struct rb_root *md_root, u32 *delta_nr_be);
extern void ploop_index_wb_init(struct ploop_index_wb *piwb,
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Devel] [PATCH hci-8.1 v5 11/11] dm-ploop: truncate unused tail of top delta on suspend
2026-10-13 7:55 ` [Devel] [PATCH hci-8.1 v5 11/11] dm-ploop: truncate unused tail of top delta on suspend Alexander Ivanov
@ 2026-10-13 8:20 ` no-reply
2026-10-13 11:00 ` Alexander Ivanov
0 siblings, 1 reply; 25+ messages in thread
From: no-reply @ 2026-10-13 8:20 UTC (permalink / raw)
To: den, Alexander Ivanov; +Cc: devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dm-ploop: no fsync between BAT writeback and top delta truncation in postsuspend
- [Low] dm-ploop: calc_used_end() WARNs on a clean-check that postsuspend does not fully wait for
--
commit 847969f12202a7013753f4e3c79c265f1c673b03
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
dm-ploop: truncate unused tail of top delta on suspend
This patch makes ploop_postsuspend() truncate the unused tail of the top
delta. The used end is computed by the new ploop_calc_used_end(), which
reads the on-disk BAT and the format extension offset. The truncation is
skipped for read-only, raw and standby devices, and when the file size
differs from top->file_size.
Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171
> diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
> --- a/drivers/md/dm-ploop-bat.c
> +++ b/drivers/md/dm-ploop-bat.c
[ ... ]
> +loff_t ploop_calc_used_end(struct ploop *ploop, struct file *file)
> +{
> + u32 i, end, nr_be, dst_clu, used_clus, *bat_entries;
[ ... ]
> + /* BAT on disk may miss allocated clusters until writeback is done */
> + if (WARN_ON_ONCE(!ploop_md_pages_are_clean(ploop)))
> + return -EBUSY;
[Severity: Low]
The commit message says "Skip it if md pages are not clean", but this
code raises a kernel WARN and then the caller logs an error.
ploop_postsuspend() only waits for inflight_md:
wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
ploop_md_pages_are_clean() also tests MD_DIRTY, MD_UPDATING,
wb_batch_llist, md->piwb and md->wait_llist.
In the error paths of ploop_process_one_deferred_bio(), the sequence is:
ploop_break_bat_update(ploop, md, piwb); /* drops inflight_md */
ploop_pio_endio(pio);
spin_lock_irqsave(&ploop->bat_lock, flags);
if (add_to_wblist)
clear_bit(MD_DIRTY, &md->status);
clear_bit(MD_UPDATING, &md->status);
Can suspend complete and reach ploop_postsuspend() with inflight_md == 0
while MD_DIRTY or MD_UPDATING are still set? If so, this would hit the
WARN_ON_ONCE and the truncation would be silently skipped.
Would it make sense to treat unclean md pages as an expected skip, without
the WARN and without the error message from ploop_truncate_unused_tail()?
[ ... ]
> diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
> --- a/drivers/md/dm-ploop-target.c
> +++ b/drivers/md/dm-ploop-target.c
[ ... ]
> + end = ploop_calc_used_end(ploop, top->file);
> + if (end < 0) {
> + PL_ERR("Can't find used end of top delta: %lld", end);
> + return;
> + }
> + if (end >= top->file_size)
> + return;
> +
> + ret = do_ftruncate(top->file, end, 0);
[Severity: Medium]
Is a flush of the top delta needed before this truncate?
ploop_calc_used_end() reads the BAT back with vfs_iter_read(), which can be
served from the page cache. The BAT page writes are issued by
ploop_index_wb_submit() as plain writes, with no flush or FUA. The only
vfs_fsync() of the top delta in the suspend path is in ploop_presuspend(),
which runs before ploop_postsuspend() waits for inflight_md:
ploop_presuspend()
vfs_fsync(ploop_top_delta(ploop)->file, 0);
ploop_postsuspend()
wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
ploop_truncate_unused_tail()
do_ftruncate(top->file, end, 0);
A BAT writeback that completes in that wait, for example one zeroing a
discarded cluster, is not covered by the earlier fsync.
If the host crashes after the size change is committed but before those BAT
pages are durable, the on-disk BAT can still reference clusters beyond the
new EOF. The next ploop_ctr() or ploop_reload_metadata() would then fail in
ploop_convert_bat_entries() with -EPROTO, which is the failure the commit
message says it is avoiding.
Should a vfs_fsync() of top->file be done before do_ftruncate()?
[ ... ]
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013075513.90618-1-alexander.ivanov@virtuozzo.com?part=11
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 25+ messages in thread