Re: [Devel] [PATCH hci-8.1 v2 1/3] dm-qcow2: fix a failed refcount table relocation leaking md pages

[Konstantin Khorenko <khorenko@virtuozzo.com>]

I won't apply this version, please send v2 with the changes below.

On 11/3/26 10:08, Denis V. Lunev wrote:
>  err_free_r2_pages:
> -	for (i = end; i < r2_end; i += clu_size) {
> +	for (i = new_md_start; i < new_md_end; i += PAGE_SIZE) {
>  		ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
>  		if (ret <= 0)
>  			break;

Item 1 of the cover letter reply: after a successful header write, do
not free the new md pages in this loop. Send the gotos that follow the
header write to a label after the loop that only returns -EIO, and
update the "Note for a future reader" to match.

This version should not go in as is: it trades one rare failure for a
worse one.

  +------------------------+----------------------+--------------------+
  |                        | without this patch   | with this patch    |
  +------------------------+----------------------+--------------------+
  | a page allocation or   | every write past the | fixed              |
  | the header write fails | old refcount         |                    |
  | during the relocation  | coverage gets EIO    |                    |
  |                        | until suspend/resume |                    |
  +------------------------+----------------------+--------------------+
  | an image with 4K       | WARN + EIO           | use-after-free: a  |
  | clusters and           |                      | page still on the  |
  | refcount_bits=64 grows |                      | writeback list is  |
  | to about 511G          |                      | freed, memory gets |
  | (refcount_bits=32: 2T) |                      | corrupted          |
  +------------------------+----------------------+--------------------+

EIO is visible and suspend/resume cures it. Memory corruption can crash
the node or silently damage someone else's data. Both cases are rare,
but the second one is normal work with a valid image, no failure needed.

How the second row happens: one of the gotos after the header write is
reachable on a valid image, and by then this loop frees a dirty page.
With 4K clusters and refcount_bits=64 the relocation at about 511G makes
the new reftable fill a whole refcount group, so that group gets no
refblock and the mark-used loop fails at i = pos (refcount_bits=32 gets
there at about 2T).

The connect loop has already dirtied the page at new_md_start and queued
it for writeback. Before this patch that page leaked and the device got
EIO. With the patch it is freed while still on the writeback list, and
the slow writeback timer then works on freed memory.

Item 6 of the cover letter reply, a separate patch: the missing refblock
for the group of pos is a bug of its own, there since the driver was
added. The connect loop gives a refblock only to the groups the new
refblocks sit in.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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