| Message ID | 20240912051734.5298-1-jim.shu@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Fix null pointer deference from error API | expand |
Hello Jim, On 9/12/24 07:17, Jim Shu wrote: > pci_dev_realize() use the local error variable, which requires > `error_setg()` API to allocate the error object at first. > > Signed-off-by: Jim Shu <jim.shu@sifive.com> > --- > hw/vfio/pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 0a99e55247..d994ad8bb9 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -3117,7 +3117,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > > if (!vbasedev->mdev && > !pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) { 'errp' will be set by pci_device_set_iommu_device() in case of failure and, in this case, calling error_prepend() is a valid thing to do. I think we are fine. Thanks, C. > - error_prepend(errp, "Failed to set iommu_device: "); > + error_setg(errp, "Failed to set iommu_device: "); > goto out_teardown; > } >
Hi Cédric, Thank you very much for the quick response! I have checked the error API again. It seems to be my porting issue of set_iommu_device() callback. I think "pci_device_set_iommu_device(..., *errp)" should set 'errp' if this function returns false, right? Thanks, Jim On Thu, Sep 12, 2024 at 2:18 PM Cédric Le Goater <clg@redhat.com> wrote: > > Hello Jim, > > On 9/12/24 07:17, Jim Shu wrote: > > pci_dev_realize() use the local error variable, which requires > > `error_setg()` API to allocate the error object at first. > > > > Signed-off-by: Jim Shu <jim.shu@sifive.com> > > --- > > hw/vfio/pci.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > index 0a99e55247..d994ad8bb9 100644 > > --- a/hw/vfio/pci.c > > +++ b/hw/vfio/pci.c > > @@ -3117,7 +3117,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > > > > if (!vbasedev->mdev && > > !pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) { > > 'errp' will be set by pci_device_set_iommu_device() in case of > failure and, in this case, calling error_prepend() is a valid > thing to do. I think we are fine. > > Thanks, > > C. > > > > > - error_prepend(errp, "Failed to set iommu_device: "); > > + error_setg(errp, "Failed to set iommu_device: "); > > goto out_teardown; > > } > > >
Hello Jim, On 9/12/24 08:36, Jim Shu wrote: > Hi Cédric, > > Thank you very much for the quick response! > > I have checked the error API again. It seems to be my porting issue of > set_iommu_device() callback. Are you adding support for a new IOMMU ? > I think "pci_device_set_iommu_device(..., *errp)" should set 'errp' if > this function returns false, right? yes, this is a requirement for routines using an Error parameter. You can take a look at the "= Rules =" section in include/qapi/error.h for more info. Thanks, C.
On Thu, Sep 12, 2024 at 5:56 PM Cédric Le Goater <clg@redhat.com> wrote: > > Hello Jim, > > On 9/12/24 08:36, Jim Shu wrote: > > Hi Cédric, > > > > Thank you very much for the quick response! > > > > I have checked the error API again. It seems to be my porting issue of > > set_iommu_device() callback. > > Are you adding support for a new IOMMU ? Yes, I am working on RISC-V IOMMU support, based on Zhenzhong's iommufd nesting series [1] and RISC-V IOMMU patch v7. [1] https://github.com/yiliu1765/qemu/commits/zhenzhong/iommufd_nesting_rfcv2 > > > I think "pci_device_set_iommu_device(..., *errp)" should set 'errp' if > > this function returns false, right? > > yes, this is a requirement for routines using an Error parameter. > You can take a look at the "= Rules =" section in include/qapi/error.h > for more info. Thanks for the information! I will take a look. > > Thanks, > > C. >
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 0a99e55247..d994ad8bb9 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -3117,7 +3117,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) if (!vbasedev->mdev && !pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) { - error_prepend(errp, "Failed to set iommu_device: "); + error_setg(errp, "Failed to set iommu_device: "); goto out_teardown; }
pci_dev_realize() use the local error variable, which requires `error_setg()` API to allocate the error object at first. Signed-off-by: Jim Shu <jim.shu@sifive.com> --- hw/vfio/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)