Re: INFO: rcu detected stall in memcpy

[Takashi Iwai <tiwai@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2731 bytes --]

On Thu, 04 Jan 2018 15:17:23 +0100,
Takashi Iwai wrote:
> 
> On Thu, 04 Jan 2018 15:01:06 +0100,
> Dmitry Vyukov wrote:
> > 
> > On Thu, Jan 4, 2018 at 1:57 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > > On Thu, 04 Jan 2018 13:08:45 +0100,
> > > Dmitry Vyukov wrote:
> > >>
> > >> On Thu, Jan 4, 2018 at 1:03 PM, syzbot
> > >> <syzbot+387f48da65cb522abfe8@syzkaller.appspotmail.com> wrote:
> > >> > Hello,
> > >> >
> > >> > syzkaller hit the following crash on
> > >> > 30a7acd573899fd8b8ac39236eff6468b195ac7d
> > >> > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
> > >> > compiler: gcc (GCC) 7.1.1 20170620
> > >> > .config is attached
> > >> > Raw console output is attached.
> > >> > Unfortunately, I don't have any reproducer for this bug yet.
> > >> >
> > >> >
> > >> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > >> > Reported-by: syzbot+387f48da65cb522abfe8@syzkaller.appspotmail.com
> > >> > It will help syzbot understand when the bug is fixed. See footer for
> > >> > details.
> > >> > If you forward the report, please keep this part and the footer.
> > >>
> > >> This looks ALSA-related. +ALSA maintainers.
> > >
> > > Not sure exactly what triggers it.  It's the simple memcpy(), and I
> > > don't know where RCU is involved in that code path.
> > >
> > > BTW, other two suspicious RCU usage reports are actually stopped at
> > > the second WARN_ON() after the RCU message, and the second WARN_ON()
> > > is independent from RCU; it's the known spurious WARN_ON() and was
> > > already removed in the sound git tree.
> > 
> > 
> > Hi Takashi,
> > 
> > Another similar one just popped up:
> > 
> > https://groups.google.com/forum/#!topic/syzkaller-bugs/X3d6-PIrJM0
> > 
> > This looks like mulaw_decode enters an infinite loop, or at least
> > doing very large amount of computations without a resched, e.g.
> > (uint64_t)-1 number of iterations of something along these lines.
> 
> OK, that makes sense.
> 
> My rough guess is that it's the misconfigured aloop device by
> concurrent setup.  The aloop device allows to restrict the parameters
> of the other side of the connection, and something bad may happen
> there if both sides are updated concurrently.
> 
> We've seen segfault by memset() at loopback_preapre() in
> sound/drivers/aloop.c by syzbot+3902b5220e8ca27889ca, too, which
> indicates also the wrongly setup parameters that overflows the
> allocated buffer.

Below two patches may possibly plug the holes, but I'm not entirely
sure whether that's the exact culprit.  Could you put them into syzbot
to watch whether they have any influence?

In anyway, they are obvious bugs to be fixed, so I'm going to queue to
my tree.


thanks,

Takashi


