[PATCH 5/5] ALSA: usx2y: us144mkii: do not restart the stream while stop_work runs

[Alexandre Becholey <alex@becholey.ch>]

tascam_pcm_trigger(STOP) clears the stream flags and schedules
stop_work, which kills the URBs and then sets active_urbs to 0.
START only checks active_urbs > 0 before it anchors and submits the
URBs again, and that check does not hold:

 - START sets playback_active before the check, so when it fails with
   -EAGAIN the PCM core undoes it with trigger(STOP), which schedules
   stop_work again. If the work is running, it runs once more as soon
   as it has finished, i.e. exactly when the next START passes the
   check. That stale run kills the feedback URBs START has just
   submitted, misses the playback URBs submitted behind it, and resets
   active_urbs to 0 while they are in flight.
 - The START error path decrements active_urbs for the URB that failed
   to submit, which was never counted.
 - A STOP of the other substream can schedule stop_work while START is
   still submitting, as the submission is done without tascam->lock.

The next STOP and START then anchor and submit playback URBs that are
still anchored and owned by the host controller, which corrupts the
anchor list. Retrying START after a STOP from user space, which the
-EAGAIN invites, triggers it within seconds. On torvalds/master
7e1392a42ea2:

  URB 00000000d35257ae submitted while active
  WARNING: drivers/usb/core/urb.c:387 at usb_submit_urb+0x1318/0x1630, CPU#0: repro/77

