Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] dm-qcow2: used-end shrink trusts refcounts without any in-kernel crash guard - [Medium] dm-qcow2: unvalidated on-disk refcounts steer destructive truncation in qcow2_calc_used_end() - [Medium] dm-qcow2: GFP_KERNEL md page allocs in postsuspend scan outside NOIO scope --
commit e35086f1c84ad69710c30b217b7a28fd6da3991c Author: Alexander Ivanov <[email protected]> dm-qcow2: truncate unused tail of image on suspend This patch adds qcow2_calc_used_end(), which scans the refcount blocks backward and returns the end of the last cluster with a non-zero refcount. On postsuspend, qcow2_truncate_unused_tail() now truncates the image file to that used end instead of only dropping the preallocated area. The scan is skipped after an md writeback error, and for a dirty image when kernel_sets_dirty_bit is set. > diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c > index 27ae22495f37..d96532b3753e 100644 > --- a/drivers/md/dm-qcow2-map.c > +++ b/drivers/md/dm-qcow2-map.c [ ... ] > +/* > + * Return the end of the last cluster with non-zero refcount. Must be > + * called on a suspended device after md writeback. > + */ > +loff_t qcow2_calc_used_end(struct qcow2 *qcow2) > +{ [ ... ] > + if (!r1_entry_is_valid(qcow2, get_u64_from_be_page(r1_md->page, > + r1.index_in_page))) > + return -EIO; [Severity: Medium] r1_entry_is_valid() only checks the reserved bits and the cluster alignment of the R1 entry. Is the refblock offset checked anywhere against the file size or against overlap with other metadata? The open path does not check it either. qcow2_parse_header() and qcow2_check_convert_hdr() do not validate R1 entries or refblock offsets. The only reftable check there is: if (qcow2->reftable_max_file_size < qcow2->file_size) goto out; If an R1 entry is zero or points at the wrong block, can the scan skip or miss clusters that L2 still maps? The file would then be truncated below live guest data. A refblock beyond EOF fails the read, and the scan returns -EIO and falls back to the old behaviour. > + > + ret = calc_r2_page_id(qcow2, &r1, &r2); [ ... ] > + for (i = r2.index_in_page; i >= 0; i--, pos -= clu_size) { > + if (get_r2_entry(qcow2, r2.md, i)) > + return min_t(loff_t, pos + clu_size, > + > qcow2->file_preallocated_area_start); > + } [Severity: Medium] The used end comes only from the on-disk refcounts. There is no cross-check against L1/L2 offsets. If a cluster is mapped by L2 but has refcount 0, for example after a crash, doesn't this loop cut it off? > + } > + > + return -EIO; > +} [ ... ] > diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c > index 1b7a75239291..7587e8f7ac0b 100644 > --- a/drivers/md/dm-qcow2-target.c > +++ b/drivers/md/dm-qcow2-target.c [ ... ] > + if (tgt->md_writeback_error) { > + QC_INFO(ti, "Skip shrinking to used end after md writeback > error"); > + } else if (kernel_sets_dirty_bit && qcow2->opened_dirty) { > + QC_INFO(ti, "Skip shrinking to used end of dirty image"); > + } else { > + used_end = qcow2_calc_used_end(qcow2); > + if (used_end < 0) > + QC_ERR(ti, "Can't find used end: %pe", > ERR_PTR(used_end)); > + else > + end = used_end; > + } > + > if (end == qcow2->file_size) > return; > > ret = qcow2_truncate_safe(qcow2->file, end); [Severity: Medium] The only in-kernel guard before this truncate is the opened_dirty check. It applies only when kernel_sets_dirty_bit is set, which defaults to false. A v2 image has no dirty bit, because qcow2_set_image_file_features() returns early for version 2, so the guard can never apply to it. The commit message says "the scan is safe only because userspace checks the image before an RW open". Is that precondition enforced anywhere in the kernel? The dirty-bit consistency check in qcow2_check_convert_hdr() is commented out: // if (!is_ro && kernel_sets_dirty_bit != // !(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT)) // return kernel_sets_dirty_bit ? -EUCLEAN : -ENOLCK; qcow2_wants_check() reflects only md_writeback_error and truncate_error from the current session. Suppose a crash leaves a refcount at 0 for a cluster that L2 already references, and the image is opened RW without a userspace check. Will qcow2_truncate_safe() then permanently cut off mapped clusters? The new end is stored into file_preallocated_area_start and file_size, so the loss cannot be recovered. The old code only dropped the preallocated area, so it could not remove mapped data. Could the kernel refuse the scan for images whose consistency it cannot verify, or could the commit message explain how the userspace check is enforced? [ ... ] > @@ -1157,7 +1170,7 @@ static void qcow2_postsuspend(struct dm_target *ti) > int ret; > > qcow2_flush_deferred_activity_all(to_qcow2_target(ti)); > - qcow2_truncate_preallocations(ti); > + qcow2_truncate_unused_tail(ti); [Severity: Medium] qcow2_truncate_unused_tail() now reaches qcow2_calc_used_end(), which loops over qcow2_md_page_read_sync(). That calls qcow2_alloc_and_insert_md_page(), which uses kmalloc(GFP_KERNEL) and alloc_page(GFP_KERNEL). The backing-file read can also allocate memory. The call chain is: dm_suspend()->dm_table_postsuspend_targets()->qcow2_postsuspend() ->qcow2_truncate_unused_tail()->qcow2_calc_used_end() ->qcow2_md_page_read_sync()->qcow2_alloc_and_insert_md_page() dm_suspend() does not wrap the postsuspend hooks in memalloc_noio_save(). Can direct reclaim triggered by these allocations wait on I/O to this same suspended dm device, for example if swap is on it? That I/O could not complete until resume, and resume is waiting for postsuspend. The old postsuspend path allocated no md pages, so this exposure is new. I did not find an in-tree user that swaps onto a qcow2 device, so this may be theoretical. Would wrapping qcow2_calc_used_end() or qcow2_postsuspend() in memalloc_noio_save()/memalloc_noio_restore(), or using GFP_NOIO in qcow2_alloc_and_insert_md_page(), be appropriate? -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=10 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
