| Message ID | 20140214173544.GK32602@nuc-i3427.alporthouse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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"); > } > >
On Fri, Feb 14, 2014 at 07:48:18PM +0200, Imre Deak wrote: > On Fri, 2014-02-14 at 17:35 +0000, Chris Wilson wrote: > > @@ -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. I don't think you can drop the continue without adding breaks to every branch; one continue won vs remembering to add a break every time. pci_dev_put(NULL) is safe so you could put it outside the loop, I left it inside as I felt it was cleaner to leave the debug msg by itself. -Chris > > } > > -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"); > > }
On Fri, 2014-02-14 at 19:48 +0200, Imre Deak wrote: > 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. Ah sorry, the continue is still needed. But for clarity I'd still move the pci_dev_put out. --Imre > > --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"); > > } > > > > >
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; } -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"); }