<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">[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.
</pre>
</blockquote>
<p>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.</p>
<p><br>
</p>
<p>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">[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.
</pre>
</blockquote>
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").</p>
<p><br>
</p>
<p>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">[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<sup class="moz-txt-sup"><span style="display:inline-block;width:0;height:0;overflow:hidden">^</span>55</sup> 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?</pre>
</blockquote>
Fixed in the next version: the extension offset is now converted
to<br>
clusters, not bytes, so the shift can't wrap and the result is
always<br>
cluster-aligned.</p>
<p><br>
</p>
<p>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">[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?
</pre>
</blockquote>
Yes, this is guaranteed by the format as implemented by the ploop
library,<br>
which is the only writer of the extension:
delta_save_optional_header()<br>
appends dirty bitmap blocks at EOF first and then appends the
header cluster<br>
behind them, setting m_FormatExtensionOffset to it.
check_ext_blocks_from_raw()<br>
rejects L1 pointers beyond m_FormatExtensionOffset as corruption,
and<br>
truncate_ext_blocks() relies on the same layout when removing the
extension.<br>
I've documented this in the comment in ploop_calc_used_end().</p>
</body>
</html>