Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] dm-qcow2: stale data_clu_pos in fast path can write to a freed or reused cluster --
Commit 34396f8c5df05ca113c51e62c4510b07f74a9fdc Author: Andrey Zhadchenko <[email protected]> dm-qcow2: implement per-qio fast processing This patch makes do_qio_work() try to process each split qio immediately with process_one_qio(..., true). Qios that need metadata changes go onto the deferred list for do_qcow2_work(). As a result, parse and submit now run in parallel with the main kwork. > 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 [ ... ] > +static void qio_try_fast(struct qio *qio, struct list_head *deferred_qios) > +{ > + list_del(&qio->link); > + > + if (process_one_qio(qio->qcow2, qio, true)) > + list_add_tail(&qio->link, deferred_qios); > +} [Severity: High] Can a write submitted from here go to a cluster that has been freed or reused? process_one_qio() runs handle_metadata() -> parse_metadata(), which drops md_pages_lock and caches the result: unlock: spin_unlock_irq(&qcow2->md_pages_lock); if (ret <= 0) /* Err, delayed, DATA is not allocated, or zero read */ return ret; map->data_clu_pos = pos; The L2 entry is not re-validated after this point. Before this patch, parse and submit ran sequentially in main kwork, so main kwork could not change the index between the two steps. The recheck added for the race with writeback in perform_rw_mapped() looks only at MD_WRITEBACK, and at MD_DIRTY with the changed bit set: spin_lock_irqsave(&qcow2->md_pages_lock, flags); if (__delay_if_writeback(qcow2, md, index_in_page, &qio, false)) { ... } if ((md->status & MD_DIRTY) && test_bit(index_in_page, md->wbd->changed_indexes)) { md->wbd->nr_submitted++; qio->data = md->wbd; } spin_unlock_irqrestore(&qcow2->md_pages_lock, flags); ... map_and_submit_rw(qcow2, map->data_clu_pos, qio); It does not call delay_if_locked() and does not re-read the L2 entry. If a discard's L2 update is DIRTY, the write is only attached to wbd and still goes to the old data_clu_pos. If the discard's writeback has already completed, the write goes to the stale position with no check. On the main kwork side, the discard sequence is prepare_cluster_discard() -> process_indexes_write() -> process_indexes_end(). The last step waits only for wpc readers and then calls dec_cluster_usage(), which frees the cluster for reuse. It does not wait for fast-path writes that parsed the old mapping before the index was locked. A possible sequence: fast worker: parse_metadata() caches data_clu_pos (worker is delayed, for example by preemption) main kwork: discard locks the index, rewrites the L2 entry, completes writeback, and process_indexes_end() frees the cluster fast worker: perform_rw_mapped() -> map_and_submit_rw() writes to the old data_clu_pos, which may now belong to other data Is there an inflight ref, an index-lock recheck, or a wait for in-flight writes that closes this window? I could not find one. It also looks like the later patches in the series do not close it: one only batches the do_qio_work() calls per CPU, and the other only sets DM_TARGET_NOWAIT. The commit message says "a write may reach perform_rw_mapped() after main kwork switched its L2 page from DIRTY to WRITEBACK. perform_rw_mapped() parks such a write until the writeback ends." Is this the only race introduced by running process_one_qio() in parallel with do_qcow2_work()? The discard and cluster reuse race described above is not mentioned. > + > static void do_qio_work(struct work_struct *ws) > { [ ... ] -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=5 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
