Re: [Devel] [PATCH hci-8.1 v9 09/10] dm-qcow2: flag the image for check after a reverted L1/L2 writeback

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-qcow2: revert_l_entries_update() skips spin_lock() on the WARN continue path
--

commit 088bd04e227a501d72562ed2321e2ca2f75e3df0
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-qcow2: flag the image for check after a reverted L1/L2 writeback

This patch adds an MD_LX_REVERTED page status that is set when an L1/L2
writeback fails and the page is reverted. The flag is cleared on the next
successful writeback. On md writeback flush, a page that is still marked
sets a new lx_writeback_error flag. That flag is reported by get_errors and
included in qcow2_wants_check().

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171

> 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

[ ... ]

> @@ -1362,6 +1362,7 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>  		if (cleared)
>  			continue; /* no cluster was allocated */
>  
> +		wbd->md->status |= MD_LX_REVERTED;
>  		spin_unlock(&qcow2->md_pages_lock);
>  		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
>  

[Severity: Medium]
Can the lock be left unheld on the WARN path in this loop?

The loop body in revert_l_entries_update() continues like this after the
quoted lines:

	ret = qcow2_handle_r1r2_maps(qcow2, pos, NULL, &r1, &r2, false);
	if (WARN_ON_ONCE(ret <= 0))
		continue;

	mark_cluster_unused(qcow2, r2.md, r2.index_in_page, pos);
	spin_lock(&qcow2->md_pages_lock);

The lock is dropped just above, but the continue skips the closing
spin_lock(). The next iteration then reads wbd->md->page, calls
set_u64_to_be_page(), and executes the new store to wbd->md->status
without md_pages_lock. It may also reach spin_unlock() a second time on a
lock that is not held.

The caller does not expect this:

complete_wbd() {
	spin_lock_irqsave(&qcow2->md_pages_lock, flags);
	revert_l_entries_update(qcow2, wbd);
	clear_writeback_status(qcow2, wbd->md, wbd->ret,
			       &wait_list, &end_list);
	spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
	...
}

If the lock is not re-acquired, clear_writeback_status() and
spin_unlock_irqrestore() run on a lock that is no longer owned. That can
corrupt the spinlock and IRQ state, and it races with other users of
md_pages_lock.

qcow2_handle_r1r2_maps() can return a value <= 0 in several cases. It
returns -ENOENT when the R1 entry is zero, and -EIO on an invalid R1 entry
or when the refcount sanity check fails (entry > 1 with no snapshots). It
returns 0 when an R1/R2 page is not cached.

The comment says R1/R2 "should be cached", but I could not show that none
of these returns can happen. For example, a failed md read or a failed
relocate_refcount_table() can erase a cached page.

The unlock/relock imbalance is not new, but this patch adds an
unlocked-path store to md->status in the same loop. The later patch in the
series, "dm-qcow2: truncate unused tail of image on suspend", does not
change this handling.

Could the WARN path re-take md_pages_lock before continuing, or could the
loop be restructured so the lock state is the same on every path?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028150645.3222391-1-alexander.ivanov@virtuozzo.com?part=9

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel