linux-kernel - Re: [GIT PULL] sound updates for 5.14-rc1

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:   Sat, 3 Jul 2021 15:38:48 +0900
From:   Takashi Sakamoto <o-takashi@...amocchi.jp>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Takashi Iwai <tiwai@...e.de>, alsa-devel@...a-project.org,
        Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        marcan@...can.st
Subject: Re: [GIT PULL] sound updates for 5.14-rc1

Hi,

On Fri, Jul 02, 2021 at 10:19:46PM -0700, Linus Torvalds wrote:
> On Fri, Jul 2, 2021 at 9:37 PM Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
> >
> > But I thought I'd report this as a likely candidate.
> 
> Confirmed. The watchdog hang bisects right down to commit 9ce650a75a3b
> ("ALSA: usb-audio: Reduce latency at playback start").
> 
> And reverting it on top of my tree also fixes the hang, so it's not
> some bisection fluke.
> 
> I have no idea what is actually wrong with that commit, but it most
> definitely is the problem, and I have reverted it in my tree so that I
> can continue merging stuff tomorrow.

The cause seems to be the attempt to lock PCM substream recursively
introduced by the issued commit.

Would I ask you to test with below patch? I apologize that the patch is
still untested in my side since at present I have no preparation to debug
USB stuffs instantly (I'm just a maintainer for ALSA firewire stack...),
so I'm glad if getting your cooperation for the issue.

======== 8< --------

>From f7ab449f10152635ad7083aa73d80e3fb1adabb4 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@...amocchi.jp>
Date: Sat, 3 Jul 2021 15:23:25 +0900
Subject: [PATCH] ALSA: usb-audio: fix recursive lock of PCM substream when
 starting playback PCM substream

A commit 9ce650a75a3b ("ALSA: usb-audio: Reduce latency at playback start")
unfortunately introduced the call of snd_pcm_period_elapsed() under acquired
lock of PCM substream. This causes recursive lock and results in dead-lock.

->ioctl(2)
  (sound/core/pcm_native.c)
  ->snd_pcm_stream_lock_irqsave() <-
    ...
    ->struct snd_pcm_ops.trigger()
    (sound/usb/pcm.c)
    = snd_usb_substream_playback_trigger()
      ->start_endpoints()
        (sound/usb/endpoint.c)
        ->snd_usb_endpoint_start()
          ->prepare_outbound_urb()
            ->struct snd_usb_endpoint.prepare_data_urb()
            (sound/usb/pcm.c)
            = prepare_playback_urb()
              (sound/core/pcm_lib.c)
              ->snd_pcm_period_elapsed()
                (sound/core/pcm_native.c)
                ->snd_pcm_stream_lock_irqsave() <-

This commit fixes the issue to use newly added function;
snd_pcm_period_elapsed_under_stream_lock() with condition to check running
context.

Reported-by: Hector Martin <marcan@...can.st>
Fixes: 9ce650a75a3b ("ALSA: usb-audio: Reduce latency at playback start")
Signed-off-by: Takashi Sakamoto <o-takashi@...amocchi.jp>
---
 sound/usb/pcm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index c66831ee15f9..235070f0236a 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -1395,8 +1395,16 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 
 	spin_unlock_irqrestore(&subs->lock, flags);
 	urb->transfer_buffer_length = bytes;
-	if (period_elapsed)
-		snd_pcm_period_elapsed(subs->pcm_substream);
+	if (period_elapsed) {
+		// The callback of struct snd_pcm_ops.trigger with SNDRV_PCM_TRIGGER_START command
+		// can reach here, under acquired lock of PCM substream. To avoid dead-lock, check
+		// current context and call corresponding function.
+		if (in_softirq()) {
+			snd_pcm_period_elapsed(subs->pcm_substream);
+		} else {
+			snd_pcm_period_elapsed_under_stream_lock(subs->pcm_substream);
+		}
+	}
 }
 
 /*
-- 
2.30.2

======== 8< --------

Thanks

Takashi (not subsystem maintainer) Sakamoto