mbox series

[0/2] vvfat: fix two crashes.

Message ID 20210524101257.119377-1-vsementsov@virtuozzo.com (mailing list archive)
Headers show
Series vvfat: fix two crashes. | expand

Message

Vladimir Sementsov-Ogievskiy May 24, 2021, 10:12 a.m. UTC
Hi!

As reported by Programmingkid, command 

qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none

crashes.

I tested it with qemu-system-x86_64 and it reproduces for me. I even
kept "<path of a host folder>" as is :).

So, here are two fixes.

Vladimir Sementsov-Ogievskiy (2):
  block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
  block/vvfat: fix vvfat_child_perm crash

 include/block/block.h | 1 +
 block.c               | 4 ++--
 block/vvfat.c         | 8 +++-----
 3 files changed, 6 insertions(+), 7 deletions(-)

Comments

Programmingkid May 24, 2021, 3:41 p.m. UTC | #1
> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> Hi!
> 
> As reported by Programmingkid, command 
> 
> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
> 
> crashes.
> 
> I tested it with qemu-system-x86_64 and it reproduces for me. I even
> kept "<path of a host folder>" as is :).
> 
> So, here are two fixes.
> 
> Vladimir Sementsov-Ogievskiy (2):
>  block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>  block/vvfat: fix vvfat_child_perm crash
> 
> include/block/block.h | 1 +
> block.c               | 4 ++--
> block/vvfat.c         | 8 +++-----
> 3 files changed, 6 insertions(+), 7 deletions(-)
> 
> -- 
> 2.29.2

I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.

Thank you.

Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
Vladimir Sementsov-Ogievskiy May 24, 2021, 3:55 p.m. UTC | #2
24.05.2021 18:41, Programmingkid wrote:
> 
> 
>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>> Hi!
>>
>> As reported by Programmingkid, command
>>
>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>
>> crashes.
>>
>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>> kept "<path of a host folder>" as is :).
>>
>> So, here are two fixes.
>>
>> Vladimir Sementsov-Ogievskiy (2):
>>   block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>   block/vvfat: fix vvfat_child_perm crash
>>
>> include/block/block.h | 1 +
>> block.c               | 4 ++--
>> block/vvfat.c         | 8 +++-----
>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>
>> -- 
>> 2.29.2
> 
> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
> 
> Thank you.
> 
> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
> 

Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.

Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.
Programmingkid May 24, 2021, 4:06 p.m. UTC | #3
> On May 24, 2021, at 11:55 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> 24.05.2021 18:41, Programmingkid wrote:
>>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>> 
>>> Hi!
>>> 
>>> As reported by Programmingkid, command
>>> 
>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>> 
>>> crashes.
>>> 
>>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>>> kept "<path of a host folder>" as is :).
>>> 
>>> So, here are two fixes.
>>> 
>>> Vladimir Sementsov-Ogievskiy (2):
>>>  block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>>  block/vvfat: fix vvfat_child_perm crash
>>> 
>>> include/block/block.h | 1 +
>>> block.c               | 4 ++--
>>> block/vvfat.c         | 8 +++-----
>>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>> 
>>> -- 
>>> 2.29.2
>> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
>> Thank you.
>> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
> 
> Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.

Yes, you are right. It should be: Tested-by: John Arbuckle <programmingkidx@gmail.com>

> 
> Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.

That should be fine but it isn't important.

On a related topic would you know if it is possible to use fat32 instead of fat16 for host folder sharing? I did try replacing the text fat16 with fat32 but it didn't appear to work.
Vladimir Sementsov-Ogievskiy May 24, 2021, 4:56 p.m. UTC | #4
24.05.2021 19:06, Programmingkid wrote:
> 
> 
>> On May 24, 2021, at 11:55 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>> 24.05.2021 18:41, Programmingkid wrote:
>>>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>
>>>> Hi!
>>>>
>>>> As reported by Programmingkid, command
>>>>
>>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>>>
>>>> crashes.
>>>>
>>>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>>>> kept "<path of a host folder>" as is :).
>>>>
>>>> So, here are two fixes.
>>>>
>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>   block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>>>   block/vvfat: fix vvfat_child_perm crash
>>>>
>>>> include/block/block.h | 1 +
>>>> block.c               | 4 ++--
>>>> block/vvfat.c         | 8 +++-----
>>>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>>>
>>>> -- 
>>>> 2.29.2
>>> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
>>> Thank you.
>>> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
>>
>> Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.
> 
> Yes, you are right. It should be: Tested-by: John Arbuckle <programmingkidx@gmail.com>
> 
>>
>> Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.
> 
> That should be fine but it isn't important.
> 
> On a related topic would you know if it is possible to use fat32 instead of fat16 for host folder sharing? I did try replacing the text fat16 with fat32 but it didn't appear to work.
> 

