Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] dm-qcow2: unused-tail truncation on suspend trusts refcounts with no kernel-side guard - [Medium] dm-qcow2: GFP_KERNEL md page reads in postsuspend can deadlock on reclaim --
commit 74d743673c019d9879114318e501a9a5de73c937 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 from file_preallocated_area_start. It returns the end of the last cluster with a non-zero refcount. qcow2_truncate_unused_tail() now truncates the image file to that point on postsuspend. It falls back to the old behaviour on scan errors, after a metadata writeback error, and for dirty images when kernel_sets_dirty_bit is set. > diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c > --- a/drivers/md/dm-qcow2-map.c > +++ b/drivers/md/dm-qcow2-map.c [ ... ] > +loff_t qcow2_calc_used_end(struct qcow2 *qcow2) > +{ [ ... ] > + if (!r1_md || r1_md->id != r1.page_id) { > + ret = qcow2_md_page_read_sync(qcow2, r1.page_id, > &r1_md); > + if (ret) > + return ret; > + } [ ... ] > + ret = qcow2_md_page_read_sync(qcow2, r2.page_id, &r2.md); > + if (ret) > + return ret; > + > + 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] Can this scan allocate memory in a way that deadlocks under reclaim? qcow2_calc_used_end() is called from the postsuspend hook: qcow2_postsuspend() qcow2_truncate_unused_tail() qcow2_calc_used_end() qcow2_md_page_read_sync() qcow2_alloc_and_insert_md_page() kmalloc(sizeof(**md), GFP_KERNEL) alloc_page(GFP_KERNEL) Every R1 or R2 page that is not already in the md cache is read this way, and then read from the image file. Neither the dm core nor the driver sets a memalloc_noio or memalloc_nofs scope around postsuspend. Direct reclaim from these GFP_KERNEL allocations could issue writeback to a filesystem or swap on this same suspended device, or on a device stacked on it. That I/O cannot complete until resume, and resume waits for postsuspend. Before this patch, postsuspend only used the already cached page 0 through qcow2_set_image_file_features(). The other allocations in the I/O and metadata paths use GFP_NOIO or run under PF_MEMALLOC_NOIO. Should the scan run under a memalloc_noio_save()/memalloc_noio_restore() scope, or use GFP_NOIO allocations? > diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c > --- 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); [Severity: Medium] Can this truncation cut off live guest data? The used end is derived from refcounts alone, with no cross-check against L2. If a cluster has refcount zero but is still referenced by L2, the cut point moves below live data and qcow2_truncate_safe() discards it permanently. The commit message says refcounts may be below real usage after a crash, since both QEMU with lazy_refcounts and this driver write L2 before refcounts. The only guards here are md_writeback_error and kernel_sets_dirty_bit && opened_dirty. A v2 image has no dirty bit. qcow2_check_convert_hdr() returns early for version 2, so opened_dirty stays false. In the default mode kernel_sets_dirty_bit is false, so the opened_dirty guard is skipped. The ctr-time 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() covers only md_writeback_error and truncate_error, which are errors from the current session. Nothing in the driver detects an on-disk inconsistency left by an earlier crash. The commit message says the scan is safe "only because userspace checks the image before an RW open". Can the driver verify that assumption? Would it be possible to refuse the truncation when the cut range is still referenced by L2, or to make the shrink opt-in? -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=10 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
