| Message ID | 77aa6c01aefe1ebc4004e87b0bc714f2759f15c4.1575985006.git.lukas@wunner.de |
|---|---|
| State | New |
| Headers | show |
| Series | ALSA: hda/hdmi - Fix duplicate unref of pci_dev | expand |
On Tue, 10 Dec 2019 14:47:28 +0100, Nicholas Johnson wrote: > > On Tue, Dec 10, 2019 at 02:39:50PM +0100, Lukas Wunner wrote: > > Nicholas Johnson reports a null pointer deref as well as a refcount > > underflow upon hot-removal of a Thunderbolt-attached AMD eGPU. > > He's bisected the issue down to commit 586bc4aab878 ("ALSA: hda/hdmi - > > fix vgaswitcheroo detection for AMD"). > > > > The commit iterates over PCI devices using pci_get_class() and > > unreferences each device found, even though pci_get_class() > > subsequently unreferences the device as well. Fix it. > > > > Fixes: 586bc4aab878 ("ALSA: hda/hdmi - fix vgaswitcheroo detection for AMD") > > Link: https://lore.kernel.org/r/PSXP216MB0438BFEAA0617283A834E11580580@PSXP216MB0438.KORP216.PROD.OUTLOOK.COM/ > > Reported-and-tested-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au> > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > > Cc: Alexander Deucher <alexander.deucher@amd.com> > > Cc: Bjorn Helgaas <helgaas@kernel.org> > > --- > > sound/pci/hda/hda_intel.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > > index 35b4526f0d28..b856b89378ac 100644 > > --- a/sound/pci/hda/hda_intel.c > > +++ b/sound/pci/hda/hda_intel.c > > @@ -1419,7 +1419,6 @@ static bool atpx_present(void) > > return true; > > } > > } > > - pci_dev_put(pdev); > > } > > return false; > > } > > -- > > 2.24.0 > > > Extremely fast turnaround from bisect to patch. > > If you want the bugzilla.kernel.org report then I can do that ASAP, but > I feel that it is redundant now. We have now Link tag to track the ML archive, so it should suffice, I suppose. > Thank you for handling my bug report. And thank you for your quick testing. Takashi
> -----Original Message----- > From: Lukas Wunner <lukas@wunner.de> > Sent: Tuesday, December 10, 2019 8:40 AM > To: Takashi Iwai <tiwai@suse.de> > Cc: Jaroslav Kysela <perex@perex.cz>; Deucher, Alexander > <Alexander.Deucher@amd.com>; Mika Westerberg > <mika.westerberg@linux.intel.com>; Bjorn Helgaas <helgaas@kernel.org>; > Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>; alsa- > devel@alsa-project.org; linux-kernel@vger.kernel.org; linux- > pci@vger.kernel.org > Subject: [PATCH] ALSA: hda/hdmi - Fix duplicate unref of pci_dev > > Nicholas Johnson reports a null pointer deref as well as a refcount underflow > upon hot-removal of a Thunderbolt-attached AMD eGPU. > He's bisected the issue down to commit 586bc4aab878 ("ALSA: hda/hdmi - fix > vgaswitcheroo detection for AMD"). > > The commit iterates over PCI devices using pci_get_class() and unreferences > each device found, even though pci_get_class() subsequently unreferences > the device as well. Fix it. The pci_dev_put() a few lines above should probably be dropped as well. Alex > > Fixes: 586bc4aab878 ("ALSA: hda/hdmi - fix vgaswitcheroo detection for > AMD") > Link: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore. > kernel.org%2Fr%2FPSXP216MB0438BFEAA0617283A834E11580580%40PSXP2 > 16MB0438.KORP216.PROD.OUTLOOK.COM%2F&data=02%7C01%7Calex > ander.deucher%40amd.com%7C254649b79a6240f3a8aa08d77d76715f%7C3d > d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637115819998031934& > ;sdata=qI%2B%2FJv3Z6pvwN7gQFSnZiP31YUAvd%2BKjo0ZqMERFbYU%3D&a > mp;reserved=0 > Reported-and-tested-by: Nicholas Johnson <nicholas.johnson- > opensource@outlook.com.au> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: Alexander Deucher <alexander.deucher@amd.com> > Cc: Bjorn Helgaas <helgaas@kernel.org> > --- > sound/pci/hda/hda_intel.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index > 35b4526f0d28..b856b89378ac 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -1419,7 +1419,6 @@ static bool atpx_present(void) > return true; > } > } > - pci_dev_put(pdev); > } > return false; > } > -- > 2.24.0
On Tue, Dec 10, 2019 at 03:34:27PM +0000, Deucher, Alexander wrote: > > Nicholas Johnson reports a null pointer deref as well as a refcount underflow > > upon hot-removal of a Thunderbolt-attached AMD eGPU. > > He's bisected the issue down to commit 586bc4aab878 ("ALSA: hda/hdmi - fix > > vgaswitcheroo detection for AMD"). > > > > The commit iterates over PCI devices using pci_get_class() and unreferences > > each device found, even though pci_get_class() subsequently unreferences > > the device as well. Fix it. > > The pci_dev_put() a few lines above should probably be dropped as well. That one looks fine to me. The refcount is already increased in the caller get_bound_vga() via pci_get_domain_bus_and_slot() and it's increased again in atpx_present() via pci_get_class(). It needs to be decremented in atpx_present() to avoid leaking a ref. Thanks, Lukas > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index > > 35b4526f0d28..b856b89378ac 100644 > > --- a/sound/pci/hda/hda_intel.c > > +++ b/sound/pci/hda/hda_intel.c > > @@ -1419,7 +1419,6 @@ static bool atpx_present(void) > > return true; > > } > > } > > - pci_dev_put(pdev); > > } > > return false; > > } > > -- > > 2.24.0
> -----Original Message----- > From: Lukas Wunner <lukas@wunner.de> > Sent: Tuesday, December 10, 2019 10:47 AM > To: Deucher, Alexander <Alexander.Deucher@amd.com> > Cc: Takashi Iwai <tiwai@suse.de>; Jaroslav Kysela <perex@perex.cz>; Mika > Westerberg <mika.westerberg@linux.intel.com>; Bjorn Helgaas > <helgaas@kernel.org>; Nicholas Johnson <nicholas.johnson- > opensource@outlook.com.au>; alsa-devel@alsa-project.org; linux- > kernel@vger.kernel.org; linux-pci@vger.kernel.org > Subject: Re: [PATCH] ALSA: hda/hdmi - Fix duplicate unref of pci_dev > > On Tue, Dec 10, 2019 at 03:34:27PM +0000, Deucher, Alexander wrote: > > > Nicholas Johnson reports a null pointer deref as well as a refcount > > > underflow upon hot-removal of a Thunderbolt-attached AMD eGPU. > > > He's bisected the issue down to commit 586bc4aab878 ("ALSA: hda/hdmi > > > - fix vgaswitcheroo detection for AMD"). > > > > > > The commit iterates over PCI devices using pci_get_class() and > > > unreferences each device found, even though pci_get_class() > > > subsequently unreferences the device as well. Fix it. > > > > The pci_dev_put() a few lines above should probably be dropped as well. > > That one looks fine to me. The refcount is already increased in the caller > get_bound_vga() via pci_get_domain_bus_and_slot() and it's increased > again in atpx_present() via pci_get_class(). It needs to be decremented in > atpx_present() to avoid leaking a ref. I'm not following. This is part of the same loop as the one you removed. All we are doing is checking whether the ATPX method exists or not om the platform. The pdev may not be the same one as the one in pci_get_domain_bus_and_slot(). The APTX method in the APU's ACPI namespace, not the dGPUs. Alex > > Thanks, > > Lukas > > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > > > index 35b4526f0d28..b856b89378ac 100644 > > > --- a/sound/pci/hda/hda_intel.c > > > +++ b/sound/pci/hda/hda_intel.c > > > @@ -1419,7 +1419,6 @@ static bool atpx_present(void) > > > return true; > > > } > > > } > > > - pci_dev_put(pdev); > > > } > > > return false; > > > } > > > -- > > > 2.24.0
On Tue, 10 Dec 2019 16:53:20 +0100, Deucher, Alexander wrote: > > > -----Original Message----- > > From: Lukas Wunner <lukas@wunner.de> > > Sent: Tuesday, December 10, 2019 10:47 AM > > To: Deucher, Alexander <Alexander.Deucher@amd.com> > > Cc: Takashi Iwai <tiwai@suse.de>; Jaroslav Kysela <perex@perex.cz>; Mika > > Westerberg <mika.westerberg@linux.intel.com>; Bjorn Helgaas > > <helgaas@kernel.org>; Nicholas Johnson <nicholas.johnson- > > opensource@outlook.com.au>; alsa-devel@alsa-project.org; linux- > > kernel@vger.kernel.org; linux-pci@vger.kernel.org > > Subject: Re: [PATCH] ALSA: hda/hdmi - Fix duplicate unref of pci_dev > > > > On Tue, Dec 10, 2019 at 03:34:27PM +0000, Deucher, Alexander wrote: > > > > Nicholas Johnson reports a null pointer deref as well as a refcount > > > > underflow upon hot-removal of a Thunderbolt-attached AMD eGPU. > > > > He's bisected the issue down to commit 586bc4aab878 ("ALSA: hda/hdmi > > > > - fix vgaswitcheroo detection for AMD"). > > > > > > > > The commit iterates over PCI devices using pci_get_class() and > > > > unreferences each device found, even though pci_get_class() > > > > subsequently unreferences the device as well. Fix it. > > > > > > The pci_dev_put() a few lines above should probably be dropped as well. > > > > That one looks fine to me. The refcount is already increased in the caller > > get_bound_vga() via pci_get_domain_bus_and_slot() and it's increased > > again in atpx_present() via pci_get_class(). It needs to be decremented in > > atpx_present() to avoid leaking a ref. > > I'm not following. This is part of the same loop as the one you removed. All we are doing is checking whether the ATPX method exists or not om the platform. The pdev may not be the same one as the one in pci_get_domain_bus_and_slot(). The APTX method in the APU's ACPI namespace, not the dGPUs. Well, the tricky part is that pci_get_class() itself does unrefeference the old object and reference the new object (if found). At the end of the loop, nothing is referenced, so it's fine. OTOH, if you go out of the loop in the middle, you're still keeping the pdev object reference, so you need to manually unreference it. Takashi > > Alex > > > > > Thanks, > > > > Lukas > > > > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > > > > index 35b4526f0d28..b856b89378ac 100644 > > > > --- a/sound/pci/hda/hda_intel.c > > > > +++ b/sound/pci/hda/hda_intel.c > > > > @@ -1419,7 +1419,6 @@ static bool atpx_present(void) > > > > return true; > > > > } > > > > } > > > > - pci_dev_put(pdev); > > > > } > > > > return false; > > > > } > > > > -- > > > > 2.24.0 >
On Tue, Dec 10, 2019 at 03:53:20PM +0000, Deucher, Alexander wrote: > > On Tue, Dec 10, 2019 at 03:34:27PM +0000, Deucher, Alexander wrote: > > > > Nicholas Johnson reports a null pointer deref as well as a refcount > > > > underflow upon hot-removal of a Thunderbolt-attached AMD eGPU. > > > > He's bisected the issue down to commit 586bc4aab878 ("ALSA: hda/hdmi > > > > - fix vgaswitcheroo detection for AMD"). > > > > > > > > The commit iterates over PCI devices using pci_get_class() and > > > > unreferences each device found, even though pci_get_class() > > > > subsequently unreferences the device as well. Fix it. > > > > > > The pci_dev_put() a few lines above should probably be dropped as well. > > > > That one looks fine to me. The refcount is already increased in the caller > > get_bound_vga() via pci_get_domain_bus_and_slot() and it's increased > > again in atpx_present() via pci_get_class(). It needs to be decremented in > > atpx_present() to avoid leaking a ref. > > I'm not following. This is part of the same loop as the one you removed. > All we are doing is checking whether the ATPX method exists or not om the > platform. The pdev may not be the same one as the one in > pci_get_domain_bus_and_slot(). The APTX method in the APU's ACPI namespace, > not the dGPUs. Okay. Still, atpx_present() doesn't pass the found pci_dev back to the caller, so it would be leaked if the ref isn't returned. The situation is different for the pci_dev_put() I removed: The ref is returned by pci_get_class() on the next loop iteration. Thanks, Lukas > > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > > > > index 35b4526f0d28..b856b89378ac 100644 > > > > --- a/sound/pci/hda/hda_intel.c > > > > +++ b/sound/pci/hda/hda_intel.c > > > > @@ -1419,7 +1419,6 @@ static bool atpx_present(void) > > > > return true; > > > > } > > > > } > > > > - pci_dev_put(pdev); > > > > } > > > > return false; > > > > } > > > > -- > > > > 2.24.0
> -----Original Message----- > From: Takashi Iwai <tiwai@suse.de> > Sent: Tuesday, December 10, 2019 11:11 AM > To: Deucher, Alexander <Alexander.Deucher@amd.com> > Cc: Lukas Wunner <lukas@wunner.de>; Jaroslav Kysela <perex@perex.cz>; > Mika Westerberg <mika.westerberg@linux.intel.com>; Bjorn Helgaas > <helgaas@kernel.org>; Nicholas Johnson <nicholas.johnson- > opensource@outlook.com.au>; alsa-devel@alsa-project.org; linux- > kernel@vger.kernel.org; linux-pci@vger.kernel.org > Subject: Re: [PATCH] ALSA: hda/hdmi - Fix duplicate unref of pci_dev > > On Tue, 10 Dec 2019 16:53:20 +0100, > Deucher, Alexander wrote: > > > > > -----Original Message----- > > > From: Lukas Wunner <lukas@wunner.de> > > > Sent: Tuesday, December 10, 2019 10:47 AM > > > To: Deucher, Alexander <Alexander.Deucher@amd.com> > > > Cc: Takashi Iwai <tiwai@suse.de>; Jaroslav Kysela <perex@perex.cz>; > > > Mika Westerberg <mika.westerberg@linux.intel.com>; Bjorn Helgaas > > > <helgaas@kernel.org>; Nicholas Johnson <nicholas.johnson- > > > opensource@outlook.com.au>; alsa-devel@alsa-project.org; linux- > > > kernel@vger.kernel.org; linux-pci@vger.kernel.org > > > Subject: Re: [PATCH] ALSA: hda/hdmi - Fix duplicate unref of pci_dev > > > > > > On Tue, Dec 10, 2019 at 03:34:27PM +0000, Deucher, Alexander wrote: > > > > > Nicholas Johnson reports a null pointer deref as well as a > > > > > refcount underflow upon hot-removal of a Thunderbolt-attached > AMD eGPU. > > > > > He's bisected the issue down to commit 586bc4aab878 ("ALSA: > > > > > hda/hdmi > > > > > - fix vgaswitcheroo detection for AMD"). > > > > > > > > > > The commit iterates over PCI devices using pci_get_class() and > > > > > unreferences each device found, even though pci_get_class() > > > > > subsequently unreferences the device as well. Fix it. > > > > > > > > The pci_dev_put() a few lines above should probably be dropped as > well. > > > > > > That one looks fine to me. The refcount is already increased in the > > > caller > > > get_bound_vga() via pci_get_domain_bus_and_slot() and it's increased > > > again in atpx_present() via pci_get_class(). It needs to be > > > decremented in > > > atpx_present() to avoid leaking a ref. > > > > I'm not following. This is part of the same loop as the one you removed. All > we are doing is checking whether the ATPX method exists or not om the > platform. The pdev may not be the same one as the one in > pci_get_domain_bus_and_slot(). The APTX method in the APU's ACPI > namespace, not the dGPUs. > > Well, the tricky part is that pci_get_class() itself does unrefeference the old > object and reference the new object (if found). > At the end of the loop, nothing is referenced, so it's fine. > OTOH, if you go out of the loop in the middle, you're still keeping the pdev > object reference, so you need to manually unreference it. > Ah, I see what you are saying. Thanks. Patch is: Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > Takashi > > > > > Alex > > > > > > > > Thanks, > > > > > > Lukas > > > > > > > > diff --git a/sound/pci/hda/hda_intel.c > > > > > b/sound/pci/hda/hda_intel.c index 35b4526f0d28..b856b89378ac > > > > > 100644 > > > > > --- a/sound/pci/hda/hda_intel.c > > > > > +++ b/sound/pci/hda/hda_intel.c > > > > > @@ -1419,7 +1419,6 @@ static bool atpx_present(void) > > > > > return true; > > > > > } > > > > > } > > > > > - pci_dev_put(pdev); > > > > > } > > > > > return false; > > > > > } > > > > > -- > > > > > 2.24.0 > >
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 35b4526f0d28..b856b89378ac 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1419,7 +1419,6 @@ static bool atpx_present(void) return true; } } - pci_dev_put(pdev); } return false; }
Nicholas Johnson reports a null pointer deref as well as a refcount underflow upon hot-removal of a Thunderbolt-attached AMD eGPU. He's bisected the issue down to commit 586bc4aab878 ("ALSA: hda/hdmi - fix vgaswitcheroo detection for AMD"). The commit iterates over PCI devices using pci_get_class() and unreferences each device found, even though pci_get_class() subsequently unreferences the device as well. Fix it. Fixes: 586bc4aab878 ("ALSA: hda/hdmi - fix vgaswitcheroo detection for AMD") Link: https://lore.kernel.org/r/PSXP216MB0438BFEAA0617283A834E11580580@PSXP216MB0438.KORP216.PROD.OUTLOOK.COM/ Reported-and-tested-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au> Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Alexander Deucher <alexander.deucher@amd.com> Cc: Bjorn Helgaas <helgaas@kernel.org> --- sound/pci/hda/hda_intel.c | 1 - 1 file changed, 1 deletion(-)