No, I don't know..

Moreover, my quick look at the code of vvfat, this fixed bug (which is obviously not covered by tests), and also status of block/vvfat in MAINTAINERS file "Odd Fixes", all this leads to advice "don't use it if possible". May be Kevin can add something about it, he is maintainer..

Could you use for example NFS or Samba, or sshfs to share folders? Or you need exactly to make a host folder available in guest vm as usb drive?
Programmingkid May 24, 2021, 5:33 p.m. UTC | #5
> On May 24, 2021, at 12:56 PM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> 24.05.2021 19:06, Programmingkid wrote:
>>> On May 24, 2021, at 11:55 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>> 
>>> 24.05.2021 18:41, Programmingkid wrote:
>>>>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>> 
>>>>> Hi!
>>>>> 
>>>>> As reported by Programmingkid, command
>>>>> 
>>>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>>>> 
>>>>> crashes.
>>>>> 
>>>>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>>>>> kept "<path of a host folder>" as is :).
>>>>> 
>>>>> So, here are two fixes.
>>>>> 
>>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>>  block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>>>>  block/vvfat: fix vvfat_child_perm crash
>>>>> 
>>>>> include/block/block.h | 1 +
>>>>> block.c               | 4 ++--
>>>>> block/vvfat.c         | 8 +++-----
>>>>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>>>> 
>>>>> -- 
>>>>> 2.29.2
>>>> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
>>>> Thank you.
>>>> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
>>> 
>>> Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.
>> Yes, you are right. It should be: Tested-by: John Arbuckle <programmingkidx@gmail.com>
>>> 
>>> Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.
>> That should be fine but it isn't important.
>> On a related topic would you know if it is possible to use fat32 instead of fat16 for host folder sharing? I did try replacing the text fat16 with fat32 but it didn't appear to work.
> 
> No, I don't know..
> 
> Moreover, my quick look at the code of vvfat, this fixed bug (which is obviously not covered by tests), and also status of block/vvfat in MAINTAINERS file "Odd Fixes", all this leads to advice "don't use it if possible". May be Kevin can add something about it, he is maintainer..
> 
> Could you use for example NFS or Samba, or sshfs to share folders? Or you need exactly to make a host folder available in guest vm as usb drive?

I do actually need it for some of my VM's. It makes sharing of folder between the host and guest so easy.
Vladimir Sementsov-Ogievskiy May 25, 2021, 6:05 a.m. UTC | #6
24.05.2021 20:33, Programmingkid wrote:
> 
> 
>> On May 24, 2021, at 12:56 PM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>> 24.05.2021 19:06, Programmingkid wrote:
>>>> On May 24, 2021, at 11:55 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>
>>>> 24.05.2021 18:41, Programmingkid wrote:
>>>>>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>>>
>>>>>> Hi!
>>>>>>
>>>>>> As reported by Programmingkid, command
>>>>>>
>>>>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>>>>>
>>>>>> crashes.
>>>>>>
>>>>>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>>>>>> kept "<path of a host folder>" as is :).
>>>>>>
>>>>>> So, here are two fixes.
>>>>>>
>>>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>>>   block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>>>>>   block/vvfat: fix vvfat_child_perm crash
>>>>>>
>>>>>> include/block/block.h | 1 +
>>>>>> block.c               | 4 ++--
>>>>>> block/vvfat.c         | 8 +++-----
>>>>>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> -- 
>>>>>> 2.29.2
>>>>> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
>>>>> Thank you.
>>>>> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
>>>>
>>>> Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.
>>> Yes, you are right. It should be: Tested-by: John Arbuckle <programmingkidx@gmail.com>
>>>>
>>>> Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.
>>> That should be fine but it isn't important.
>>> On a related topic would you know if it is possible to use fat32 instead of fat16 for host folder sharing? I did try replacing the text fat16 with fat32 but it didn't appear to work.
>>
>> No, I don't know..
>>
>> Moreover, my quick look at the code of vvfat, this fixed bug (which is obviously not covered by tests), and also status of block/vvfat in MAINTAINERS file "Odd Fixes", all this leads to advice "don't use it if possible". May be Kevin can add something about it, he is maintainer..
>>
>> Could you use for example NFS or Samba, or sshfs to share folders? Or you need exactly to make a host folder available in guest vm as usb drive?
> 
> I do actually need it for some of my VM's. It makes sharing of folder between the host and guest so easy.
> 

