diff mbox series

PCI/ACPI: do not reference a pci device after it has been released

Message ID 20220428142854.1065953-1-gregkh@linuxfoundation.org
State New
Headers show
Series PCI/ACPI: do not reference a pci device after it has been released | expand

Commit Message

Greg Kroah-Hartman April 28, 2022, 2:28 p.m. UTC
In acpi_get_pci_dev(), the debugging message for when a PCI bridge is
not found uses a pointer to a pci device whose reference has just been
dropped.  The chance that this really is a device that is now been
removed from the system is almost impossible to happen, but to be safe,
let's print out the debugging message based on the acpi root device
which we do have a valid reference to at the moment.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-pci@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Reported-by: whitehat002 <hackyzh002@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/acpi/pci_root.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Bjorn Helgaas April 28, 2022, 3:58 p.m. UTC | #1
On Thu, Apr 28, 2022 at 04:28:53PM +0200, Greg Kroah-Hartman wrote:
> In acpi_get_pci_dev(), the debugging message for when a PCI bridge is
> not found uses a pointer to a pci device whose reference has just been
> dropped.  The chance that this really is a device that is now been
> removed from the system is almost impossible to happen, but to be safe,
> let's print out the debugging message based on the acpi root device
> which we do have a valid reference to at the moment.

This code was added by 497fb54f578e ("ACPI / PCI: Fix NULL pointer
dereference in acpi_get_pci_dev() (rev. 2)").  Not sure if it's worth
a Fixes: tag.

acpi_get_pci_dev() is used by only five callers, three of which are
video/backlight related.  I'm always skeptical of one-off interfaces
like this, but I don't know enough to propose any refactoring or other
alternatives.

I'll leave this for Rafael, but if I were applying I would silently
touch up the subject to match convention:

  PCI/ACPI: Do not reference PCI device after it has been released

> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-pci@vger.kernel.org
> Cc: linux-acpi@vger.kernel.org
> Reported-by: whitehat002 <hackyzh002@gmail.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/acpi/pci_root.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 6f9e75d14808..ecda378dbc09 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -303,7 +303,8 @@ struct pci_dev *acpi_get_pci_dev(acpi_handle handle)
>  		 * case pdev->subordinate will be NULL for the parent.
>  		 */
>  		if (!pbus) {
> -			dev_dbg(&pdev->dev, "Not a PCI-to-PCI bridge\n");
> +			dev_dbg(&root->device->dev,
> +				"dev %d, function %d is not a PCI-to-PCI bridge\n", dev, fn);

This should use "%02x.%d" to be consistent with the dev_set_name() in
pci_setup_device().

>  			pdev = NULL;
>  			break;
>  		}
> -- 
> 2.36.0
>
Rafael J. Wysocki April 28, 2022, 8:30 p.m. UTC | #2
On Thu, Apr 28, 2022 at 10:15 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Apr 28, 2022 at 6:22 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Apr 28, 2022 at 10:58:58AM -0500, Bjorn Helgaas wrote:
> > > On Thu, Apr 28, 2022 at 04:28:53PM +0200, Greg Kroah-Hartman wrote:
> > > > In acpi_get_pci_dev(), the debugging message for when a PCI bridge is
> > > > not found uses a pointer to a pci device whose reference has just been
> > > > dropped.  The chance that this really is a device that is now been
> > > > removed from the system is almost impossible to happen, but to be safe,
> > > > let's print out the debugging message based on the acpi root device
> > > > which we do have a valid reference to at the moment.
> > >
> > > This code was added by 497fb54f578e ("ACPI / PCI: Fix NULL pointer
> > > dereference in acpi_get_pci_dev() (rev. 2)").  Not sure if it's worth
> > > a Fixes: tag.
> >
> > Can't hurt, I'll add it for the v2 based on this review.
> >
> > >
> > > acpi_get_pci_dev() is used by only five callers, three of which are
> > > video/backlight related.  I'm always skeptical of one-off interfaces
> > > like this, but I don't know enough to propose any refactoring or other
> > > alternatives.
> > >
> > > I'll leave this for Rafael, but if I were applying I would silently
> > > touch up the subject to match convention:
> > >
> > >   PCI/ACPI: Do not reference PCI device after it has been released
> >
> > Much simpler, thanks.
> >
> > >
> > > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > Cc: Len Brown <lenb@kernel.org>
> > > > Cc: linux-pci@vger.kernel.org
> > > > Cc: linux-acpi@vger.kernel.org
> > > > Reported-by: whitehat002 <hackyzh002@gmail.com>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > ---
> > > >  drivers/acpi/pci_root.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > > > index 6f9e75d14808..ecda378dbc09 100644
> > > > --- a/drivers/acpi/pci_root.c
> > > > +++ b/drivers/acpi/pci_root.c
> > > > @@ -303,7 +303,8 @@ struct pci_dev *acpi_get_pci_dev(acpi_handle handle)
> > > >              * case pdev->subordinate will be NULL for the parent.
> > > >              */
> > > >             if (!pbus) {
> > > > -                   dev_dbg(&pdev->dev, "Not a PCI-to-PCI bridge\n");
> > > > +                   dev_dbg(&root->device->dev,
> > > > +                           "dev %d, function %d is not a PCI-to-PCI bridge\n", dev, fn);
> > >
> > > This should use "%02x.%d" to be consistent with the dev_set_name() in
> > > pci_setup_device().
> >
> > Ah, missed that, will change it and send out a new version tomorrow.
>
> I would make the change below (modulo the gmail-induced wthite space
> breakage), though.

That said ->

> ---
>  drivers/acpi/pci_root.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/acpi/pci_root.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/pci_root.c
> +++ linux-pm/drivers/acpi/pci_root.c
> @@ -295,8 +295,6 @@ struct pci_dev *acpi_get_pci_dev(acpi_ha
>              break;
>
>          pbus = pdev->subordinate;
> -        pci_dev_put(pdev);
> -
>          /*
>           * This function may be called for a non-PCI device that has a
>           * PCI parent (eg. a disk under a PCI SATA controller).  In that
> @@ -304,9 +302,12 @@ struct pci_dev *acpi_get_pci_dev(acpi_ha
>           */
>          if (!pbus) {
>              dev_dbg(&pdev->dev, "Not a PCI-to-PCI bridge\n");
> +            pci_dev_put(pdev);
>              pdev = NULL;
>              break;
>          }
> +
> +        pci_dev_put(pdev);

-> we are going to use pbus after this and it is pdev->subordinate
which cannot survive without pdev AFAICS.

Are we not concerned about this case?

>      }
>  out:
>      list_for_each_entry_safe(node, tmp, &device_list, node)
Greg Kroah-Hartman June 27, 2022, 3:07 p.m. UTC | #3
On Thu, Apr 28, 2022 at 10:30:38PM +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 28, 2022 at 10:15 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Apr 28, 2022 at 6:22 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Apr 28, 2022 at 10:58:58AM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Apr 28, 2022 at 04:28:53PM +0200, Greg Kroah-Hartman wrote:
> > > > > In acpi_get_pci_dev(), the debugging message for when a PCI bridge is
> > > > > not found uses a pointer to a pci device whose reference has just been
> > > > > dropped.  The chance that this really is a device that is now been
> > > > > removed from the system is almost impossible to happen, but to be safe,
> > > > > let's print out the debugging message based on the acpi root device
> > > > > which we do have a valid reference to at the moment.
> > > >
> > > > This code was added by 497fb54f578e ("ACPI / PCI: Fix NULL pointer
> > > > dereference in acpi_get_pci_dev() (rev. 2)").  Not sure if it's worth
> > > > a Fixes: tag.
> > >
> > > Can't hurt, I'll add it for the v2 based on this review.
> > >
> > > >
> > > > acpi_get_pci_dev() is used by only five callers, three of which are
> > > > video/backlight related.  I'm always skeptical of one-off interfaces
> > > > like this, but I don't know enough to propose any refactoring or other
> > > > alternatives.
> > > >
> > > > I'll leave this for Rafael, but if I were applying I would silently
> > > > touch up the subject to match convention:
> > > >
> > > >   PCI/ACPI: Do not reference PCI device after it has been released
> > >
> > > Much simpler, thanks.
> > >
> > > >
> > > > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > > Cc: Len Brown <lenb@kernel.org>
> > > > > Cc: linux-pci@vger.kernel.org
> > > > > Cc: linux-acpi@vger.kernel.org
> > > > > Reported-by: whitehat002 <hackyzh002@gmail.com>
> > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > ---
> > > > >  drivers/acpi/pci_root.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > > > > index 6f9e75d14808..ecda378dbc09 100644
> > > > > --- a/drivers/acpi/pci_root.c
> > > > > +++ b/drivers/acpi/pci_root.c
> > > > > @@ -303,7 +303,8 @@ struct pci_dev *acpi_get_pci_dev(acpi_handle handle)
> > > > >              * case pdev->subordinate will be NULL for the parent.
> > > > >              */
> > > > >             if (!pbus) {
> > > > > -                   dev_dbg(&pdev->dev, "Not a PCI-to-PCI bridge\n");
> > > > > +                   dev_dbg(&root->device->dev,
> > > > > +                           "dev %d, function %d is not a PCI-to-PCI bridge\n", dev, fn);
> > > >
> > > > This should use "%02x.%d" to be consistent with the dev_set_name() in
> > > > pci_setup_device().
> > >
> > > Ah, missed that, will change it and send out a new version tomorrow.
> >
> > I would make the change below (modulo the gmail-induced wthite space
> > breakage), though.
> 
> That said ->
> 
> > ---
> >  drivers/acpi/pci_root.c |    5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/pci_root.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/pci_root.c
> > +++ linux-pm/drivers/acpi/pci_root.c
> > @@ -295,8 +295,6 @@ struct pci_dev *acpi_get_pci_dev(acpi_ha
> >              break;
> >
> >          pbus = pdev->subordinate;
> > -        pci_dev_put(pdev);
> > -
> >          /*
> >           * This function may be called for a non-PCI device that has a
> >           * PCI parent (eg. a disk under a PCI SATA controller).  In that
> > @@ -304,9 +302,12 @@ struct pci_dev *acpi_get_pci_dev(acpi_ha
> >           */
> >          if (!pbus) {
> >              dev_dbg(&pdev->dev, "Not a PCI-to-PCI bridge\n");
> > +            pci_dev_put(pdev);
> >              pdev = NULL;
> >              break;
> >          }
> > +
> > +        pci_dev_put(pdev);
> 
> -> we are going to use pbus after this and it is pdev->subordinate
> which cannot survive without pdev AFAICS.
> 
> Are we not concerned about this case?

Good point.

whitehat002, any ideas?  You found this issue but it really looks like
it is not anything that can ever be hit, so how far do you want to go to
unwind it?

thanks,

greg k-h
Rafael J. Wysocki June 27, 2022, 4:37 p.m. UTC | #4
On Mon, Jun 27, 2022 at 5:07 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Apr 28, 2022 at 10:30:38PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 28, 2022 at 10:15 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Thu, Apr 28, 2022 at 6:22 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Apr 28, 2022 at 10:58:58AM -0500, Bjorn Helgaas wrote:
> > > > > On Thu, Apr 28, 2022 at 04:28:53PM +0200, Greg Kroah-Hartman wrote:
> > > > > > In acpi_get_pci_dev(), the debugging message for when a PCI bridge is
> > > > > > not found uses a pointer to a pci device whose reference has just been
> > > > > > dropped.  The chance that this really is a device that is now been
> > > > > > removed from the system is almost impossible to happen, but to be safe,
> > > > > > let's print out the debugging message based on the acpi root device
> > > > > > which we do have a valid reference to at the moment.
> > > > >
> > > > > This code was added by 497fb54f578e ("ACPI / PCI: Fix NULL pointer
> > > > > dereference in acpi_get_pci_dev() (rev. 2)").  Not sure if it's worth
> > > > > a Fixes: tag.
> > > >
> > > > Can't hurt, I'll add it for the v2 based on this review.
> > > >
> > > > >
> > > > > acpi_get_pci_dev() is used by only five callers, three of which are
> > > > > video/backlight related.  I'm always skeptical of one-off interfaces
> > > > > like this, but I don't know enough to propose any refactoring or other
> > > > > alternatives.
> > > > >
> > > > > I'll leave this for Rafael, but if I were applying I would silently
> > > > > touch up the subject to match convention:
> > > > >
> > > > >   PCI/ACPI: Do not reference PCI device after it has been released
> > > >
> > > > Much simpler, thanks.
> > > >
> > > > >
> > > > > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > > > Cc: Len Brown <lenb@kernel.org>
> > > > > > Cc: linux-pci@vger.kernel.org
> > > > > > Cc: linux-acpi@vger.kernel.org
> > > > > > Reported-by: whitehat002 <hackyzh002@gmail.com>
> > > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > ---
> > > > > >  drivers/acpi/pci_root.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > > > > > index 6f9e75d14808..ecda378dbc09 100644
> > > > > > --- a/drivers/acpi/pci_root.c
> > > > > > +++ b/drivers/acpi/pci_root.c
> > > > > > @@ -303,7 +303,8 @@ struct pci_dev *acpi_get_pci_dev(acpi_handle handle)
> > > > > >              * case pdev->subordinate will be NULL for the parent.
> > > > > >              */
> > > > > >             if (!pbus) {
> > > > > > -                   dev_dbg(&pdev->dev, "Not a PCI-to-PCI bridge\n");
> > > > > > +                   dev_dbg(&root->device->dev,
> > > > > > +                           "dev %d, function %d is not a PCI-to-PCI bridge\n", dev, fn);
> > > > >
> > > > > This should use "%02x.%d" to be consistent with the dev_set_name() in
> > > > > pci_setup_device().
> > > >
> > > > Ah, missed that, will change it and send out a new version tomorrow.
> > >
> > > I would make the change below (modulo the gmail-induced wthite space
> > > breakage), though.
> >
> > That said ->
> >
> > > ---
> > >  drivers/acpi/pci_root.c |    5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > Index: linux-pm/drivers/acpi/pci_root.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/pci_root.c
> > > +++ linux-pm/drivers/acpi/pci_root.c
> > > @@ -295,8 +295,6 @@ struct pci_dev *acpi_get_pci_dev(acpi_ha
> > >              break;
> > >
> > >          pbus = pdev->subordinate;
> > > -        pci_dev_put(pdev);
> > > -
> > >          /*
> > >           * This function may be called for a non-PCI device that has a
> > >           * PCI parent (eg. a disk under a PCI SATA controller).  In that
> > > @@ -304,9 +302,12 @@ struct pci_dev *acpi_get_pci_dev(acpi_ha
> > >           */
> > >          if (!pbus) {
> > >              dev_dbg(&pdev->dev, "Not a PCI-to-PCI bridge\n");
> > > +            pci_dev_put(pdev);
> > >              pdev = NULL;
> > >              break;
> > >          }
> > > +
> > > +        pci_dev_put(pdev);
> >
> > -> we are going to use pbus after this and it is pdev->subordinate
> > which cannot survive without pdev AFAICS.
> >
> > Are we not concerned about this case?
>
> Good point.
>
> whitehat002, any ideas?  You found this issue but it really looks like
> it is not anything that can ever be hit, so how far do you want to go to
> unwind it?

I have an idea, sorry for the delay here.

I should be ready to post something tomorrow.

Cheers!
diff mbox series

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 6f9e75d14808..ecda378dbc09 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -303,7 +303,8 @@  struct pci_dev *acpi_get_pci_dev(acpi_handle handle)
 		 * case pdev->subordinate will be NULL for the parent.
 		 */
 		if (!pbus) {
-			dev_dbg(&pdev->dev, "Not a PCI-to-PCI bridge\n");
+			dev_dbg(&root->device->dev,
+				"dev %d, function %d is not a PCI-to-PCI bridge\n", dev, fn);
 			pdev = NULL;
 			break;
 		}