and in another run, without the WARNING first, a "list_add double add"
from usb_anchor_urb() in tascam_pcm_trigger():

  kernel BUG at lib/list_debug.c:35!
  Internal error: Oops - BUG: 00000000f2000800 [#1]  SMP

Track a queued or running stop_work in a new stop_pending flag, set
with the STOP under tascam->lock and cleared by the work under the lock
after the URBs are dead, and refuse START with -EAGAIN while it is set,
without touching the stream flags. Submit the URBs with the lock held,
so that a STOP cannot come in between, and on a failed submission stop
through the same path instead of miscounting active_urbs. Flush
stop_work instead of cancelling it on suspend, so stop_pending cannot
be left set.

The bug was found, and the reproducer and this fix were written, by AI
agents (Claude Mythos and Claude Opus models). The analysis and the
patch were reviewed by a human.

Fixes: a2a2210f2c2e ("ALSA: usb-audio: us144mkii: Implement audio playback and feedback")
Cc: stable@vger.kernel.org
Signed-off-by: Alexandre Becholey <alex@becholey.ch>
---
Notes for reviewers (not part of the commit message):

Trigger: an emulated US-144MKII on an emulated xHCI controller, QEMU
arm64, KASAN; the device only streams normally. A local program with
access to the PCM device stops (DROP) a running playback substream and
retries START in a loop while the driver answers -EAGAIN ("Cannot
start, URBs still active"), then does one more DROP/PREPARE/START. A
device can supply the STOP itself with bad feedback packets (the
stop_pcm_work path of patch 1/5); that variant was seen in our tests
of 1/5 over USB/IP but not reproduced on its own. The reproducer is
available to maintainers on request.

Decoded, torvalds/master 7e1392a42ea2:

  WARNING: drivers/usb/core/urb.c:387 at usb_submit_urb+0x1318/0x1630, CPU#0: repro/77
  Call trace:
   usb_submit_urb (drivers/usb/core/urb.c:387 (discriminator 12)) (P)
   tascam_pcm_trigger (sound/usb/usx2y/us144mkii_pcm.c:320 (discriminator 1))
   snd_pcm_do_start (sound/core/pcm_native.c:1530 sound/core/pcm_native.c:1523)
   snd_pcm_action_single (sound/core/pcm_native.c:1384)
   snd_pcm_action (sound/core/pcm_native.c:1467)
   snd_pcm_common_ioctl (sound/core/pcm_native.c:1480 sound/core/pcm_native.c:1584 sound/core/pcm_native.c:3526)
   snd_pcm_ioctl (sound/core/pcm_native.c:3591)

The list_add case is usb_anchor_urb() (drivers/usb/core/urb.c:132)
from tascam_pcm_trigger() (us144mkii_pcm.c:318). On v7.3.1 the same
program shows the WARNING at urb.c:379, and runs that continue past it
end in "list_del corruption" in __usb_unanchor_urb().

Testing, KASAN, panic_on_warn: without the patch the program hit the
WARNING or the list corruption in every run (v7.3.1 1/1 in this test;
16/16 in the verification on v7.3.1, 7e1392a42ea2 and v7.3.1 with
patches 1/5..3/5). With the whole series applied: no report in 18
runs (v7.3.1 14, 97d73c825122 4), 593 to 1114 STOP/START cycles per
run, with the new stop_pending check refusing a START in about a
third of the cycles of the main program. A control run that waits
200 ms after each DROP streams normally: 187 of 187 STARTs succeed.

Not changed here (pre-existing): the .open callbacks clear the
*_active flags without stopping anything, and PAUSE_RELEASE is not
handled although SNDRV_PCM_INFO_PAUSE is advertised; source reading
only, not tested.

Thanks,
Alexandre Becholey

 sound/usb/usx2y/us144mkii.c     |   5 +-
 sound/usb/usx2y/us144mkii.h     |   2 +
 sound/usb/usx2y/us144mkii_pcm.c | 148 ++++++++++++++++----------------
 3 files changed, 78 insertions(+), 77 deletions(-)

diff --git a/sound/usb/usx2y/us144mkii.c b/sound/usb/usx2y/us144mkii.c
index 58ef23146f20..8270e09281cb 100644
--- a/sound/usb/usx2y/us144mkii.c
+++ b/sound/usb/usx2y/us144mkii.c
@@ -266,7 +266,10 @@ void tascam_stop_work_handler(struct work_struct *work)
 	usb_kill_anchored_urbs(&tascam->playback_anchor);
 	usb_kill_anchored_urbs(&tascam->feedback_anchor);
 	usb_kill_anchored_urbs(&tascam->capture_anchor);
+
+	guard(spinlock_irqsave)(&tascam->lock);
 	atomic_set(&tascam->active_urbs, 0);
+	tascam->stop_pending = false;
 }
 
 /*
@@ -311,7 +314,7 @@ static int tascam_suspend(struct usb_interface *intf, pm_message_t message)
 
 	snd_pcm_suspend_all(tascam->pcm);
 
-	cancel_work_sync(&tascam->stop_work);
+	flush_work(&tascam->stop_work);
 	cancel_work_sync(&tascam->capture_work);
 	cancel_work_sync(&tascam->midi_in_work);
 	cancel_work_sync(&tascam->midi_out_work);
diff --git a/sound/usb/usx2y/us144mkii.h b/sound/usb/usx2y/us144mkii.h
index 95c4341f038a..852551e0b5d0 100644
--- a/sound/usb/usx2y/us144mkii.h
+++ b/sound/usb/usx2y/us144mkii.h
@@ -174,6 +174,7 @@ struct us144mkii_frame_pattern_observer {
  *
  * @lock: Main spinlock for protecting shared driver state.
  * @active_urbs: Atomic counter for active URBs.
+ * @stop_pending: stop_work is queued or running; protected by @lock.
  * @current_rate: Currently configured sample rate of the device.
  * @line_out_source: Source for Line Outputs (0: Playback 1-2, 1: Playback 3-4).
  * @digital_out_source: Source for Digital Outputs (0: Playback 1-2, 1: Playback
@@ -232,6 +233,7 @@ struct tascam_card {
 	atomic_t playback_active;
 	atomic_t capture_active;
 	atomic_t active_urbs;
+	bool stop_pending;
 	int current_rate;
 
 	/* --- Playback State --- */
diff --git a/sound/usb/usx2y/us144mkii_pcm.c b/sound/usb/usx2y/us144mkii_pcm.c
index 5add9c1b253a..0890884a55eb 100644
--- a/sound/usb/usx2y/us144mkii_pcm.c
+++ b/sound/usb/usx2y/us144mkii_pcm.c
@@ -278,93 +278,89 @@ int tascam_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	struct tascam_card *tascam = snd_pcm_substream_chip(substream);
 	int err = 0;
 	int i;
-	bool do_start = false;
-	bool do_stop = false;
-
-	scoped_guard(spinlock_irqsave, &tascam->lock) {
-		switch (cmd) {
-		case SNDRV_PCM_TRIGGER_START:
-		case SNDRV_PCM_TRIGGER_RESUME:
-			if (!atomic_read(&tascam->playback_active)) {
-				atomic_set(&tascam->playback_active, 1);
-				atomic_set(&tascam->capture_active, 1);
-				do_start = true;
-			}
-			break;
-		case SNDRV_PCM_TRIGGER_STOP:
-		case SNDRV_PCM_TRIGGER_SUSPEND:
-		case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-			if (atomic_read(&tascam->playback_active)) {
-				atomic_set(&tascam->playback_active, 0);
-				atomic_set(&tascam->capture_active, 0);
-				do_stop = true;
-			}
-			break;
-		default:
-			err = -EINVAL;
-			break;
-		}
-	}
 
-	if (do_start) {
-		if (atomic_read(&tascam->active_urbs) > 0) {
+	/*
+	 * Hold the lock while submitting, so that a STOP of the other
+	 * substream cannot schedule stop_work in the middle of a START.
+	 */
+	guard(spinlock_irqsave)(&tascam->lock);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+		if (atomic_read(&tascam->playback_active))
+			return 0;
+		/*
+		 * stop_work kills the URBs of the previous run asynchronously;
+		 * they must not be anchored and submitted again before it is
+		 * done.
+		 */
+		if (tascam->stop_pending ||
+		    atomic_read(&tascam->active_urbs) > 0) {
 			dev_warn(tascam->card->dev,
 				 "Cannot start, URBs still active.\n");
 			return -EAGAIN;
 		}
+		atomic_set(&tascam->playback_active, 1);
+		atomic_set(&tascam->capture_active, 1);
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		if (!atomic_read(&tascam->playback_active))
+			return 0;
+		goto stop;
+	default:
+		return -EINVAL;
+	}
 
-		for (i = 0; i < NUM_FEEDBACK_URBS; i++) {
-			usb_get_urb(tascam->feedback_urbs[i]);
-			usb_anchor_urb(tascam->feedback_urbs[i],
-					       &tascam->feedback_anchor);
-			err = usb_submit_urb(tascam->feedback_urbs[i],
-						     GFP_ATOMIC);
-			if (err < 0) {
-				usb_unanchor_urb(tascam->feedback_urbs[i]);
-				usb_put_urb(tascam->feedback_urbs[i]);
-				atomic_dec(&tascam->active_urbs);
-				goto start_rollback;
-			}
-			atomic_inc(&tascam->active_urbs);
+	for (i = 0; i < NUM_FEEDBACK_URBS; i++) {
+		usb_get_urb(tascam->feedback_urbs[i]);
+		usb_anchor_urb(tascam->feedback_urbs[i],
+			       &tascam->feedback_anchor);
+		err = usb_submit_urb(tascam->feedback_urbs[i], GFP_ATOMIC);
+		if (err < 0) {
+			usb_unanchor_urb(tascam->feedback_urbs[i]);
+			usb_put_urb(tascam->feedback_urbs[i]);
+			goto start_rollback;
 		}
-		for (i = 0; i < NUM_PLAYBACK_URBS; i++) {
-			usb_get_urb(tascam->playback_urbs[i]);
-			usb_anchor_urb(tascam->playback_urbs[i],
-					       &tascam->playback_anchor);
-			err = usb_submit_urb(tascam->playback_urbs[i],
-						     GFP_ATOMIC);
-			if (err < 0) {
-				usb_unanchor_urb(tascam->playback_urbs[i]);
-				usb_put_urb(tascam->playback_urbs[i]);
-				atomic_dec(&tascam->active_urbs);
-				goto start_rollback;
-			}
-			atomic_inc(&tascam->active_urbs);
+		atomic_inc(&tascam->active_urbs);
+	}
+	for (i = 0; i < NUM_PLAYBACK_URBS; i++) {
+		usb_get_urb(tascam->playback_urbs[i]);
+		usb_anchor_urb(tascam->playback_urbs[i],
+			       &tascam->playback_anchor);
+		err = usb_submit_urb(tascam->playback_urbs[i], GFP_ATOMIC);
+		if (err < 0) {
+			usb_unanchor_urb(tascam->playback_urbs[i]);
+			usb_put_urb(tascam->playback_urbs[i]);
+			goto start_rollback;
 		}
-		for (i = 0; i < NUM_CAPTURE_URBS; i++) {
-			usb_get_urb(tascam->capture_urbs[i]);
-			usb_anchor_urb(tascam->capture_urbs[i],
-				       &tascam->capture_anchor);
-			err = usb_submit_urb(tascam->capture_urbs[i],
-					     GFP_ATOMIC);
-			if (err < 0) {
-				usb_unanchor_urb(tascam->capture_urbs[i]);
-				usb_put_urb(tascam->capture_urbs[i]);
-				atomic_dec(&tascam->active_urbs);
-				goto start_rollback;
-			}
-			atomic_inc(&tascam->active_urbs);
+		atomic_inc(&tascam->active_urbs);
+	}
+	for (i = 0; i < NUM_CAPTURE_URBS; i++) {
+		usb_get_urb(tascam->capture_urbs[i]);
+		usb_anchor_urb(tascam->capture_urbs[i],
+			       &tascam->capture_anchor);
+		err = usb_submit_urb(tascam->capture_urbs[i], GFP_ATOMIC);
+		if (err < 0) {
+			usb_unanchor_urb(tascam->capture_urbs[i]);
+			usb_put_urb(tascam->capture_urbs[i]);
+			goto start_rollback;
 		}
-
-		return 0;
-start_rollback:
-		dev_err(tascam->card->dev,
-			"Failed to submit URBs to start stream: %d\n", err);
-		do_stop = true;
+		atomic_inc(&tascam->active_urbs);
 	}
 
-	if (do_stop)
-		schedule_work(&tascam->stop_work);
+	return 0;
+
+start_rollback:
+	dev_err(tascam->card->dev,
+		"Failed to submit URBs to start stream: %d\n", err);
+stop:
+	atomic_set(&tascam->playback_active, 0);
+	atomic_set(&tascam->capture_active, 0);
+	tascam->stop_pending = true;
+	schedule_work(&tascam->stop_work);
 
 	return err;
 }