From patchwork Tue Feb 20 22:03:00 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 10230927 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D7CAA60392 for ; Tue, 20 Feb 2018 22:04:09 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C42C3288A9 for ; Tue, 20 Feb 2018 22:04:09 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B68FF28947; Tue, 20 Feb 2018 22:04:09 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 91855288A9 for ; Tue, 20 Feb 2018 22:04:08 +0000 (UTC) Received: from localhost ([::1]:57980 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eoG19-0006Xo-UH for patchwork-qemu-devel@patchwork.kernel.org; Tue, 20 Feb 2018 17:04:07 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52284) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eoG0U-00069g-D7 for qemu-devel@nongnu.org; Tue, 20 Feb 2018 17:03:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eoG0T-0002V9-CE for qemu-devel@nongnu.org; Tue, 20 Feb 2018 17:03:26 -0500 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:51584 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eoG0N-0002Rx-36; Tue, 20 Feb 2018 17:03:19 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 532EC402290A; Tue, 20 Feb 2018 22:03:08 +0000 (UTC) Received: from [10.10.122.122] (ovpn-122-122.rdu2.redhat.com [10.10.122.122]) by smtp.corp.redhat.com (Postfix) with ESMTP id 89E1810A96E0; Tue, 20 Feb 2018 22:03:01 +0000 (UTC) To: Alberto Garcia , qemu-devel@nongnu.org References: <20180220170135.5299-1-berto@igalia.com> <2d9713f2-87c1-12ba-3f46-a9a476279cda@redhat.com> From: Eric Blake Organization: Red Hat, Inc. Message-ID: <6064e2b5-c7aa-7ec1-b917-5431f0dc57a8@redhat.com> Date: Tue, 20 Feb 2018 16:03:00 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <2d9713f2-87c1-12ba-3f46-a9a476279cda@redhat.com> Content-Language: en-US X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 20 Feb 2018 22:03:08 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 20 Feb 2018 22:03:08 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'eblake@redhat.com' RCPT:'' X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 66.187.233.73 Subject: Re: [Qemu-devel] [PATCH v2] specs/qcow2: Fix documentation of the compressed cluster descriptor X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , qemu-block@nongnu.org, Max Reitz Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On 02/20/2018 01:40 PM, Eric Blake wrote: > On 02/20/2018 11:01 AM, Alberto Garcia wrote: > > tl:dr; I think we need a v3 with even more clarification. > > > I'm also making an additional observationn: Due to the pigeonhole > principle and the fact that the compression stream adds metadata, we > KNOW that there are some (rare) cases where attempting to compress data > will actually result in an INCREASE in size ('man gzip' backs up this > claim, calling out a worst case -0.015% compression ratio, or 15 bytes > added for every 1000 bytes of input, on uncompressible data).  So > presumably, we should state that a cluster can only be written in > compressed form IF it occupies less space than the uncompressed cluster > (we could also allow a compressed form that occupies the same length as > the uncompressed cluster, but that's a waste of CPU cycles). > > Once we have that restriction stated, then it becomes obvious that a > compressed cluster should never REQUIRE using more than one host cluster > (and this is backed up by qcow2_alloc_bytes() asserting that size <= > s->cluster_size).  Where things get interesting, though, is whether we > PERMIT a compressed cluster to overlap a host cluster boundary. > Technically, it might be possible, but qemu does NOT do that (again, > looking at qcow2_alloc_bytes() - we loop if free_in_cluster < size) - so > we may want to be explicit about this point to prevent OTHER > implementations from creating a compressed cluster that crosses host > cluster boundaries (right now, I can't see qcow2_decompress_cluster() > validating it, though - YIKES). That said, a simple patch to try this: * uncompressed and the memory overhead can be avoided. The buffers * are freed in .bdrv_close(). triggers failures in iotests 122: --- /home/eblake/qemu/tests/qemu-iotests/122.out 2017-10-06 13:45:25.559279136 -0500 +++ /home/eblake/qemu/tests/qemu-iotests/122.out.bad 2018-02-20 15:54:29.890221575 -0600 @@ -117,8 +117,8 @@ convert -c -S 0: read 3145728/3145728 bytes at offset 0 3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 63963136/63963136 bytes at offset 3145728 -61 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qcow2: Marking image as corrupt: Compressed cluster at 0x5ffd2 crosses host cluster boundary; further corruption events will be suppressed +read failed: Input/output error [{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true}] so it looks like I'm reading qcow2_alloc_bytes() wrong and that we CAN have a compressed cluster that crosses host cluster boundaries? > So if I may suggest: > >    x+1 - 61:    Number of additional 512-byte sectors used for the >                 compressed data, beyond the sector containing the >                 offset in the previous field.  These sectors must fit >                 within the same host cluster. This sentence needs tweaking to match reality, given that my simple patch to flag cross-sector hosts triggered (or I need to figure out what was wrong with my patch). >  Note that the compressed >                 data does not necessarily occupy all of the bytes in >                 the final sector; rather, decompression stops when it >                 has produced a cluster of data.  Another compressed >                 cluster may map to the tail of the final sector used >                 by this compressed cluster. > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 8c4b26ceaf2..85b5dbd9c16 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1598,6 +1598,15 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset) sector_offset = coffset & 511; csize = nb_csectors * 512 - sector_offset; + /* We never write a compressed cluster that crosses host + * cluster boundaries; reject images that do that. */ + if (csize + (coffset % s->cluster_size) > s->cluster_size) { + qcow2_signal_corruption(bs, true, coffset, csize, + "Compressed cluster at %#" PRIx64 + " crosses host cluster boundary", coffset); + return -EIO; + } + /* Allocate buffers on first decompress operation, most images are