| Message ID | 20210821150535.763541-1-ani@anisinha.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/acpi/pcihp: validate bsel property of the bus before unplugging device | expand |
On Sat, Aug 21, 2021 at 08:35:35PM +0530, Ani Sinha wrote: > Bsel property of the pci bus indicates whether the bus supports acpi hotplug. > We need to validate the presence of this property before performing any hotplug > related callback operations. Currently validation of the existence of this > property was absent from acpi_pcihp_device_unplug_cb() function but is present > in other hotplug/unplug callback functions. Hence, this change adds the missing > check for the above function. > > Signed-off-by: Ani Sinha <ani@anisinha.ca> I queued this but I have a general question: are all these errors logged with LOG_GUEST_ERROR? Because if not we have a security problem. I also note that bsel is an internal property, I am not sure we should be printing this to users, it might just confuse them. Same question for all the other places validating bsel. > --- > hw/acpi/pcihp.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index 0fd0c1d811..9982815a87 100644 > --- a/hw/acpi/pcihp.c > +++ b/hw/acpi/pcihp.c > @@ -372,9 +372,15 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > DeviceState *dev, Error **errp) > { > PCIDevice *pdev = PCI_DEVICE(dev); > + int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev)); > + > + trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), bsel); > > - trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), > - acpi_pcihp_get_bsel(pci_get_bus(pdev))); > + if (bsel < 0) { > + error_setg(errp, "Unsupported bus. Bus doesn't have property '" > + ACPI_PCIHP_PROP_BSEL "' set"); > + return; > + } > > /* > * clean up acpi-index so it could reused by another device > -- > 2.25.1
On Mon, 23 Aug 2021, Michael S. Tsirkin wrote: > On Sat, Aug 21, 2021 at 08:35:35PM +0530, Ani Sinha wrote: > > Bsel property of the pci bus indicates whether the bus supports acpi hotplug. > > We need to validate the presence of this property before performing any hotplug > > related callback operations. Currently validation of the existence of this > > property was absent from acpi_pcihp_device_unplug_cb() function but is present > > in other hotplug/unplug callback functions. Hence, this change adds the missing > > check for the above function. > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca> > > I queued this but I have a general question: > are all these errors logged with LOG_GUEST_ERROR? I do not think they are logged that way. These logs go to stderr which can end up in the qemu guest specific log file when qemu is run daemonized. That being said, other platforms, for example virtio-pci also seems to do what we do. They use error_setg() as well under similar conditions. > Because if not we have a security problem. > I also note that bsel is an internal property, yeah this I think is an issue. We can change the log so as to not say anything about bsel. I will let Igor comment. I can send out a separate patch to fix this. > I am not sure we should be printing this to users, > it might just confuse them. > > Same question for all the other places validating bsel. > > > --- > > hw/acpi/pcihp.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > index 0fd0c1d811..9982815a87 100644 > > --- a/hw/acpi/pcihp.c > > +++ b/hw/acpi/pcihp.c > > @@ -372,9 +372,15 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > > DeviceState *dev, Error **errp) > > { > > PCIDevice *pdev = PCI_DEVICE(dev); > > + int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev)); > > + > > + trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), bsel); > > > > - trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), > > - acpi_pcihp_get_bsel(pci_get_bus(pdev))); > > + if (bsel < 0) { > > + error_setg(errp, "Unsupported bus. Bus doesn't have property '" > > + ACPI_PCIHP_PROP_BSEL "' set"); > > + return; > > + } > > > > /* > > * clean up acpi-index so it could reused by another device > > -- > > 2.25.1 > >
On Mon, 23 Aug 2021 19:06:47 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Sat, Aug 21, 2021 at 08:35:35PM +0530, Ani Sinha wrote: > > Bsel property of the pci bus indicates whether the bus supports acpi hotplug. > > We need to validate the presence of this property before performing any hotplug > > related callback operations. Currently validation of the existence of this > > property was absent from acpi_pcihp_device_unplug_cb() function but is present > > in other hotplug/unplug callback functions. Hence, this change adds the missing > > check for the above function. > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca> > > I queued this but I have a general question: I convinced myself that this patch is wrong, pls drop it. > are all these errors logged with LOG_GUEST_ERROR? > Because if not we have a security problem. > I also note that bsel is an internal property, > I am not sure we should be printing this to users, > it might just confuse them. > > Same question for all the other places validating bsel. Commit message misses reproducer/explanation about how it could be triggered? If it's actually reachable, from my point of view putting checks all through out call chain is not robust and it's easy to miss issues caused by invalid bsel. Instead of putting check all over the code, I'd check value on entry points (pci_read/pci_write) if code there is broken. > > > --- > > hw/acpi/pcihp.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > index 0fd0c1d811..9982815a87 100644 > > --- a/hw/acpi/pcihp.c > > +++ b/hw/acpi/pcihp.c > > @@ -372,9 +372,15 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > > DeviceState *dev, Error **errp) > > { > > PCIDevice *pdev = PCI_DEVICE(dev); > > + int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev)); > > + > > + trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), bsel); > > > > - trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), > > - acpi_pcihp_get_bsel(pci_get_bus(pdev))); > > + if (bsel < 0) { > > + error_setg(errp, "Unsupported bus. Bus doesn't have property '" > > + ACPI_PCIHP_PROP_BSEL "' set"); > > + return; > > + } 1st: Error here is useless. this path is triggered on guest MMIO write and there is no consumer for error whatsoever. If I recall correctly, in such cases we in PCIHP code we make such access a silent NOP. And tracing is there for a us to help figure out what's going on. 2nd: if it got this far, it means a device on a bus with bsel was found and we are completing cleanup. Error-ing out at this point will leak acpi_index. > > > > /* > > * clean up acpi-index so it could reused by another device > > -- > > 2.25.1 >
On Tue, 24 Aug 2021, Igor Mammedov wrote: > On Mon, 23 Aug 2021 19:06:47 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Sat, Aug 21, 2021 at 08:35:35PM +0530, Ani Sinha wrote: > > > Bsel property of the pci bus indicates whether the bus supports acpi hotplug. > > > We need to validate the presence of this property before performing any hotplug > > > related callback operations. Currently validation of the existence of this > > > property was absent from acpi_pcihp_device_unplug_cb() function but is present > > > in other hotplug/unplug callback functions. Hence, this change adds the missing > > > check for the above function. > > > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca> > > > > I queued this but I have a general question: > I convinced myself that this patch is wrong, pls drop it. OK so now we have a situation where this function callback does not have this check whereas others does. Should we drop them from everywhere?
On Tue, 24 Aug 2021, Igor Mammedov wrote: > On Mon, 23 Aug 2021 19:06:47 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Sat, Aug 21, 2021 at 08:35:35PM +0530, Ani Sinha wrote: > > > Bsel property of the pci bus indicates whether the bus supports acpi hotplug. > > > We need to validate the presence of this property before performing any hotplug > > > related callback operations. Currently validation of the existence of this > > > property was absent from acpi_pcihp_device_unplug_cb() function but is present > > > in other hotplug/unplug callback functions. Hence, this change adds the missing > > > check for the above function. > > > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca> > > > > I queued this but I have a general question: > I convinced myself that this patch is wrong, pls drop it. > > > are all these errors logged with LOG_GUEST_ERROR? > > Because if not we have a security problem. > > I also note that bsel is an internal property, > > I am not sure we should be printing this to users, > > it might just confuse them. > > > > Same question for all the other places validating bsel. > > Commit message misses reproducer/explanation about > how it could be triggered? > > If it's actually reachable, from my point of view > putting checks all through out call chain is not robust > and it's easy to miss issues caused by invalid bsel. > Instead of putting check all over the code, I'd > check value on entry points (pci_read/pci_write) > if code there is broken. > > > > > > --- > > > hw/acpi/pcihp.c | 10 ++++++++-- > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > > index 0fd0c1d811..9982815a87 100644 > > > --- a/hw/acpi/pcihp.c > > > +++ b/hw/acpi/pcihp.c > > > @@ -372,9 +372,15 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > > > DeviceState *dev, Error **errp) > > > { > > > PCIDevice *pdev = PCI_DEVICE(dev); > > > + int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev)); > > > + > > > + trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), bsel); > > > > > > - trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), > > > - acpi_pcihp_get_bsel(pci_get_bus(pdev))); > > > + if (bsel < 0) { > > > + error_setg(errp, "Unsupported bus. Bus doesn't have property '" > > > + ACPI_PCIHP_PROP_BSEL "' set"); > > > + return; > > > + } > > 1st: > Error here is useless. this path is triggered on guest > MMIO write and there is no consumer for error whatsoever. > If I recall correctly, in such cases we in PCIHP code we make > such access a silent NOP. And tracing is there for a us > to help figure out what's going on. > > 2nd: > if it got this far, it means a device on a bus with bsel > was found and we are completing cleanup. Error-ing out at > this point will leak acpi_index. The above two points seems to apply in this case as well and so should we do this? diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c index 0fd0c1d811..c7692f5d5f 100644 --- a/hw/acpi/pcihp.c +++ b/hw/acpi/pcihp.c @@ -400,12 +400,6 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev, trace_acpi_pci_unplug_request(bsel, slot); - if (bsel < 0) { - error_setg(errp, "Unsupported bus. Bus doesn't have property '" - ACPI_PCIHP_PROP_BSEL "' set"); - return; - } - s->acpi_pcihp_pci_status[bsel].down |= (1U << slot); acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); } I wanted to check before sending out a formal patch. I like symmetric code.
On Tue, 24 Aug 2021, Ani Sinha wrote: > > > On Tue, 24 Aug 2021, Igor Mammedov wrote: > > > On Mon, 23 Aug 2021 19:06:47 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Sat, Aug 21, 2021 at 08:35:35PM +0530, Ani Sinha wrote: > > > > Bsel property of the pci bus indicates whether the bus supports acpi hotplug. > > > > We need to validate the presence of this property before performing any hotplug > > > > related callback operations. Currently validation of the existence of this > > > > property was absent from acpi_pcihp_device_unplug_cb() function but is present > > > > in other hotplug/unplug callback functions. Hence, this change adds the missing > > > > check for the above function. > > > > > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca> > > > > > > I queued this but I have a general question: > > I convinced myself that this patch is wrong, pls drop it. > > > > > are all these errors logged with LOG_GUEST_ERROR? > > > Because if not we have a security problem. > > > I also note that bsel is an internal property, > > > I am not sure we should be printing this to users, > > > it might just confuse them. > > > > > > Same question for all the other places validating bsel. > > > > Commit message misses reproducer/explanation about > > how it could be triggered? > > > > If it's actually reachable, from my point of view > > putting checks all through out call chain is not robust > > and it's easy to miss issues caused by invalid bsel. > > Instead of putting check all over the code, I'd > > check value on entry points (pci_read/pci_write) > > if code there is broken. > > > > > > > > > --- > > > > hw/acpi/pcihp.c | 10 ++++++++-- > > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > > > index 0fd0c1d811..9982815a87 100644 > > > > --- a/hw/acpi/pcihp.c > > > > +++ b/hw/acpi/pcihp.c > > > > @@ -372,9 +372,15 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > > > > DeviceState *dev, Error **errp) > > > > { > > > > PCIDevice *pdev = PCI_DEVICE(dev); > > > > + int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev)); > > > > + > > > > + trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), bsel); > > > > > > > > - trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), > > > > - acpi_pcihp_get_bsel(pci_get_bus(pdev))); > > > > + if (bsel < 0) { > > > > + error_setg(errp, "Unsupported bus. Bus doesn't have property '" > > > > + ACPI_PCIHP_PROP_BSEL "' set"); > > > > + return; > > > > + } > > > > 1st: > > Error here is useless. this path is triggered on guest > > MMIO write and there is no consumer for error whatsoever. > > If I recall correctly, in such cases we in PCIHP code we make > > such access a silent NOP. And tracing is there for a us > > to help figure out what's going on. > > > > 2nd: > > if it got this far, it means a device on a bus with bsel > > was found and we are completing cleanup. Error-ing out at > > this point will leak acpi_index. > > The above two points seems to apply in this case as well and so should we > do this? > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index 0fd0c1d811..c7692f5d5f 100644 > --- a/hw/acpi/pcihp.c > +++ b/hw/acpi/pcihp.c > @@ -400,12 +400,6 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev, > > trace_acpi_pci_unplug_request(bsel, slot); > > - if (bsel < 0) { > - error_setg(errp, "Unsupported bus. Bus doesn't have property '" > - ACPI_PCIHP_PROP_BSEL "' set"); > - return; > - } > - > s->acpi_pcihp_pci_status[bsel].down |= (1U << slot); > acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > } or add g_assert() on both instead.
On Tue, 24 Aug 2021 16:07:30 +0530 (IST) Ani Sinha <ani@anisinha.ca> wrote: > On Tue, 24 Aug 2021, Igor Mammedov wrote: > > > On Mon, 23 Aug 2021 19:06:47 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Sat, Aug 21, 2021 at 08:35:35PM +0530, Ani Sinha wrote: > > > > Bsel property of the pci bus indicates whether the bus supports acpi hotplug. > > > > We need to validate the presence of this property before performing any hotplug > > > > related callback operations. Currently validation of the existence of this > > > > property was absent from acpi_pcihp_device_unplug_cb() function but is present > > > > in other hotplug/unplug callback functions. Hence, this change adds the missing > > > > check for the above function. > > > > > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca> > > > > > > I queued this but I have a general question: > > I convinced myself that this patch is wrong, pls drop it. > > > > > are all these errors logged with LOG_GUEST_ERROR? > > > Because if not we have a security problem. > > > I also note that bsel is an internal property, > > > I am not sure we should be printing this to users, > > > it might just confuse them. > > > > > > Same question for all the other places validating bsel. > > > > Commit message misses reproducer/explanation about > > how it could be triggered? > > > > If it's actually reachable, from my point of view > > putting checks all through out call chain is not robust > > and it's easy to miss issues caused by invalid bsel. > > Instead of putting check all over the code, I'd > > check value on entry points (pci_read/pci_write) > > if code there is broken. > > > > > > > > > --- > > > > hw/acpi/pcihp.c | 10 ++++++++-- > > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > > > index 0fd0c1d811..9982815a87 100644 > > > > --- a/hw/acpi/pcihp.c > > > > +++ b/hw/acpi/pcihp.c > > > > @@ -372,9 +372,15 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > > > > DeviceState *dev, Error **errp) > > > > { > > > > PCIDevice *pdev = PCI_DEVICE(dev); > > > > + int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev)); > > > > + > > > > + trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), bsel); > > > > > > > > - trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), > > > > - acpi_pcihp_get_bsel(pci_get_bus(pdev))); > > > > + if (bsel < 0) { > > > > + error_setg(errp, "Unsupported bus. Bus doesn't have property '" > > > > + ACPI_PCIHP_PROP_BSEL "' set"); > > > > + return; > > > > + } > > > > 1st: > > Error here is useless. this path is triggered on guest > > MMIO write and there is no consumer for error whatsoever. > > If I recall correctly, in such cases we in PCIHP code we make > > such access a silent NOP. And tracing is there for a us > > to help figure out what's going on. > > > > 2nd: > > if it got this far, it means a device on a bus with bsel > > was found and we are completing cleanup. Error-ing out at > > this point will leak acpi_index. > > The above two points seems to apply in this case as well and so should we > do this? Please see where acpi_pcihp_device_unplug_request_cb() is called from, that should answer your question. > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index 0fd0c1d811..c7692f5d5f 100644 > --- a/hw/acpi/pcihp.c > +++ b/hw/acpi/pcihp.c > @@ -400,12 +400,6 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev, > > trace_acpi_pci_unplug_request(bsel, slot); > > - if (bsel < 0) { > - error_setg(errp, "Unsupported bus. Bus doesn't have property '" > - ACPI_PCIHP_PROP_BSEL "' set"); > - return; > - } > - > s->acpi_pcihp_pci_status[bsel].down |= (1U << slot); > acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > } > > > I wanted to check before sending out a formal patch. I like symmetric > code.
On 8/24/21 1:06 PM, Ani Sinha wrote: > On Tue, 24 Aug 2021, Ani Sinha wrote: >> On Tue, 24 Aug 2021, Igor Mammedov wrote: >>> On Mon, 23 Aug 2021 19:06:47 -0400 >>> "Michael S. Tsirkin" <mst@redhat.com> wrote: >>> >>>> On Sat, Aug 21, 2021 at 08:35:35PM +0530, Ani Sinha wrote: >>>>> Bsel property of the pci bus indicates whether the bus supports acpi hotplug. >>>>> We need to validate the presence of this property before performing any hotplug >>>>> related callback operations. Currently validation of the existence of this >>>>> property was absent from acpi_pcihp_device_unplug_cb() function but is present >>>>> in other hotplug/unplug callback functions. Hence, this change adds the missing >>>>> check for the above function. >>>>> >>>>> Signed-off-by: Ani Sinha <ani@anisinha.ca> >>>> >>>> I queued this but I have a general question: >>> I convinced myself that this patch is wrong, pls drop it. >>> >>>> are all these errors logged with LOG_GUEST_ERROR? >>>> Because if not we have a security problem. >>>> I also note that bsel is an internal property, >>>> I am not sure we should be printing this to users, >>>> it might just confuse them. >>>> >>>> Same question for all the other places validating bsel. >>> >>> Commit message misses reproducer/explanation about >>> how it could be triggered? >>> >>> If it's actually reachable, from my point of view >>> putting checks all through out call chain is not robust >>> and it's easy to miss issues caused by invalid bsel. >>> Instead of putting check all over the code, I'd >>> check value on entry points (pci_read/pci_write) >>> if code there is broken. >>> >>>> >>>>> --- >>>>> hw/acpi/pcihp.c | 10 ++++++++-- >>>>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c >>>>> index 0fd0c1d811..9982815a87 100644 >>>>> --- a/hw/acpi/pcihp.c >>>>> +++ b/hw/acpi/pcihp.c >>>>> @@ -372,9 +372,15 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, >>>>> DeviceState *dev, Error **errp) >>>>> { >>>>> PCIDevice *pdev = PCI_DEVICE(dev); >>>>> + int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev)); >>>>> + >>>>> + trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), bsel); >>>>> >>>>> - trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), >>>>> - acpi_pcihp_get_bsel(pci_get_bus(pdev))); >>>>> + if (bsel < 0) { >>>>> + error_setg(errp, "Unsupported bus. Bus doesn't have property '" >>>>> + ACPI_PCIHP_PROP_BSEL "' set"); >>>>> + return; >>>>> + } >>> >>> 1st: >>> Error here is useless. this path is triggered on guest >>> MMIO write and there is no consumer for error whatsoever. >>> If I recall correctly, in such cases we in PCIHP code we make >>> such access a silent NOP. And tracing is there for a us >>> to help figure out what's going on. >>> >>> 2nd: >>> if it got this far, it means a device on a bus with bsel >>> was found and we are completing cleanup. Error-ing out at >>> this point will leak acpi_index. >> >> The above two points seems to apply in this case as well and so should we >> do this? >> >> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c >> index 0fd0c1d811..c7692f5d5f 100644 >> --- a/hw/acpi/pcihp.c >> +++ b/hw/acpi/pcihp.c >> @@ -400,12 +400,6 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev, >> >> trace_acpi_pci_unplug_request(bsel, slot); >> >> - if (bsel < 0) { >> - error_setg(errp, "Unsupported bus. Bus doesn't have property '" >> - ACPI_PCIHP_PROP_BSEL "' set"); >> - return; >> - } >> - >> s->acpi_pcihp_pci_status[bsel].down |= (1U << slot); >> acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); >> } > > or add g_assert() on both instead. 'git-blame' & read git history ('git-log -p hw/acpi/pcihp.c') often helps to understand how the code evolved and why it is not "symmetric". For example see: commit ec266f408882fd38475f72c4e864ed576228643b Author: David Hildenbrand <david@redhat.com> Date: Wed Dec 12 10:16:17 2018 +0100 pci/pcihp: perform check for bus capability in pre_plug handler Perform the check in the pre_plug handler. In addition, we need the capability only if the device is actually hotplugged (and not created during machine initialization). This is a preparation for coldplugging pci devices via that hotplug handler. From here try to figure out what happened, why this changed was necessary, why there is no equivalent g_assert() as you noticed. Then try to convince the reviewers why in your commit description :) See: https://www.freecodecamp.org/news/writing-good-commit-messages-a-practical-guide/#how-to-write-good-commit-messages
On Tue, 24 Aug 2021, Philippe Mathieu-Daudé wrote: > On 8/24/21 1:06 PM, Ani Sinha wrote: > > On Tue, 24 Aug 2021, Ani Sinha wrote: > >> On Tue, 24 Aug 2021, Igor Mammedov wrote: > >>> On Mon, 23 Aug 2021 19:06:47 -0400 > >>> "Michael S. Tsirkin" <mst@redhat.com> wrote: > >>> > >>>> On Sat, Aug 21, 2021 at 08:35:35PM +0530, Ani Sinha wrote: > >>>>> Bsel property of the pci bus indicates whether the bus supports acpi hotplug. > >>>>> We need to validate the presence of this property before performing any hotplug > >>>>> related callback operations. Currently validation of the existence of this > >>>>> property was absent from acpi_pcihp_device_unplug_cb() function but is present > >>>>> in other hotplug/unplug callback functions. Hence, this change adds the missing > >>>>> check for the above function. > >>>>> > >>>>> Signed-off-by: Ani Sinha <ani@anisinha.ca> > >>>> > >>>> I queued this but I have a general question: > >>> I convinced myself that this patch is wrong, pls drop it. > >>> > >>>> are all these errors logged with LOG_GUEST_ERROR? > >>>> Because if not we have a security problem. > >>>> I also note that bsel is an internal property, > >>>> I am not sure we should be printing this to users, > >>>> it might just confuse them. > >>>> > >>>> Same question for all the other places validating bsel. > >>> > >>> Commit message misses reproducer/explanation about > >>> how it could be triggered? > >>> > >>> If it's actually reachable, from my point of view > >>> putting checks all through out call chain is not robust > >>> and it's easy to miss issues caused by invalid bsel. > >>> Instead of putting check all over the code, I'd > >>> check value on entry points (pci_read/pci_write) > >>> if code there is broken. > >>> > >>>> > >>>>> --- > >>>>> hw/acpi/pcihp.c | 10 ++++++++-- > >>>>> 1 file changed, 8 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > >>>>> index 0fd0c1d811..9982815a87 100644 > >>>>> --- a/hw/acpi/pcihp.c > >>>>> +++ b/hw/acpi/pcihp.c > >>>>> @@ -372,9 +372,15 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > >>>>> DeviceState *dev, Error **errp) > >>>>> { > >>>>> PCIDevice *pdev = PCI_DEVICE(dev); > >>>>> + int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev)); > >>>>> + > >>>>> + trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), bsel); > >>>>> > >>>>> - trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), > >>>>> - acpi_pcihp_get_bsel(pci_get_bus(pdev))); > >>>>> + if (bsel < 0) { > >>>>> + error_setg(errp, "Unsupported bus. Bus doesn't have property '" > >>>>> + ACPI_PCIHP_PROP_BSEL "' set"); > >>>>> + return; > >>>>> + } > >>> > >>> 1st: > >>> Error here is useless. this path is triggered on guest > >>> MMIO write and there is no consumer for error whatsoever. > >>> If I recall correctly, in such cases we in PCIHP code we make > >>> such access a silent NOP. And tracing is there for a us > >>> to help figure out what's going on. > >>> > >>> 2nd: > >>> if it got this far, it means a device on a bus with bsel > >>> was found and we are completing cleanup. Error-ing out at > >>> this point will leak acpi_index. > >> > >> The above two points seems to apply in this case as well and so should we > >> do this? > >> > >> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > >> index 0fd0c1d811..c7692f5d5f 100644 > >> --- a/hw/acpi/pcihp.c > >> +++ b/hw/acpi/pcihp.c > >> @@ -400,12 +400,6 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev, > >> > >> trace_acpi_pci_unplug_request(bsel, slot); > >> > >> - if (bsel < 0) { > >> - error_setg(errp, "Unsupported bus. Bus doesn't have property '" > >> - ACPI_PCIHP_PROP_BSEL "' set"); > >> - return; > >> - } > >> - > >> s->acpi_pcihp_pci_status[bsel].down |= (1U << slot); > >> acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > >> } > > > > or add g_assert() on both instead. > > 'git-blame' & read git history ('git-log -p hw/acpi/pcihp.c') > often helps to understand how the code evolved and why it is > not "symmetric". Right, my mistake was not to dig through the history. Will do that next time. For example see: > > commit ec266f408882fd38475f72c4e864ed576228643b > Author: David Hildenbrand <david@redhat.com> > Date: Wed Dec 12 10:16:17 2018 +0100 > > pci/pcihp: perform check for bus capability in pre_plug handler > > Perform the check in the pre_plug handler. In addition, we need > the capability only if the device is actually hotplugged (and not > created during machine initialization). This is a preparation for > coldplugging pci devices via that hotplug handler. > > From here try to figure out what happened, why this changed was > necessary, why there is no equivalent g_assert() as you noticed. > Actually this change isn't very insightful for unplug() callbacks. A more interesting change is: (a) commit c97adf3ccfbfbe6885fd9de7293162489d293d44 Author: David Hildenbrand <david@redhat.com> Date: Wed Dec 12 10:16:19 2018 +0100 pci/pcihp: perform unplug via the hotplug handler which basically says that the original function acpi_pcihp_device_unplug_cb() written by Igor got renamed to acpi_pcihp_device_unplug_request_cb(). Now, in Igor's original version of acpi_pcihp_device_unplug_cb(), it did have the check. See : (b) commit c24d5e0b91d138f8cc95f5694d4964de36a739d3 Author: Igor Mammedov <imammedo@redhat.com> Date: Wed Feb 5 16:36:49 2014 +0100 acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API Now because of (a) we see acpi_pcihp_device_unplug_request_cb() inherit the bsel check from acpi_pcihp_device_unplug_cb() but the later no longer gained it back. Anyways maybe it is good enough to have the bsel check in the pre-unplug handler. Still I would argue along the lines of the two points Igor mentions above that if there is no one to catch the error, why have such error messages printed on stderr anyway? A trace should be enough. > Then try to convince the reviewers why in your commit description :) > > See: > https://www.freecodecamp.org/news/writing-good-commit-messages-a-practical-guide/#how-to-write-good-commit-messages Yes I am aware of this article. Lets have a culture where people are encouraged to spend their unpaid spare time looking into Qemu/Libvirt and send patches.
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c index 0fd0c1d811..9982815a87 100644 --- a/hw/acpi/pcihp.c +++ b/hw/acpi/pcihp.c @@ -372,9 +372,15 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, DeviceState *dev, Error **errp) { PCIDevice *pdev = PCI_DEVICE(dev); + int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev)); + + trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), bsel); - trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), - acpi_pcihp_get_bsel(pci_get_bus(pdev))); + if (bsel < 0) { + error_setg(errp, "Unsupported bus. Bus doesn't have property '" + ACPI_PCIHP_PROP_BSEL "' set"); + return; + } /* * clean up acpi-index so it could reused by another device
Bsel property of the pci bus indicates whether the bus supports acpi hotplug. We need to validate the presence of this property before performing any hotplug related callback operations. Currently validation of the existence of this property was absent from acpi_pcihp_device_unplug_cb() function but is present in other hotplug/unplug callback functions. Hence, this change adds the missing check for the above function. Signed-off-by: Ani Sinha <ani@anisinha.ca> --- hw/acpi/pcihp.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)