[alsa-devel] [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.
Fixes: 586bc4aab878 ("ALSA: hda/hdmi - fix vgaswitcheroo detection for AMD") Link: https://lore.kernel.org/r/PSXP216MB0438BFEAA0617283A834E11580580@PSXP216MB04... 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; }
On Tue, 10 Dec 2019 14:39:50 +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@PSXP216MB04... 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
Applied now. Thanks.
Takashi
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);}
2.24.0
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@PSXP216MB04... 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);}
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.
Thank you for handling my bug report.
Regards, Nicholas
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@PSXP216MB04... 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);}
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);}
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);}
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);}
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);}
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);}
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);}
2.24.0
participants (4)
-
Deucher, Alexander -
Lukas Wunner -
Nicholas Johnson -
Takashi Iwai