Re: [Devel] [PATCH hci-8.1 3/3] dm-qcow2: fix SEEK_HOLE/SEEK_DATA using an image freed by a merge

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-qcow2: qcow2_llseek_hole() holds inflight ref for whole walk, stalling ref switch
--

commit 042e70787bbad4a7f53038a208c72c6e24e2bfb5
Author: Denis V. Lunev <den@openvz.org>

dm-qcow2: fix SEEK_HOLE/SEEK_DATA using an image freed by a merge

This patch makes qcow2_llseek_hole() take an inflight reference with
qcow2_ref_inc() before it starts walking the image chain. It drops the
reference with qcow2_ref_dec() after the walk finishes. This is meant to
stop merge_backward complete from freeing an image while the walk is still
using it.

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

> 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

[ ... ]

> @@ -4841,18 +4841,25 @@ static int qcow2_llseek_hole_qio(struct qio *qio, int whence, loff_t *result)
>  loff_t qcow2_llseek_hole(struct dm_target *ti, loff_t offset, int whence)
>  {
> -	struct qcow2 *qcow2 = to_qcow2_target(ti)->top;
> +	struct qcow2_target *tgt = to_qcow2_target(ti);
>  	loff_t result = -EINVAL;
> +	struct qcow2 *qcow2;
>  	struct qio *qio;
> +	u8 ref_index;
>  	int ret;
>  
> +	/* Keep merge_backward complete from freeing images under the walk */
> +	qcow2 = qcow2_ref_inc(tgt, &ref_index);
>  	qio = alloc_seek_qio(qcow2, NULL, qcow2->hdr.size);
> -	if (!qio)
> +	if (!qio) {
> +		qcow2_ref_dec(tgt, ref_index);
>  		return -ENOMEM;
> +	}
>  
>  	seek_qio_set_sector(qio, to_sector(offset));
>  
>  	ret = qcow2_llseek_hole_qio(qio, whence, &result);
> +	qcow2_ref_dec(tgt, ref_index);

[Severity: Medium]
This isn't a bug in the use-after-free fix itself, but the inflight ref
is now held across the whole qcow2_llseek_hole_qio() walk.  That walk is
unbounded for a large sparse image and sleeps in wait_for_completion()
while metadata pages are read.

Several callers of qcow2_inflight_ref_switch() have to drain that ref
first, so each of them now waits for the whole scan:

qcow2_merge_backward_complete()
  qcow2_inflight_ref_switch()
    wait_for_completion(comp)

This runs under ctl_mutex, so other control messages queue behind it.

set_backward_merge_in_process()
  top->pause_submitting_qios = true;
  qcow2_inflight_ref_switch()

While this waits, qcow2_submit_embedded_qio() parks new requests on
paused_qios. Can guest I/O to the target stall until the seek finishes?

qcow2_flush_deferred_activity()
  qcow2_inflight_ref_switch()

This is reached from suspend and teardown. dm_llseek_hole() checks
dm_suspended_md() only before the walk, so a suspend that starts
mid-walk would wait for the whole scan.

I did not find a deadlock. The seek's metadata reads call qcow2_ref_inc()
after the index flip and get the other slot, and the worker does not wait
on the ref. So the stall is bounded by the length of the walk.

Could the commit message mention this cost? Would it be reasonable to
drop and retake the ref per chunk and revalidate the top image, so
the ref is not held for the whole scan?

>  	/* In case of error remap ENXIO as it have special meaning for llseek */
>  	if (ret < 0)
>  		return (ret == -ENXIO) ? -EINVAL : ret;

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103083708.131379-1-den@openvz.org?part=3

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