| Message ID | 20220617062024.3168331-1-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] virtio-iommu: Fix the partial copy of probe request | expand |
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; }; 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)); > > return ret ? ret : virtio_iommu_probe(s, &req, buf); > }
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 > > 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 Thanks, Jean > > > > return ret ? ret : virtio_iommu_probe(s, &req, buf); > > } >
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); >>> }
On Wed, Jun 22, 2022 at 02:22:18PM +0200, Eric Auger wrote: > >> 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 Right, it would have been better to use a different name than virtio_iommu_req_probe in virtio_iommu.h, to make the pitfall clear. The larger problem is using C structs across the virtio spec instead of an abstract format. Someone implementing the device in another language would already not encounter this problem since they would read the spec as an abstract format. For documentation purposes I do prefer displaying the whole struct like this rather than working around limitations of C, which may be more confusing. > >>> 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. Aah right sorry. The resulting code may be less confusing if we moved "- sizeof(struct virtio_iommu_req_tail)" to virtio_iommu_handle_req() Thanks, Jean
>-----Original Message----- >From: Jean-Philippe Brucker <jean-philippe@linaro.org> >Sent: Wednesday, June 22, 2022 9:58 PM >To: Eric Auger <eric.auger@redhat.com> >Cc: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu- >devel@nongnu.org; mst@redhat.com >Subject: Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request > >On Wed, Jun 22, 2022 at 02:22:18PM +0200, Eric Auger wrote: >> >> 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 > >Right, it would have been better to use a different name than >virtio_iommu_req_probe in virtio_iommu.h, to make the pitfall clear. > Maybe virtio_iommu_req_probe_no_tail? >The larger problem is using C structs across the virtio spec instead of an >abstract format. Someone implementing the device in another language >would already not encounter this problem since they would read the spec as >an abstract format. For documentation purposes I do prefer displaying the >whole struct like this rather than working around limitations of C, which may >be more confusing. > > >> >>> 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. > >Aah right sorry. The resulting code may be less confusing if we moved >"- sizeof(struct virtio_iommu_req_tail)" to virtio_iommu_handle_req() > Looks better, thanks. Will send v3. Zhenzhong
On Thu, Jun 23, 2022 at 01:40:58AM +0000, Duan, Zhenzhong wrote: > > > >-----Original Message----- > >From: Jean-Philippe Brucker <jean-philippe@linaro.org> > >Sent: Wednesday, June 22, 2022 9:58 PM > >To: Eric Auger <eric.auger@redhat.com> > >Cc: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu- > >devel@nongnu.org; mst@redhat.com > >Subject: Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request > > > >On Wed, Jun 22, 2022 at 02:22:18PM +0200, Eric Auger wrote: > >> >> 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 > > > >Right, it would have been better to use a different name than > >virtio_iommu_req_probe in virtio_iommu.h, to make the pitfall clear. > > > Maybe virtio_iommu_req_probe_no_tail? Yes, we can't change the probe struct anymore since it's API, but we could use the no_tail prefix on future structs Thanks, Jean
On 6/23/22 10:41, Jean-Philippe Brucker wrote: > On Thu, Jun 23, 2022 at 01:40:58AM +0000, Duan, Zhenzhong wrote: >> >>> -----Original Message----- >>> From: Jean-Philippe Brucker <jean-philippe@linaro.org> >>> Sent: Wednesday, June 22, 2022 9:58 PM >>> To: Eric Auger <eric.auger@redhat.com> >>> Cc: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu- >>> devel@nongnu.org; mst@redhat.com >>> Subject: Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request >>> >>> On Wed, Jun 22, 2022 at 02:22:18PM +0200, Eric Auger wrote: >>>>>> 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 >>> Right, it would have been better to use a different name than >>> virtio_iommu_req_probe in virtio_iommu.h, to make the pitfall clear. >>> >> Maybe virtio_iommu_req_probe_no_tail? > Yes, we can't change the probe struct anymore since it's API, but we could > use the no_tail prefix on future structs agreed Eric > > Thanks, > Jean >
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)); return ret ? ret : virtio_iommu_probe(s, &req, buf); }
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> --- v2: keep bugfix change and drop cleanup change hw/virtio/virtio-iommu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)