linux-kernel - Re: switcheroo registration vs switching race...

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 03 Dec 2012 15:40:37 +0100
From:	Takashi Iwai <tiwai@...e.de>
To:	Daniel J Blueman <daniel@...ra.org>
Cc:	Seth Forshee <seth.forshee@...onical.com>,
	Dave Airlie <airlied@...ux.ie>,
	Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: switcheroo registration vs switching race...

At Mon, 3 Dec 2012 22:25:52 +0800,
Daniel J Blueman wrote:
> 
> On 3 December 2012 19:17, Takashi Iwai <tiwai@...e.de> wrote:
> > At Wed, 28 Nov 2012 09:45:39 +0100,
> > Takashi Iwai wrote:
> >>
> >> At Wed, 28 Nov 2012 11:45:07 +0800,
> >> Daniel J Blueman wrote:
> >> >
> >> > Hi Seth, Dave, Takashi,
> >> >
> >> > If I power down the unused discrete GPU before lightdm starts by
> >> > fiddling with the sysfs file [1] in the upstart script, I see a race
> >> > manifesting as the discrete GPU's HDA controller timing out to
> >> > commands [2].
> >> >
> >> > Adding some debug, I see that the registered audio devices are put
> >> > into D3 before the GPU is, but it turns out that the discrete (and
> >> > internal) GPU's HDA controller gets registered a bit later, so the
> >> > list is empty. The symptom is since the HDA driver it's talking to
> >> > hardware which is now in D3.
> >> >
> >> > We could add a mutex to nouveau to allow us to wait for the DGPU HDA
> >> > controller, but perhaps this should be solved at a higher level in the
> >> > vgaswitcheroo code; what do you think?
> >>
> >> Maybe it's a side effect for the recent effort to fix another race in
> >> the probe.  A part of them problem is that the registration is done at
> >> the very last of probing.
> >>
> >> Instead of delaying the registration, how about the patch below?
> >
> > Ping.  If this really works, I'd like to queue it for 3.8 merge, at
> > least...
> 
> Ping ack; I was trying to find time to understand another race that
> occurs with GPU probing after switching, but is separate from the
> situation before switching, here.
> 
> In the context of writing the switch, it looks like struct azx isn't
> allocated by the time azx_vs_set_state accesses it [1,2]; racing with
> azx_codec_create?

It was allocated, but it wasn't assigned properly in pci drvdata.

Below is the revised patch.  Just moved pci_set_drvdata() before
register_vga_switcheroo().  Could you retest with it?


thanks,

Takashi

---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 4bb52da..b7c482a 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -49,6 +49,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/clocksource.h>
 #include <linux/time.h>
+#include <linux/completion.h>
 
 #ifdef CONFIG_X86
 /* for snoop control */
@@ -469,6 +470,7 @@ struct azx {
 	/* locks */
 	spinlock_t reg_lock;
 	struct mutex open_mutex;
+	struct completion probe_wait;
 
 	/* streams (x num_streams) */
 	struct azx_dev *azx_dev;
@@ -2790,6 +2792,7 @@ static bool azx_vs_can_switch(struct pci_dev *pci)
 	struct snd_card *card = pci_get_drvdata(pci);
 	struct azx *chip = card->private_data;
 
+	wait_for_completion(&chip->probe_wait);
 	if (chip->init_failed)
 		return false;
 	if (chip->disabled || !chip->bus)
@@ -2851,6 +2854,9 @@ static int azx_free(struct azx *chip)
 
 	azx_notifier_unregister(chip);
 
+	chip->init_failed = 1; /* to be sure */
+	complete(&chip->probe_wait);
+
 	if (use_vga_switcheroo(chip)) {
 		if (chip->disabled && chip->bus)
 			snd_hda_unlock_devices(chip->bus);
@@ -3156,6 +3162,7 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci,
 	INIT_LIST_HEAD(&chip->pcm_list);
 	INIT_LIST_HEAD(&chip->list);
 	init_vga_switcheroo(chip);
+	init_completion(&chip->probe_wait);
 
 	chip->position_fix[0] = chip->position_fix[1] =
 		check_position_fix(chip, position_fix[dev]);
@@ -3462,17 +3469,8 @@ static int __devinit azx_probe(struct pci_dev *pci,
 	}
 #endif /* CONFIG_SND_HDA_PATCH_LOADER */
 
-	if (probe_now) {
-		err = azx_probe_continue(chip);
-		if (err < 0)
-			goto out_free;
-	}
-
 	pci_set_drvdata(pci, card);
 
-	if (pci_dev_run_wake(pci))
-		pm_runtime_put_noidle(&pci->dev);
-
 	err = register_vga_switcheroo(chip);
 	if (err < 0) {
 		snd_printk(KERN_ERR SFX
@@ -3480,11 +3478,22 @@ static int __devinit azx_probe(struct pci_dev *pci,
 		goto out_free;
 	}
 
+	if (probe_now) {
+		err = azx_probe_continue(chip);
+		if (err < 0)
+			goto out_free;
+	}
+
+	if (pci_dev_run_wake(pci))
+		pm_runtime_put_noidle(&pci->dev);
+
 	dev++;
+	complete(&chip->probe_wait);
 	return 0;
 
 out_free:
 	snd_card_free(card);
+	pci_set_drvdata(pci, NULL);
 	return err;
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/