[Qemu-devel] [PATCH v1 0/1] qemu: fix the bug reported by qemu-iotests case 055

[Qemu-devel] [PATCH v1 0/1] qemu: fix the bug reported by qemu-iotests case 055

[QingFeng Hao]

Hi all,
This patch is to fix the bug reported by qemu-iotests case 055
and based on upstream master's commit:
00227fefd205: Update version for v2.8.0-rc1 release
Jing Liu and I found the cause was in vmdk and the fix is in the followed patch.
Thanks!

Upstream master's qemu-iotests case 055 reports the following error:
+qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument
+qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument
+qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument
+qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument 

QingFeng Hao (1):
  block/vmdk: Fix the endian problem of buf_len

 block/vmdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.8.4

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

[QingFeng Hao]

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>
---
 block/vmdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
 
         n_bytes = buf_len + sizeof(VmdkGrainMarker);
         iov = (struct iovec) {
-- 
2.8.4

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

[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.

> 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.

Maybe we should audit the whole driver for endianness problems.

Kevin

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

[Hao QingFeng]

在 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? :-)
>
>> 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!
>
> 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. :-)
Thanks!
>
> Kevin
>

-- 
QingFeng Hao(Robin)

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

[Kevin Wolf]

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

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

[Hao QingFeng]

在 2016-11-25 20:05, Kevin Wolf 写道:
> 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.
Thanks and I'll add it in the next patch.
>
>>>> 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?
Yes, I think so.  :-)
>
>>> 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).
Thanks for your advice! I'll ensure the bug fixes come first and prepare 
other stuff meanwhile.
> Kevin
>

-- 
QingFeng Hao(Robin)