[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request
From: |
Eric Auger |
Subject: |
Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request |
Date: |
Wed, 22 Jun 2022 14:22:18 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 |
On 6/22/22 13:55, Jean-Philippe Brucker wrote:
> Hi,
>
> On Wed, Jun 22, 2022 at 12:20:45PM +0200, Eric Auger wrote:
>> Hi,
>>
>> On 6/17/22 08:20, Zhenzhong Duan wrote:
>>> The structure of probe request doesn't include the tail, this leads
>>> to a few field missed to be copied. Currently this isn't an issue as
>>> those missed field belong to reserved field, just in case reserved
>>> field will be used in the future.
>>>
>>> Fixes: 1733eebb9e75b ("virtio-iommu: Implement RESV_MEM probe request")
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> nice catch.
>>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>
>> the spec is pretty confusing here though (virtio-v1.2-csd01.pdf) as it
>> presents the struct as follows:
>>
>> struct virtio_iommu_req_probe {
>> struct virtio_iommu_req_head head;
>> /* Device-readable */
>> le32 endpoint;
>> u8 reserved[64];
>>
>> /* Device-writable */
>> u8 properties[probe_size];
>> struct virtio_iommu_req_tail tail;
>> };
> Hm, which part is confusing? Yes it's not valid C since probe_size is
> defined dynamically ('probe_size' in the device config), but I thought it
> would be nicer to show the whole request layout this way. Besides, at
> least virtio-blk and virtio-scsi have similar variable-sized arrays in
> their definitions
the fact "struct virtio_iommu_req_tail tail;" was part of the
virtio_iommu_req_probe struct
>
>> Adding Jean in the loop ...
>>
>> Thanks!
>>
>> Eric
>>
>>
>>
>>
>>> ---
>>> v2: keep bugfix change and drop cleanup change
>>>
>>> hw/virtio/virtio-iommu.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>> index 7c122ab95780..195f909620ab 100644
>>> --- a/hw/virtio/virtio-iommu.c
>>> +++ b/hw/virtio/virtio-iommu.c
>>> @@ -708,7 +708,8 @@ static int virtio_iommu_handle_probe(VirtIOIOMMU *s,
>>> uint8_t *buf)
>>> {
>>> struct virtio_iommu_req_probe req;
>>> - int ret = virtio_iommu_iov_to_req(iov, iov_cnt, &req, sizeof(req));
>>> + int ret = virtio_iommu_iov_to_req(iov, iov_cnt, &req,
>>> + sizeof(req) + sizeof(struct virtio_iommu_req_tail));
> Not sure this is correct, because what we are doing here is reading the
> device-readable part of the property from the request. That part is only
> composed of fields 'head', 'endpoint' and 'reserved[64]' and its size is
> indeed sizeof(struct virtio_iommu_req_probe).
>
> The 'properties' and 'tail' fields shouldn't be read by the device here,
> they are instead written later. It is virtio_iommu_handle_command() that
> copies both of them into the request:
>
> output_size = s->config.probe_size + sizeof(tail);
> buf = g_malloc0(output_size);
>
> ptail = (struct virtio_iommu_req_tail *)
> (buf + s->config.probe_size);
> ptail->status = virtio_iommu_handle_probe(s, iov, iov_cnt, buf);
> // and virtio_iommu_probe() fills 'properties' as needed
> ...
>
> // then copy the lot
> sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
> buf ? buf : &tail, output_size);
>
> So I think the current code is correct, as all fields are accounted for
In virtio_iommu_iov_to_req(), payload_sz is computed as
payload_sz = req_sz - sizeof(struct virtio_iommu_req_tail);
sz = iov_to_buf(iov, iov_cnt, 0, req, payload_sz);
This works for other command structs but not for probe one.
Eric
>
> Thanks,
> Jean
>
>>>
>>> return ret ? ret : virtio_iommu_probe(s, &req, buf);
>>> }
- [PATCH v2] virtio-iommu: Fix the partial copy of probe request, Zhenzhong Duan, 2022/06/17
- Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request, Eric Auger, 2022/06/22
- Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request, Jean-Philippe Brucker, 2022/06/22
- Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request,
Eric Auger <=
- Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request, Jean-Philippe Brucker, 2022/06/22
- RE: [PATCH v2] virtio-iommu: Fix the partial copy of probe request, Duan, Zhenzhong, 2022/06/22
- Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request, Jean-Philippe Brucker, 2022/06/23
- Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request, Eric Auger, 2022/06/23