linux-kernel - Re: sound: deadlock involving snd_hrtimer

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]

Message-ID: <s5hpotgqbs5.wl-tiwai@suse.de>
Date:	Sat, 23 Apr 2016 23:02:18 +0200
From:	Takashi Iwai <tiwai@...e.de>
To:	Dmitry Vyukov <dvyukov@...gle.com>
Cc:	Jaroslav Kysela <perex@...ex.cz>, alsa-devel@...a-project.org,
	LKML <linux-kernel@...r.kernel.org>,
	Alexander Potapenko <glider@...gle.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	syzkaller <syzkaller@...glegroups.com>
Subject: Re: sound: deadlock involving snd_hrtimer_callback

On Sat, 23 Apr 2016 15:40:21 +0200,
Dmitry Vyukov wrote:
> 
> Hi Takashi,
> 
> I've incorporated your hrtimer fixes (but also updated to
> ddce192106e4f984123884f8e878f66ace94b573) and now I am seeing lots of
> the following deadlock messages:
> 
> 
> [ INFO: possible circular locking dependency detected ]
> 4.6.0-rc4+ #351 Not tainted
> -------------------------------------------------------
> swapper/0/0 is trying to acquire lock:
>  (&(&timer->lock)->rlock){-.-...}, at: [<ffffffff8537a749>]
> snd_timer_interrupt+0xa9/0xd30 sound/core/timer.c:701
> 
> but task is already holding lock:
>  (&(&stime->lock)->rlock){-.....}, at: [<ffffffff85383d3f>]
> snd_hrtimer_callback+0x4f/0x2b0 sound/core/hrtimer.c:54
> 
> which lock already depends on the new lock.

Oh crap, my second patch is buggy, it leads to ABBA lock, indeed.
The first patch is still OK, as it just adds a new behavior mode.

Could you replace the second patch with the below one?


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@...e.de>
Subject: [PATCH] ALSA: hrtimer: Use manual start/stop in callback (v2)

With the new SNDRV_TIMER_HW_RET_CTRL flag, hrtimer can manage the
callback behavior more correctly.  Now it gets a return value from
snd_timer_interrupt() whether to reprogram or stop the timer, and it
can choose the right return value.

Thanks to this change, we can revert the long-standing hack to work
around the hrtimer_cancel() call at stopping.  Since the stop is
called only explicitly outside the callback, hrtimer_cancel() can be
called safely now.

This fix essentially reverts the old commit [fcfdebe70759: ALSA:
hrtimer - Fix lock-up] while converting to use the new framework with
SNDRV_TIMER_HW_RET_CTRL.

Signed-off-by: Takashi Iwai <tiwai@...e.de>
---
 sound/core/hrtimer.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/sound/core/hrtimer.c b/sound/core/hrtimer.c
index 656d9a9032dc..27e2493c94e4 100644
--- a/sound/core/hrtimer.c
+++ b/sound/core/hrtimer.c
@@ -38,7 +38,6 @@ static unsigned int resolution;
 struct snd_hrtimer {
 	struct snd_timer *timer;
 	struct hrtimer hrt;
-	atomic_t running;
 };
 
 static enum hrtimer_restart snd_hrtimer_callback(struct hrtimer *hrt)
@@ -46,14 +45,11 @@ static enum hrtimer_restart snd_hrtimer_callback(struct hrtimer *hrt)
 	struct snd_hrtimer *stime = container_of(hrt, struct snd_hrtimer, hrt);
 	struct snd_timer *t = stime->timer;
 	unsigned long oruns;
-
-	if (!atomic_read(&stime->running))
-		return HRTIMER_NORESTART;
+	int ret;
 
 	oruns = hrtimer_forward_now(hrt, ns_to_ktime(t->sticks * resolution));
-	snd_timer_interrupt(stime->timer, t->sticks * oruns);
-
-	if (!atomic_read(&stime->running))
+	ret = snd_timer_interrupt(stime->timer, t->sticks * oruns);
+	if (ret == SNDRV_TIMER_RET_STOP)
 		return HRTIMER_NORESTART;
 	return HRTIMER_RESTART;
 }
@@ -68,7 +64,6 @@ static int snd_hrtimer_open(struct snd_timer *t)
 	hrtimer_init(&stime->hrt, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	stime->timer = t;
 	stime->hrt.function = snd_hrtimer_callback;
-	atomic_set(&stime->running, 0);
 	t->private_data = stime;
 	return 0;
 }
@@ -89,24 +84,22 @@ static int snd_hrtimer_start(struct snd_timer *t)
 {
 	struct snd_hrtimer *stime = t->private_data;
 
-	atomic_set(&stime->running, 0);
-	hrtimer_try_to_cancel(&stime->hrt);
 	hrtimer_start(&stime->hrt, ns_to_ktime(t->sticks * resolution),
 		      HRTIMER_MODE_REL);
-	atomic_set(&stime->running, 1);
 	return 0;
 }
 
 static int snd_hrtimer_stop(struct snd_timer *t)
 {
 	struct snd_hrtimer *stime = t->private_data;
-	atomic_set(&stime->running, 0);
-	hrtimer_try_to_cancel(&stime->hrt);
+
+	hrtimer_cancel(&stime->hrt);
 	return 0;
 }
 
 static struct snd_timer_hardware hrtimer_hw = {
-	.flags =	SNDRV_TIMER_HW_AUTO | SNDRV_TIMER_HW_TASKLET,
+	.flags =	SNDRV_TIMER_HW_AUTO | SNDRV_TIMER_HW_TASKLET |
+			SNDRV_TIMER_HW_RET_CTRL,
 	.open =		snd_hrtimer_open,
 	.close =	snd_hrtimer_close,
 	.start =	snd_hrtimer_start,
-- 
2.8.1