Re: [Qemu-devel] [PATCH for-2.8 v1 1/1] block/vmdk: Fix the endian problem of buf_len

[Kevin Wolf <kwolf@redhat.com>]

Am 25.11.2016 um 11:48 hat Hao QingFeng geschrieben:
> 在 2016-11-25 18:21, Kevin Wolf 写道:
> >[ Cc: Fam, qemu-stable ]
> >
> >Am 25.11.2016 um 11:06 hat QingFeng Hao geschrieben:
> >>The problem was triggered by qemu-iotests case 055. It failed when it
> >>was comparing the compressed vmdk image with original test.img.
> >>
> >>The cause is that buf_len in vmdk_write_extent wasn't converted to
> >>little-endian before it was stored to disk. But later vmdk_read_extent
> >>read it and converted it from little-endian to cpu endian.
> >>If the cpu is big-endian like s390, the problem will happen and
> >>the data length read by vmdk_read_extent will become invalid!
> >>The fix is to add the conversion in vmdk_write_extent.
> >>
> >>Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
> >>Signed-off-by: Jing Liu <liujbjl@linux.vnet.ibm.com>
> >Sounds like something that should still be fixed for 2.8 and in the
> >stable branches.
> I didn't find the stable branch on upstream, the latest is 2.6 maybe
> it's a private one? :-)

I think maybe it's only created once work for the 2.7.1 release begins.

Anyway, this is not your problem, just make sure to CC the qemu-stable
mailing list for bug fixes that affect previous versions as well. It's
also helpful if you put a line like this immediately before the
signoffs:

Cc: qemu-stable@nongnu.org

This makes sure that the stable release maintainers see the patch,
and everything else will be taken care of by them.

> >>diff --git a/block/vmdk.c b/block/vmdk.c
> >>index a11c27a..bf6667f 100644
> >>--- a/block/vmdk.c
> >>+++ b/block/vmdk.c
> >>@@ -1355,7 +1355,7 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
> >>          }
> >>          data->lba = offset >> BDRV_SECTOR_BITS;
> >>-        data->size = buf_len;
> >>+        data->size = cpu_to_le32(buf_len);
> >At least data->lba needs to be fixed, too, both here and in
> >vmdk_read_extent(). Host endianness in an image file is always wrong.
> Good detection!

Are you going to send a v2 that adds this fix?

> >Maybe we should audit the whole driver for endianness problems.
> Good sight! maybe we can encapsulate a suite of endianness functions
> for the structures to avoid missing some elements of them like lba?
> Also will be better for reuse. When I am checking the endianness calls
> in vmdk_create_extent, I am just afraid of missing one. :-)

Sounds like a good move in general, but let's make sure to keep it
separate from the fixes. The reason is that the fixes can still be
included in the 2.8 release, but code refactoring would only be for 2.9.

You can work on both at the same time, of course, but organise things so
that we have a patch series where all the bug fixes come first (for 2.8)
and only then the type-safe refactoring (for 2.9).

Kevin