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

[Alexander Ivanov]

> [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.

Fixed in "dm-ploop: reload metadata on resume": ploop_postsuspend() now 
waits on inflight_md_wq before reload/truncation relies on clean md 
pages, and the wakers use wake_up() since the waiter is uninterruptible. 
With that, the WARN_ON_ONCE() remains only for a real invariant violation.


> [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.
Already handled: ploop_truncate_unused_tail() (and thus 
ploop_calc_used_end() → ploop_read_delta_metadata()) is called under 
memalloc_noio_save()/memalloc_noio_restore() in ploop_postsuspend(), 
same as ploop_reload_metadata() in ploop_preresume() (added in 
"dm-ploop: reload metadata on resume").


> [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?
Fixed in the next version: the extension offset is now converted to
clusters, not bytes, so the shift can't wrap and the result is always
cluster-aligned.


> [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?
Yes, this is guaranteed by the format as implemented by the ploop library,
which is the only writer of the extension: delta_save_optional_header()
appends dirty bitmap blocks at EOF first and then appends the header cluster
behind them, setting m_FormatExtensionOffset to it. 
check_ext_blocks_from_raw()
rejects L1 pointers beyond m_FormatExtensionOffset as corruption, and
truncate_ext_blocks() relies on the same layout when removing the extension.
I've documented this in the comment in ploop_calc_used_end().
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/devel/attachments/20261008/5bd89b01/attachment.html>