[-- Attachment #2: 0001-ALSA-pcm-Add-missing-error-checks-in-OSS-emulation-p.patch --]
[-- Type: application/octet-stream, Size: 1676 bytes --]

>From 6708913750344a900f2e73bfe4a4d6dbbce4fe8d Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Thu, 4 Jan 2018 16:39:27 +0100
Subject: [PATCH 1/2] ALSA: pcm: Add missing error checks in OSS emulation
 plugin builder

In the OSS emulation plugin builder where the frame size is parsed in
the plugin chain, some places miss the possible errors returned from
the plugin src_ or dst_frames callback.

This patch papers over such places.

Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/oss/pcm_plugin.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/sound/core/oss/pcm_plugin.c b/sound/core/oss/pcm_plugin.c
index cadc93792868..85a56af104bd 100644
--- a/sound/core/oss/pcm_plugin.c
+++ b/sound/core/oss/pcm_plugin.c
@@ -592,18 +592,26 @@ snd_pcm_sframes_t snd_pcm_plug_write_transfer(struct snd_pcm_substream *plug, st
 	snd_pcm_sframes_t frames = size;
 
 	plugin = snd_pcm_plug_first(plug);
-	while (plugin && frames > 0) {
+	while (plugin) {
+		if (frames <= 0)
+			return frames;
 		if ((next = plugin->next) != NULL) {
 			snd_pcm_sframes_t frames1 = frames;
-			if (plugin->dst_frames)
+			if (plugin->dst_frames) {
 				frames1 = plugin->dst_frames(plugin, frames);
+				if (frames1 <= 0)
+					return frames1;
+			}
 			if ((err = next->client_channels(next, frames1, &dst_channels)) < 0) {
 				return err;
 			}
 			if (err != frames1) {
 				frames = err;
-				if (plugin->src_frames)
+				if (plugin->src_frames) {
 					frames = plugin->src_frames(plugin, frames1);
+					if (frames <= 0)
+						return frames;
+				}
 			}
 		} else
 			dst_channels = NULL;
-- 
2.15.1


[-- Attachment #3: 0002-ALSA-aloop-Fix-racy-hw-constraints-adjustment.patch --]
[-- Type: application/octet-stream, Size: 5748 bytes --]

>From ba043d316c911c80ec067bccff4e76e7c11619d3 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Thu, 4 Jan 2018 17:38:54 +0100
Subject: [PATCH 2/2] ALSA: aloop: Fix racy hw constraints adjustment

The aloop driver tries to update the hw constraints of the connected
target on the cable of the opened PCM substream.  This is done by
adding the extra hw constraints rules referring to the substream
runtime->hw fields, while the other substream may update the runtime
hw of another side on the fly.

This is, however, racy and may result in the inconsistent values when
both PCM streams perform the prepare concurrently.  One of the reason
is that it overwrites the other's runtime->hw field; which is not only
racy but also broken when it's called before the open of another side
finishes.  And, since the reference to runtime->hw isn't protected,
the concurrent write may give the partial value update and become
inconsistent.

This patch is an attempt to fix and clean up:
- The prepare doesn't change the runtime->hw of other side any longer,
  but only update the cable->hw that is referred commonly.
- The extra rules refer to the loopback_pcm object instead of the
  runtime->hw.  The actual hw is deduced from cable->hw.
- The extra rules take the cable_lock to protect against the race.

Fixes: b1c73fc8e697 ("ALSA: snd-aloop: Fix hw_params restrictions and checking")
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/drivers/aloop.c | 51 +++++++++++++++++++++------------------------------
 1 file changed, 21 insertions(+), 30 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index afac886ffa28..b1eee1a1e925 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -305,19 +305,6 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 	return 0;
 }
 
-static void params_change_substream(struct loopback_pcm *dpcm,
-				    struct snd_pcm_runtime *runtime)
-{
-	struct snd_pcm_runtime *dst_runtime;
-
-	if (dpcm == NULL || dpcm->substream == NULL)
-		return;
-	dst_runtime = dpcm->substream->runtime;
-	if (dst_runtime == NULL)
-		return;
-	dst_runtime->hw = dpcm->cable->hw;
-}
-
 static void params_change(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
@@ -329,10 +316,6 @@ static void params_change(struct snd_pcm_substream *substream)
 	cable->hw.rate_max = runtime->rate;
 	cable->hw.channels_min = runtime->channels;
 	cable->hw.channels_max = runtime->channels;
-	params_change_substream(cable->streams[SNDRV_PCM_STREAM_PLAYBACK],
-				runtime);
-	params_change_substream(cable->streams[SNDRV_PCM_STREAM_CAPTURE],
-				runtime);
 }
 
 static int loopback_prepare(struct snd_pcm_substream *substream)
@@ -620,12 +603,14 @@ static unsigned int get_cable_index(struct snd_pcm_substream *substream)
 static int rule_format(struct snd_pcm_hw_params *params,
 		       struct snd_pcm_hw_rule *rule)
 {
-
-	struct snd_pcm_hardware *hw = rule->private;
+	struct loopback_pcm *dpcm = rule->private;
+	struct loopback_cable *cable = dpcm->cable;
 	struct snd_mask *maskp = hw_param_mask(params, rule->var);
 
-	maskp->bits[0] &= (u_int32_t)hw->formats;
-	maskp->bits[1] &= (u_int32_t)(hw->formats >> 32);
+	mutex_lock(&dpcm->loopback->cable_lock);
+	maskp->bits[0] &= (u_int32_t)cable->hw.formats;
+	maskp->bits[1] &= (u_int32_t)(cable->hw.formats >> 32);
+	mutex_unlock(&dpcm->loopback->cable_lock);
 	memset(maskp->bits + 2, 0, (SNDRV_MASK_MAX-64) / 8); /* clear rest */
 	if (! maskp->bits[0] && ! maskp->bits[1])
 		return -EINVAL;
@@ -635,11 +620,14 @@ static int rule_format(struct snd_pcm_hw_params *params,
 static int rule_rate(struct snd_pcm_hw_params *params,
 		     struct snd_pcm_hw_rule *rule)
 {
-	struct snd_pcm_hardware *hw = rule->private;
+	struct loopback_pcm *dpcm = rule->private;
+	struct loopback_cable *cable = dpcm->cable;
 	struct snd_interval t;
 
-        t.min = hw->rate_min;
-        t.max = hw->rate_max;
+	mutex_lock(&dpcm->loopback->cable_lock);
+	t.min = cable->hw.rate_min;
+	t.max = cable->hw.rate_max;
+	mutex_unlock(&dpcm->loopback->cable_lock);
         t.openmin = t.openmax = 0;
         t.integer = 0;
 	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
@@ -648,11 +636,14 @@ static int rule_rate(struct snd_pcm_hw_params *params,
 static int rule_channels(struct snd_pcm_hw_params *params,
 			 struct snd_pcm_hw_rule *rule)
 {
-	struct snd_pcm_hardware *hw = rule->private;
+	struct loopback_pcm *dpcm = rule->private;
+	struct loopback_cable *cable = dpcm->cable;
 	struct snd_interval t;
 
-        t.min = hw->channels_min;
-        t.max = hw->channels_max;
+	mutex_lock(&dpcm->loopback->cable_lock);
+	t.min = cable->hw.channels_min;
+	t.max = cable->hw.channels_max;
+	mutex_unlock(&dpcm->loopback->cable_lock);
         t.openmin = t.openmax = 0;
         t.integer = 0;
 	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
@@ -699,19 +690,19 @@ static int loopback_open(struct snd_pcm_substream *substream)
 	/* are cached -> they do not reflect the actual state */
 	err = snd_pcm_hw_rule_add(runtime, 0,
 				  SNDRV_PCM_HW_PARAM_FORMAT,
-				  rule_format, &runtime->hw,
+				  rule_format, dpcm,
 				  SNDRV_PCM_HW_PARAM_FORMAT, -1);
 	if (err < 0)
 		goto unlock;
 	err = snd_pcm_hw_rule_add(runtime, 0,
 				  SNDRV_PCM_HW_PARAM_RATE,
-				  rule_rate, &runtime->hw,
+				  rule_rate, dpcm,
 				  SNDRV_PCM_HW_PARAM_RATE, -1);
 	if (err < 0)
 		goto unlock;
 	err = snd_pcm_hw_rule_add(runtime, 0,
 				  SNDRV_PCM_HW_PARAM_CHANNELS,
-				  rule_channels, &runtime->hw,
+				  rule_channels, dpcm,
 				  SNDRV_PCM_HW_PARAM_CHANNELS, -1);
 	if (err < 0)
 		goto unlock;
-- 
2.15.1