Re: Overlay limit bug

[Peter Krempa]

Adding libvir-list to cc. If you come across stuff that involves both
libvirt and qemu it's best to crosspost it to libvir-list too as I've
noticed this only accidentally.

On Mon, Oct 12, 2020 at 15:03:10 -0400, Yoonho Park wrote:
> I stumbled on a bug in qemu 4.2.0 (virsh 6.0.0) with a large number of
> overlays. I am using "qemu-img create" and "virsh snapshot-create-as" to
> create each overlay. When I run "virsh snapshot-create-as" for the 42nd
> overlay, I get "error: No complete monitor response found in 10485760

This error comes from libvirt's DoS protection. The response from qemu
is too large. This happens when 'query-named-block-nodes' is called on
too-deep backing chains as the reply contains both nested and linear
entries. Thus for a N deep backing chain, you get N + N-1 + N-2 ...
entries in the returned data.

Libvirt stops reading after 1MiB of json to prevent buffer overflows
from a rogue qemu.

> bytes: Numerical result out of range". However, I pulled down qemu 5.1.50
> (still using virsh 6.0.0), and it looks like the problem has disappeared
> which is great. Does anyone know the patch set that addressed this bug?

qemu patch:

commit facda5443f5a8676fb635b82ac1046ac6b6a67ce
Author: Peter Krempa <pkrempa@redhat.com>
Date:   Mon Jan 20 09:50:49 2020 +0100

    qapi: Allow getting flat output from 'query-named-block-nodes'

    When a management application manages node names there's no reason to
    recurse into backing images in the output of query-named-block-nodes.

    Add a parameter to the command which will return just the top level
    structs.

v4.2.0-1584-gfacda5443f

libvirt patches:

commit 95080cc8b470c977d53043d4eff3e30781f472eb
Author: Peter Krempa <pkrempa@redhat.com>
Date:   Tue Jan 21 16:51:40 2020 +0100

    qemu: Don't request nested entries in qemuBlockGetNamedNodeData

    Use the 'flat' flag for 'query-named-block-nodes' if qemu supports
    QEMU_CAPS_QMP_QUERY_NAMED_BLOCK_NODES_FLAT in qemuBlockGetNamedNodeData.

    We don't need the data so plumb in whether qemu supports the
    'flat' output.

    Signed-off-by: Peter Krempa <pkrempa@redhat.com>
    Reviewed-by: Ján Tomko <jtomko@redhat.com>

commit 855211bbf3ed45a73159f45eba1b15f05d771b76
Author: Peter Krempa <pkrempa@redhat.com>
Date:   Tue Jan 21 16:42:49 2020 +0100

    qemu: monitor: Add 'flat' parameter for qemuMonitorJSONQueryNamedBlockNodes

    Modern qemu allows to skip the nested redundant data in the output of
    query-named-block-nodes. Plumb in the support for the argument that
    enables it.

    Signed-off-by: Peter Krempa <pkrempa@redhat.com>
    Reviewed-by: Ján Tomko <jtomko@redhat.com>

commit 63610bd5fbe67ba9eb4f22f67c4bdab6eda8c041
Author: Peter Krempa <pkrempa@redhat.com>
Date:   Wed Feb 26 12:50:53 2020 +0100

    qemuCheckpointDiscardBitmaps: Use qemuBlockGetNamedNodeData
    
    Replace qemuMonitorBlockGetNamedNodeData by qemuBlockGetNamedNodeData.
    
    Signed-off-by: Peter Krempa <pkrempa@redhat.com>
    Reviewed-by: Ján Tomko <jtomko@redhat.com>

commit f886c9f33051fc03dd6c78134c92d31e7caf0c81
Author: Peter Krempa <pkrempa@redhat.com>
Date:   Tue Jan 21 16:33:12 2020 +0100

    qemu: monitor: Refactor variable cleanup in 
qemuMonitorJSONQueryNamedBlockNodes
    
    Use g_autoptr to get rid of the cleanup section.
    
    Signed-off-by: Peter Krempa <pkrempa@redhat.com>
    Reviewed-by: Ján Tomko <jtomko@redhat.com>

commit b7991c903cdd5b3c8b79a206584a4db81a6d4eaa
Author: Peter Krempa <pkrempa@redhat.com>
Date:   Tue Jan 21 15:13:47 2020 +0100

    qemu: capabilities: Add capability for the 'flat' argument of 
'query-named-block-nodes'
    
    Detect the presence of the flag and make it available internally as
    QEMU_CAPS_QMP_QUERY_NAMED_BLOCK_NODES_FLAT.
    
    Signed-off-by: Peter Krempa <pkrempa@redhat.com>
    Reviewed-by: Ján Tomko <jtomko@redhat.com>

v6.1.0-29-g95080cc8b4

> Also, does anyone know the "official" limit on the number of overlays that
> can be created and is there a qemu test that exercises this? I could not

Libvirt's official limit is 200 layers.

This is the documentation for the validation function explaining the
rationale:

/**
 * qemuDomainStorageSourceValidateDepth:
 * @src: storage source chain to validate
 * @add: offsets the calculated number of images
 * @diskdst: optional disk target to use in error message
 *
 * The XML parser limits the maximum element nesting to 256 layers. As libvirt
 * reports the chain into the status and in some cases the config XML we must
 * validate that any user-provided chains will not exceed the XML nesting limit
 * when formatted to the XML.
 *
 * This function validates that the storage source chain starting @src is at
 * most 200 layers deep. @add modifies the calculated value to offset the number
 * to allow checking cases when new layers are going to be added to the chain.
 *
 * Returns 0 on success and -1 if the chain is too deep. Error is reported.
 */
int
qemuDomainStorageSourceValidateDepth(virStorageSourcePtr src,
                                     int add,
                                     const char *diskdst)


> find an overlay limit test in tests/qemu-iotests.

I'd guess qemu doesn't really limit that, but at some point you'll get
too much of a performance penalty form traversing all the structs.