Re: [Devel] [PATCH hci-8.1 v2 6/6] dm-ploop: truncate unused tail of top delta on suspend

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] dm-ploop: unvalidated m_FormatExtensionOffset can truncate delta to unaligned size
- [Medium] dm-ploop: GFP_KERNEL BAT read in postsuspend() can deadlock via reclaim IO
- [Low] dm-ploop: WARN_ON_ONCE in ploop_calc_used_end() can fire on a benign inflight_md race
- [Low] dm-ploop: tail truncation assumes CBT header is the last extension cluster
--

commit 209cb17124d4cf7236cb1d3d9e28ea5cc4361bbf
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: truncate unused tail of top delta on suspend

This patch adds ploop_calc_used_end(), which reads the on-disk BAT and the
format extension offset to find the end of the used part of the top delta.
ploop_postsuspend() now truncates the top delta to that end under
ctl_rwsem. The header field m_Reserved is renamed to m_FormatExtensionOffset.

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

> diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
> --- a/drivers/md/dm-ploop-bat.c
> +++ b/drivers/md/dm-ploop-bat.c

[ ... ]

> +loff_t ploop_calc_used_end(struct ploop *ploop, struct file *file)
> +{

[ ... ]

> +	/* BAT on disk may miss allocated clusters until writeback is done */
> +	if (WARN_ON_ONCE(!ploop_md_pages_are_clean(ploop)))
> +		return -EBUSY;

[Severity: Low]
Can this WARN_ON_ONCE() fire on a benign race? The commit message
describes the not-clean case as something to skip, but here it raises a
warning backtrace (and would panic with panic_on_warn).

In ploop_put_piwb(), the data pios are ended first, and inflight_md is
decremented only afterwards:

    ploop_bat_write_finish(piwb->pio, piwb, piwb->bi_status);
    ...
    if (atomic_dec_and_test(&piwb->ploop->inflight_md))
            wake_up_interruptible(&piwb->ploop->inflight_md_wq);

dm suspend can see those bios complete and reach ploop_postsuspend()
while inflight_md is still 1. ploop_postsuspend() only calls
ploop_wait_prealloc(), and does not wait on inflight_md_wq. So
ploop_md_pages_are_clean() can return false with no I/O error involved.

ploop_truncate_unused_tail() then also prints "Can't find used end of top
delta: -16" and skips the truncation. Should this be a plain condition,
or should ploop_postsuspend() wait on inflight_md_wq first? The same
pattern is already in ploop_reload_metadata(), but this adds a second
site.

[ ... ]

> +	ret = ploop_read_delta_metadata(ploop, file, &md_root, &nr_be);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Can this full BAT read deadlock when called from ploop_postsuspend()?

ploop_read_delta_metadata() allocates with GFP_KERNEL, both through
ploop_prealloc_md_pages() (alloc_page()) and in:

    bvec = kvmalloc(sizeof(*bvec) * nr_segs, GFP_KERNEL);

dm_suspend() calls dm_table_postsuspend_targets() after IO to the device
has been stopped, with suspend_lock held and no memalloc_noio scope.
Direct reclaim here could write back dirty pages of a filesystem on the
ploop device, or swap out to it. Those bios are deferred because the
device is suspended, so the reclaiming task waits for a resume. The
resume needs md->suspend_lock, which the suspending task still holds.

The old ploop_postsuspend() did no allocations. Would wrapping the call
in memalloc_noio_save()/memalloc_noio_restore() be appropriate? The same
hazard already exists in ploop_reload_metadata() from ploop_preresume(),
but this patch adds the postsuspend site.

[ ... ]

> +	/* Format extension header is the last cluster of extension */
> +	hdr = ploop_md_first_entry(&md_root)->kmpage;
> +	ext_off = le64_to_cpu(hdr->m_FormatExtensionOffset);
> +	if (ext_off) {
> +		ext_end = to_bytes(ext_off) + CLU_SIZE(ploop);

[Severity: Medium]
Can an unvalidated m_FormatExtensionOffset lead to an unaligned
truncation? This field used to be m_Reserved[8] and was never
interpreted. ploop_delta_check_header() and ploop_setup_metadata() do not
look at it.

If ext_off is sector-aligned but not cluster-aligned, ext_end is not
cluster-aligned. If it is also above the BAT-derived used_end and below
top->file_size, ploop_truncate_unused_tail() passes it to do_ftruncate().

The next ploop_preresume() -> ploop_reload_metadata() ->
ploop_read_delta() -> ploop_check_delta_length() then hits:

    if (loff & (CLU_SIZE(ploop) - 1))
            return -EPROTO;

and the device cannot be resumed.

Also, to_bytes() is an unsigned long shift by 9, so a large ext_off such
as 2^55 or more wraps, and the wrapped value can land in the same range.

Should ext_off be required to be cluster-aligned and checked for
overflow, with truncation skipped when it is out of range?

> +		if (ext_end > used_end)
> +			used_end = ext_end;
> +	}

[Severity: Low]
Is it guaranteed that the format extension header is the last cluster of
the extension? The code assumes the extension ends one cluster after its
offset:

    ext_end = to_bytes(ext_off) + CLU_SIZE(ploop);

If a CBT extension ever has data after its header cluster, anything beyond
ext_end that lies past the last BAT-referenced cluster is cut off by
do_ftruncate() in ploop_truncate_unused_tail(). That loss is irreversible.

The only support for this layout is the commit message ("as userspace
does") and the code comment. Could the layout be documented in the code or
in a header comment?

[ ... ]

> diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
> --- a/drivers/md/dm-ploop-target.c
> +++ b/drivers/md/dm-ploop-target.c

[ ... ]

> +	end = ploop_calc_used_end(ploop, top->file);
> +	if (end < 0) {
> +		PL_ERR("Can't find used end of top delta: %lld", end);
> +		return;
> +	}

[Severity: Low]
This isn't a bug, but when ploop_calc_used_end() returns -EBUSY for the
not-clean skip case, this path reports it as an error on top of the
WARN_ON_ONCE() in ploop_calc_used_end(). Should the skip case be silent?

[ ... ]

> +	ret = do_ftruncate(top->file, end, 0);

[ ... ]

> +	ploop_wait_prealloc(ploop);
> +	ploop_truncate_unused_tail(ploop);
> +	ploop->suspended = true;
> +	up_write(&ploop->ctl_rwsem);

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

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