Probably virtio-fs is modern way for such thing:

https://virtio-fs.gitlab.io/
Programmingkid May 25, 2021, 12:10 p.m. UTC | #7
> On May 25, 2021, at 2:05 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> 24.05.2021 20:33, Programmingkid wrote:
>>> On May 24, 2021, at 12:56 PM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>> 
>>> 24.05.2021 19:06, Programmingkid wrote:
>>>>> On May 24, 2021, at 11:55 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>> 
>>>>> 24.05.2021 18:41, Programmingkid wrote:
>>>>>>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>>>> 
>>>>>>> Hi!
>>>>>>> 
>>>>>>> As reported by Programmingkid, command
>>>>>>> 
>>>>>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>>>>>> 
>>>>>>> crashes.
>>>>>>> 
>>>>>>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>>>>>>> kept "<path of a host folder>" as is :).
>>>>>>> 
>>>>>>> So, here are two fixes.
>>>>>>> 
>>>>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>>>>  block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>>>>>>  block/vvfat: fix vvfat_child_perm crash
>>>>>>> 
>>>>>>> include/block/block.h | 1 +
>>>>>>> block.c               | 4 ++--
>>>>>>> block/vvfat.c         | 8 +++-----
>>>>>>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>>>>>> 
>>>>>>> -- 
>>>>>>> 2.29.2
>>>>>> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
>>>>>> Thank you.
>>>>>> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
>>>>> 
>>>>> Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.
>>>> Yes, you are right. It should be: Tested-by: John Arbuckle <programmingkidx@gmail.com>
>>>>> 
>>>>> Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.
>>>> That should be fine but it isn't important.
>>>> On a related topic would you know if it is possible to use fat32 instead of fat16 for host folder sharing? I did try replacing the text fat16 with fat32 but it didn't appear to work.
>>> 
>>> No, I don't know..
>>> 
>>> Moreover, my quick look at the code of vvfat, this fixed bug (which is obviously not covered by tests), and also status of block/vvfat in MAINTAINERS file "Odd Fixes", all this leads to advice "don't use it if possible". May be Kevin can add something about it, he is maintainer..
>>> 
>>> Could you use for example NFS or Samba, or sshfs to share folders? Or you need exactly to make a host folder available in guest vm as usb drive?
>> I do actually need it for some of my VM's. It makes sharing of folder between the host and guest so easy.
> 
> Probably virtio-fs is modern way for such thing:
> 
> https://virtio-fs.gitlab.io/

Thank you for this.
Kevin Wolf May 25, 2021, 4:18 p.m. UTC | #8
Am 24.05.2021 um 18:06 hat Programmingkid geschrieben:
> >>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
> >>> 
> On a related topic would you know if it is possible to use fat32
> instead of fat16 for host folder sharing? I did try replacing the text
> fat16 with fat32 but it didn't appear to work.

I think the correct syntax is fat:32:rw:<path>. But one of the first
things it does is:

    warn_report("FAT32 has not been tested. You are welcome to do so!");

So probably nobody would be surprised if it broke.

Kevin
Programmingkid May 26, 2021, 12:04 p.m. UTC | #9
> On May 25, 2021, at 12:18 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> 
> Am 24.05.2021 um 18:06 hat Programmingkid geschrieben:
>>>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>>>> 
>> On a related topic would you know if it is possible to use fat32
>> instead of fat16 for host folder sharing? I did try replacing the text
>> fat16 with fat32 but it didn't appear to work.
> 
> I think the correct syntax is fat:32:rw:<path>. But one of the first
> things it does is:
> 
>    warn_report("FAT32 has not been tested. You are welcome to do so!");
> 
> So probably nobody would be surprised if it broke.
> 
> Kevin
> 

Thank you very much for this information.
Kevin Wolf May 27, 2021, 10:22 a.m. UTC | #10
Am 24.05.2021 um 12:12 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi!
> 
> As reported by Programmingkid, command 
> 
> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
> 
> crashes.
> 
> I tested it with qemu-system-x86_64 and it reproduces for me. I even
> kept "<path of a host folder>" as is :).
> 
> So, here are two fixes.

Thanks, applied to the block branch.

Kevin