On Fri, 2014-02-14 at 17:35 +0000, Chris Wilson wrote:
> On Fri, Feb 14, 2014 at 07:23:32PM +0200, Imre Deak wrote:
> > pci_get_class(class, from) drops the refcount for 'from', so the
> > extra pci_dev_put we do on it will result in a use after free bug
> > sometime later starting with the WARN below.
> 
> That's a very nice find.
> 
> But you can tidy this loop up even more...
> 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 2d05d7c..4e4fc0c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -347,7 +347,6 @@ void intel_detect_pch(struct drm_device *dev)
> >      */
> >     pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
> >     while (pch) {
> 
> ...by noting that what you want here is
> while ((pch = pci_get_class(ISA<<8, pch)) {
> 
> > -           struct pci_dev *curr = pch;
> >             if (pch->vendor == PCI_VENDOR_ID_INTEL) {
> >                     unsigned short id;
> >                     id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
> > @@ -385,15 +384,15 @@ void intel_detect_pch(struct drm_device *dev)
> >                     } else {
> >                             goto check_next;
> >                     }
> > -                   pci_dev_put(pch);
> >                     break;
> >             }
> >  check_next:
> > -           pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
> > -           pci_dev_put(curr);
> > +           pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch);
> >     }
> >     if (!pch)
> >             DRM_DEBUG_KMS("No PCH found?\n");
> > +   else
> > +           pci_dev_put(pch);
> >  }
> 
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2d05d7c..9962aef 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -324,7 +324,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
>  void intel_detect_pch(struct drm_device *dev)
>  {
>       struct drm_i915_private *dev_priv = dev->dev_private;
> -     struct pci_dev *pch;
> +     struct pci_dev *pch = NULL;
>  
>       /* In all current cases, num_pipes is equivalent to the PCH_NOP setting
>        * (which really amounts to a PCH but no South Display).
> @@ -345,12 +345,9 @@ void intel_detect_pch(struct drm_device *dev)
>        * all the ISA bridge devices and check for the first match, instead
>        * of only checking the first one.
>        */
> -     pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
> -     while (pch) {
> -             struct pci_dev *curr = pch;
> +     while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch))) {
>               if (pch->vendor == PCI_VENDOR_ID_INTEL) {
> -                     unsigned short id;
> -                     id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
> +                     unsigned short id = pch->device & 
> INTEL_PCH_DEVICE_ID_MASK;
>                       dev_priv->pch_id = id;
>  
>                       if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
> @@ -382,18 +379,15 @@ void intel_detect_pch(struct drm_device *dev)
>                               DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
>                               WARN_ON(!IS_HASWELL(dev));
>                               WARN_ON(!IS_ULT(dev));
> -                     } else {
> -                             goto check_next;
> -                     }
> +                     } else
> +                             continue;
> +
>                       pci_dev_put(pch);
>                       break;

Yep, looks better. I would also move the pci_dev_put out of the loop and
remove the above continue. But it's fine for me either way.

--Imre

>               }
> -check_next:
> -             pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
> -             pci_dev_put(curr);
>       }
>       if (!pch)
> -             DRM_DEBUG_KMS("No PCH found?\n");
> +             DRM_DEBUG_KMS("No PCH found.\n");
>  }
>  
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to