Re: vmd(8): refcount lazily-allocated refcount blocks in qcow2 writer

Chris Cunningham <[email protected]> writes:

> On Monday, June 15th, 2026 at 9:25 AM, Dave Voutila <[email protected]> wrote:
>
>> Chris Cunningham <[email protected]> writes:
>>
>> > Hello again,
>> >
>> > This is the second of the two patches I mentioned, split into its own
>> > email. I think I've found a small qcow2 metadata leak in vmd(8)'s qcow2
>> > writer, though I may be misreading the intent of the code, so please
>> > correct me if so.
>> >
>> > As far as I can tell, when a guest grows a qcow2 image past a 2GB
>> > refcount-block boundary, vmd lazily allocates a new refcount block and
>> > installs it in the refcount table, but doesn't mark the new refcount
>> > block's own cluster as allocated.
>> >
>> > qemu-img check then reports one leaked metadata cluster per 2GB of
>> > allocated image data. For example, on one affected 40GB image:
>> >
>> > ERROR cluster 32769 refcount=0 reference=1
>> > ERROR cluster 65537 refcount=0 reference=1
>> > ERROR cluster 98305 refcount=0 reference=1
>> > ERROR cluster 131073 refcount=0 reference=1
>> > ERROR cluster 163841 refcount=0 reference=1
>> > ERROR cluster 196609 refcount=0 reference=1
>> > ERROR cluster 229377 refcount=0 reference=1
>> > ERROR cluster 262145 refcount=0 reference=1
>> >
>> > The pattern is N * 32768 + 1 with the default 64KB cluster size and
>> > 16-bit refcounts. Each refcount block covers 32768 clusters, or 2GB.
>> >
>> > The first refcount block created by vmctl create is fine. The problem
>> > seems to be only in the runtime lazy allocation path in inc_refs().
>> > When the refcount table entry is zero, inc_refs() allocates a new
>> > refcount block at disk->end and writes that offset into the refcount
>> > table. The new block is then reachable from qcow2 metadata, but its own
>> > refcount entry remains zero.
>> >
>> > The diff below marks the newly allocated refcount block itself in use
>> > after installing it in the refcount table. The refcount-table entry is
>> > written before the recursive call, so each refcount region enters the
>> > allocation path at most once and the recursion terminates (depth 1 in
>> > the common case, where the new block lands in the same region as the
>> > cluster that triggered the allocation). The recursive call into the
>> > allocator was the part I was least sure about, so I'd welcome a closer
>> > look there.
>> >
>> > The corruption is metadata-only as far as I can see: data and the L1/L2
>> > mapping tables are unaffected and guests keep running. qemu-img check
>> > -r all repairs the leaks, but qemu-img resize refuses the image until
>> > it is repaired.
>> >
>> > I reproduced this both with real vmd-written qcow2 images and with a
>> > small standalone harness that drives vioqcow2.c past the 2GB boundary.
>> > Before this change, the harness reports self_refs=0 for the lazy
>> > refcount block. After this change, it reports self_refs=1.
>>
>> I suspect you're correct. Your diff below looks mangled though. Luckily
>> it looks like a 1 line change?
>
> Yes, just a one-line change. Sorry about the formatting.
>

Thanks again. Finally got around to testing. ori@ said it looked good
and the reasoning makes sense.

>>
>> I'll need to take some time to read through vioqcow2.c to confirm this
>> but I'd suspect if qemu-img is happy and vmd works, it's correct. Unless
>> someone else more intimately familiar with qcow2 can jump on this, it
>> may take me a few days to find the time to double check. Given this is
>> dealing with data persistence I don't want to get this wrong. :D
>>
>> >
>> > Thanks,
>> > Chris
>> >
>> > diff --git a/usr.sbin/vmd/vioqcow2.c b/usr.sbin/vmd/vioqcow2.c
>> > index 917cba2cbc0..79a481f3ee5 100644
>> > --- a/usr.sbin/vmd/vioqcow2.c
>> > +++ b/usr.sbin/vmd/vioqcow2.c
>> > @@ -621,6 +621,13 @@ inc_refs(struct qcdisk *disk, off_t off, int newcluster)
>> > buf = htobe64(l2cluster);
>> > if (pwrite(disk->fd, &buf, sizeof(buf), l1off) != 8)
>> > fatal("%s: failed to write ref block", __func__);
>> > + /*
>> > + * The newly allocated refcount block cluster must itself
>> > + * be marked in use, or qcow2 metadata leaks one cluster
>> > + * per refcount block (one per 2 GiB of allocated data at
>> > + * the default 64K cluster / 16-bit refcount sizing).
>> > + */
>> > + inc_refs(disk, l2cluster, 1);
>> > }
>> >
>> > refs = 1;
>>
>>