[PATCH 0/5] ALSA: usx2y: us144mkii: fix PCM stream lifetime bugs

[PATCH 0/5] ALSA: usx2y: us144mkii: fix PCM stream lifetime bugs

[Alexandre Becholey]

Hi,

Five bugs in the snd-usb-us144mkii PCM code, all reproduced with an
emulated US-144MKII under KASAN on torvalds/master and v7.3.1:

 1/5 stop_pcm_work calls snd_pcm_stop() on a substream that is being
     closed (use-after-free of the runtime); the device can schedule
     the work at any time by sending bad feedback.
 2/5 capture_work keeps using the capture runtime after the capture
     PCM is closed while playback is running (use-after-free).
 3/5 the capture work and the playback URB completion use the buffer
     of a substream that was never started (NULL dma_area), because
     the URBs of both directions always run together.
 4/5 the playback and feedback URB completions use the substream
     runtime and buffer without waiting for close (use-after-free).
 5/5 a START retried while stop_work runs submits URBs that are still
     in flight (URB submitted while active, anchor list corruption).

The patches depend on each other and are meant to be applied in this
order: 2/5 builds on the close callback of 1/5, the check added in 3/5
is only safe once 2/5 guarantees that the capture work does not outlive
the substream, and 4/5 and 5/5 apply on top of the first three. In
testing, 3/5 without 2/5 crashed in the new check, and 1/5 + 2/5
without 3/5 still crashed on the NULL dma_area. Each patch has its
testing notes below its '---' line.

Hanxiang Xu reported the bugs fixed by 1/5, 2/5 and 4/5 on this list
on 2026-11-13, from source inspection and without a patch; we found
them independently with emulated devices. This series is sent as a
reply to that report, and 1/5, 2/5 and 4/5 carry Closes: tags for the
two messages. The reports also mention close racing with disconnect:
that concerns a fix that kills URBs in .close, which this series does
not do (the .close callbacks only touch the card private data), so
nothing is changed for it.

How we found it: the bugs were found, and the reproducers and these
patches were written, by AI agents (Claude Mythos and Claude Opus
models), in QEMU with emulated USB devices (raw-gadget/dummy_hcd,
emulated xHCI, USB/IP).
A human reviewed the analysis and the patches. The reproducers are
available on request.

Thanks,
Alexandre Becholey

Alexandre Becholey (5):
  ALSA: usx2y: us144mkii: fix stop_pcm_work race with PCM close
  ALSA: usx2y: us144mkii: wait for the capture work on capture close
  ALSA: usx2y: us144mkii: only touch the buffer of a running PCM stream
  ALSA: usx2y: us144mkii: wait for the URB completion handlers on stop and close
  ALSA: usx2y: us144mkii: do not restart the stream while stop_work runs

 sound/usb/usx2y/us144mkii.c          |   5 +-
 sound/usb/usx2y/us144mkii.h          |   2 +
 sound/usb/usx2y/us144mkii_capture.c  |  22 ++++-
 sound/usb/usx2y/us144mkii_pcm.c      | 162 +++++++++++++++++++----------------
 sound/usb/usx2y/us144mkii_pcm.h      |   8 ++
 sound/usb/usx2y/us144mkii_playback.c |  65 ++++++++++----
 6 files changed, 170 insertions(+), 94 deletions(-)

[PATCH 1/5] ALSA: usx2y: us144mkii: fix stop_pcm_work race with PCM close

[Alexandre Becholey]

The feedback URB completion handler schedules stop_pcm_work when the
device stops sending valid feedback. The work handler then calls
snd_pcm_stop() on tascam->playback_substream and
tascam->capture_substream without the stream lock, and nothing keeps
these substreams open while it runs: the PCM .close callbacks only clear
the pointers and do not wait for the work.

If the PCM is closed while the work is running, snd_pcm_detach_substream()
frees the runtime under it, and the work then accesses the freed runtime
in snd_pcm_post_stop(), or dereferences a NULL substream->runtime in
snd_pcm_do_stop(), depending on how far the close has got. A USB device
can fire the work whenever it wants by sending bad feedback packets, so
a malicious device plus a process closing the PCM can trigger this.
With KASAN, on torvalds/master 067c5c24cef2:

  BUG: KASAN: slab-use-after-free in _raw_spin_lock_irqsave+0xb0/0x104
  Workqueue: events tascam_stop_pcm_work_handler

Clear the substream pointers under tascam->lock in the .close callbacks
and flush stop_pcm_work afterwards, so the work can no longer run on a
closing substream. In the work, read the pointers under the same lock
and stop the streams with snd_pcm_stop_xrun(), which takes the stream
lock and checks that the stream is still running. The lock is dropped
before snd_pcm_stop_xrun() because the trigger callback takes
tascam->lock with the stream lock held.

Found with an emulated US-144MKII device (USB/IP, vhci_hcd).

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.

Reported-by: Hanxiang Xu <xuhx@hust.edu.cn>
Closes: https://lore.kernel.org/all/6891aa7d.26365.1a1d4e2c4e4.Coremail.xuhx@hust.edu.cn/
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 TASCAM US-144MKII served over USB/IP to vhci_hcd
(an AF_UNIX socketpair inside the VM; dummy_hcd cannot do isochronous
transfers) under QEMU, arm64, KASAN. The program opens a PCM and
starts streaming; the device sends invalid feedback packets until the
driver logs "Fatal: Feedback sync lost. Stopping stream." and schedules
stop_pcm_work, while the program closes the PCM. The reproducer is available to maintainers on request.

Decoded trace, torvalds/master 067c5c24cef2 (arm64, QEMU virt); the
object is the kmalloc-2k snd_pcm_runtime, allocated by
snd_pcm_attach_substream() on open and freed by
snd_pcm_detach_substream() from snd_pcm_release() on close:

  BUG: KASAN: slab-use-after-free in _raw_spin_lock_irqsave (include/linux/spinlock_api_smp.h:155 (discriminator 1) kernel/locking/spinlock.c:181 (discriminator 1))
  Read of size 1 at addr ffff00001318a180 by task kworker/1:0/24
  CPU: 1 UID: 0 PID: 24 Comm: kworker/1:0 Not tainted 7.4.0-rc2+ #1 PREEMPT(full)
  Hardware name: linux,dummy-virt (DT)
  Workqueue: events tascam_stop_pcm_work_handler
  Call trace:
   show_stack (arch/arm64/kernel/stacktrace.c:498 arch/arm64/kernel/stacktrace.c:499) (C)
   dump_stack_lvl (lib/dump_stack.c:94 lib/dump_stack.c:120)
   print_report (mm/kasan/report.c:378 mm/kasan/report.c:482)
   kasan_report (mm/kasan/report.c:595)
   __kasan_check_byte (mm/kasan/common.c:585)
   lock_acquire (include/linux/kasan.h:402 (discriminator 1) kernel/locking/lockdep.c:5916 (discriminator 1) kernel/locking/lockdep.c:5899 (discriminator 1))
   _raw_spin_lock_irqsave (include/linux/spinlock_api_smp.h:155 (discriminator 1) kernel/locking/spinlock.c:181 (discriminator 1))
   __wake_up (kernel/sched/wait.c:124 (discriminator 3) kernel/sched/wait.c:146 (discriminator 3))
   snd_pcm_post_stop (sound/core/pcm_native.c:1622)
   snd_pcm_action_single (sound/core/pcm_native.c:1386)
   snd_pcm_action (sound/core/pcm_native.c:1467)
   snd_pcm_stop (sound/core/pcm_native.c:1642)
   tascam_stop_pcm_work_handler (sound/usb/usx2y/us144mkii_playback.c:453)
   process_one_work (kernel/workqueue.c:3495)
   worker_thread (kernel/workqueue.c:3578 kernel/workqueue.c:3659)
   kthread (kernel/kthread.c:436)
   ret_from_fork (arch/arm64/kernel/entry.S:870)

On v7.3.1 the same reproducer also ends in
"KASAN: null-ptr-deref in range [0x1c0-0x1c7]" in snd_pcm_do_stop()
called from tascam_stop_pcm_work_handler() (us144mkii_playback.c:453),
the snd_pcm_stop() call this patch replaces.

sound/usb/usx2y/us144mkii*.c are identical in v7.3.1, 067c5c24cef2
and 9d66f26fefac.

Testing, KASAN: without the patch every run crashed in
tascam_stop_pcm_work_handler (v7.3.1 3/3, 067c5c24cef2 1/1). With the
patch no run showed this crash (v7.3.1 0/14 with three test programs,
067c5c24cef2 0/5 with one).
With patches 1/5..3/5 applied (v7.3.1, the same three test programs,
8 runs), the stop work fired 10 to 1740 times per run and no run
showed this crash; 4 of those 8 runs ended in the first two other
problems listed below. These programs were not re-run with 4/5 and
5/5 added; those patches were tested with their own reproducers.

17 of the 19 runs with this patch alone still crashed elsewhere:
- 12 runs: list_del corruption in __usb_unanchor_urb(), on v7.3.1
  after a WARNING at drivers/usb/core/urb.c:379 in the stream start
  path (that WARNING was also seen without the patch); fixed by
  patch 5/5;
- 4 runs: KASAN use-after-free in vhci_urb_dequeue() from
  usb_kill_anchored_urbs() in tascam_stop_work_handler(), a race in
  the vhci_hcd test harness, not in this driver;
- 1 run: KASAN slab-use-after-free in feedback_urb_complete()
  (us144mkii_playback.c:347, reading playback_rt->rate); fixed by
  patch 4/5.

Thanks,
Alexandre Becholey

 sound/usb/usx2y/us144mkii_capture.c  |  5 ++++-
 sound/usb/usx2y/us144mkii_playback.c | 19 ++++++++++++++-----
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/sound/usb/usx2y/us144mkii_capture.c b/sound/usb/usx2y/us144mkii_capture.c
index d7cdf9e0f685..30cdd73ca99c 100644
--- a/sound/usb/usx2y/us144mkii_capture.c
+++ b/sound/usb/usx2y/us144mkii_capture.c
@@ -36,7 +36,10 @@ static int tascam_capture_close(struct snd_pcm_substream *substream)
 {
 	struct tascam_card *tascam = snd_pcm_substream_chip(substream);
 
-	tascam->capture_substream = NULL;
+	scoped_guard(spinlock_irqsave, &tascam->lock)
+		tascam->capture_substream = NULL;
+	/* The stop work may still be using this substream; wait for it. */
+	flush_work(&tascam->stop_pcm_work);
 
 	return 0;
 }
diff --git a/sound/usb/usx2y/us144mkii_playback.c b/sound/usb/usx2y/us144mkii_playback.c
index 7efaca0a6489..0df95dbf585d 100644
--- a/sound/usb/usx2y/us144mkii_playback.c
+++ b/sound/usb/usx2y/us144mkii_playback.c
@@ -36,7 +36,10 @@ static int tascam_playback_close(struct snd_pcm_substream *substream)
 {
 	struct tascam_card *tascam = snd_pcm_substream_chip(substream);
 
-	tascam->playback_substream = NULL;
+	scoped_guard(spinlock_irqsave, &tascam->lock)
+		tascam->playback_substream = NULL;
+	/* The stop work may still be using this substream; wait for it. */
+	flush_work(&tascam->stop_pcm_work);
 
 	return 0;
 }
@@ -448,9 +451,15 @@ void tascam_stop_pcm_work_handler(struct work_struct *work)
 {
 	struct tascam_card *tascam =
 		container_of(work, struct tascam_card, stop_pcm_work);
+	struct snd_pcm_substream *playback_ss, *capture_ss;
+
+	scoped_guard(spinlock_irqsave, &tascam->lock) {
+		playback_ss = tascam->playback_substream;
+		capture_ss = tascam->capture_substream;
+	}
 
-	if (tascam->playback_substream)
-		snd_pcm_stop(tascam->playback_substream, SNDRV_PCM_STATE_XRUN);
-	if (tascam->capture_substream)
-		snd_pcm_stop(tascam->capture_substream, SNDRV_PCM_STATE_XRUN);
+	if (playback_ss)
+		snd_pcm_stop_xrun(playback_ss);
+	if (capture_ss)
+		snd_pcm_stop_xrun(capture_ss);
 }

[PATCH 2/5] ALSA: usx2y: us144mkii: wait for the capture work on capture close

[Alexandre Becholey]

The capture URBs are started and stopped together with the playback
stream by the shared trigger callback, and every completed capture URB
schedules capture_work. tascam_capture_work_handler() reads
tascam->capture_substream and its runtime once and then keeps writing
into runtime->dma_area, using runtime->frame_bits and
runtime->buffer_size, for as long as capture_active is set and the
capture ring has data.

tascam_capture_close() only clears tascam->capture_substream and does
not wait for the work. When the capture PCM was opened and prepared but
not started while the playback stream is running, closing it does not
stop anything: capture_active stays set and the URBs keep completing.
The runtime is freed by snd_pcm_detach_substream() right after the close
callback returns, while a running work instance still uses it. With
KASAN, on torvalds/master 9d66f26fefac:

  BUG: KASAN: slab-use-after-free in tascam_capture_work_handler (sound/usb/usx2y/us144mkii_capture.c:230)
  Read of size 8 at addr ffff000013270458 by task kworker/0:1/11
  Workqueue: events tascam_capture_work_handler
  ...
  Freed by task 79:
   kfree
   snd_pcm_detach_substream
   snd_pcm_release_substream.part.0
   snd_pcm_release
   __fput

A USB device that keeps the capture endpoint busy keeps the work loop
running, which makes the window wide.

Cancel the capture work in the close callback, after the substream
pointer has been cleared under tascam->lock (patch 1/5), so that no
work instance can still use the runtime when it is freed. Instances
queued later see the cleared pointer and return early.

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.

Reported-by: Hanxiang Xu <xuhx@hust.edu.cn>
Closes: https://lore.kernel.org/all/12a3a26a.26361.1a1d4e1ad57.Coremail.xuhx@hust.edu.cn/
Fixes: c1bb0c13e430 ("ALSA: usb-audio: us144mkii: Implement audio capture and decoding")
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 (raw-gadget + dummy_hcd, QEMU arm64,
KASAN, 2 CPUs) that streams capture data. In each of 400 iterations
the program opens the capture PCM and prepares it without starting
it, starts the playback PCM, and closes the capture PCM. A variant
also stops playback before the next capture open, so that only this
race is left. The reproducers are available to maintainers on request.

The context of this patch is the close hunk of patch 1/5. Patch 3/5
also depends on it: with 3/5 alone, a capture work instance still
running after close dereferences the detached runtime in the new
snd_pcm_running() check (seen in testing, KASAN null-ptr-deref at
include/sound/pcm.h:711 from us144mkii_capture.c:231 on v7.3.1).

Testing, KASAN: without the series the close/reopen program crashed as
above on the first attempt (v7.3.1 1/1, 9d66f26fefac 1/1). With
patches 1/5..3/5 applied: v7.3.1 0/15 runs (both programs), 9d66f26fefac
0/5 runs, each run completing all 400 iterations.

Not changed here: feedback_urb_complete() also reads
tascam->capture_substream->runtime before taking tascam->lock and uses
capture_rt->period_size under it; patch 4/5 fixes that.

Thanks,
Alexandre Becholey

 sound/usb/usx2y/us144mkii_capture.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/usb/usx2y/us144mkii_capture.c b/sound/usb/usx2y/us144mkii_capture.c
index 30cdd73ca99c..973bed60bfcd 100644
--- a/sound/usb/usx2y/us144mkii_capture.c
+++ b/sound/usb/usx2y/us144mkii_capture.c
@@ -40,6 +40,12 @@ static int tascam_capture_close(struct snd_pcm_substream *substream)
 		tascam->capture_substream = NULL;
 	/* The stop work may still be using this substream; wait for it. */
 	flush_work(&tascam->stop_pcm_work);
+	/*
+	 * The capture work keeps a pointer to the runtime, which is freed
+	 * when this callback returns. The capture URBs (and so the work) keep
+	 * running as long as the playback stream does, so wait for it here.
+	 */
+	cancel_work_sync(&tascam->capture_work);
 
 	return 0;
 }

[PATCH 3/5] ALSA: usx2y: us144mkii: only touch the buffer of a running PCM stream

[Alexandre Becholey]

The playback and capture URBs of the US-144MKII always run together:
tascam_pcm_trigger() sets both playback_active and capture_active and
submits all URBs when either substream is started. The capture work
handler and the playback URB completion handler, however, use the
buffer of their own substream as soon as the corresponding *_active
flag is set and the substream is open, without checking that this
substream is actually running.

So if only one direction is started, the handler of the other direction
works on a substream that may have no buffer at all:

 - capture PCM opened without HW_PARAMS, playback started:
   tascam_capture_work_handler() writes the captured frames through the
   NULL runtime->dma_area, and divides by the zero buffer_size.

 - playback PCM prepared but not started (or opened without HW_PARAMS),
   capture started, then HW_FREE on the playback PCM, which ALSA allows
   in the PREPARED state: playback_urb_complete() copies from the NULL
   runtime->dma_area.

With KASAN, on torvalds/master 9d66f26fefac:

  BUG: KASAN: null-ptr-deref in tascam_capture_work_handler
  Write of size 3 at addr 0000000000000000 by task kworker/0:2/62

  BUG: KASAN: null-ptr-deref in playback_urb_complete
  Read of size 480 at addr 0000000000000660 by task ksoftirqd/0/15

Both only need an attached device and this order of PCM ioctls from a
user with access to the PCM devices.

Only copy to or from the substream buffer while that substream is
running. The capture data is then dropped, and the playback URBs carry
silence, as the device keeps streaming in both directions.

The bugs were found, and the reproducers 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")
Fixes: c1bb0c13e430 ("ALSA: usb-audio: us144mkii: Implement audio capture and decoding")
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 under QEMU arm64 with KASAN; the
capture case on dummy_hcd (raw-gadget), the playback case on an
emulated xHCI controller, because the playback URBs are isochronous.
The reproducers are available to maintainers on request.

Decoded, torvalds/master 9d66f26fefac:
  tascam_capture_work_handler (sound/usb/usx2y/us144mkii_capture.c:246)
  playback_urb_complete (sound/usb/usx2y/us144mkii_playback.c:230)

This patch depends on patch 2/5: the new snd_pcm_running() check reads
substream->runtime, so a capture work instance must not outlive the
close of its substream. With this patch alone, the close/reopen
reproducer of 2/5 crashed in the check (KASAN null-ptr-deref at
include/sound/pcm.h:711 from us144mkii_capture.c:231, v7.3.1).

Testing, KASAN: without the series both cases crashed on the first
attempt (v7.3.1 and 9d66f26fefac, 1/1 each). With patches 1/5..3/5
applied: no report in v7.3.1 15 runs (capture 10, playback 5 with two
programs) and 9d66f26fefac 5 runs (capture 3, playback 2). Normal
streaming in both directions was checked on v7.3.1 with patches
1/5..3/5 applied (2 runs): the playback and capture positions keep
advancing. The playback test of patch 5/5 (normal streaming, 187 of
187 STARTs) also ran with all five patches.

Thanks,
Alexandre Becholey

 sound/usb/usx2y/us144mkii_capture.c  |  8 +++++++-
 sound/usb/usx2y/us144mkii_playback.c | 23 ++++++++++++++++++-----
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/sound/usb/usx2y/us144mkii_capture.c b/sound/usb/usx2y/us144mkii_capture.c
index 973bed60bfcd..9c142a85e8b2 100644
--- a/sound/usb/usx2y/us144mkii_capture.c
+++ b/sound/usb/usx2y/us144mkii_capture.c
@@ -231,7 +231,13 @@ void tascam_capture_work_handler(struct work_struct *work)
 						  routed_block);
 
 		scoped_guard(spinlock_irqsave, &tascam->lock) {
-			if (atomic_read(&tascam->capture_active)) {
+			/*
+			 * capture_active is also set when only the playback
+			 * stream was started; the capture buffer is only valid
+			 * while the capture stream itself is running.
+			 */
+			if (atomic_read(&tascam->capture_active) &&
+			    snd_pcm_running(substream)) {
 				int f;
 
 				for (f = 0; f < FRAMES_PER_DECODE_BLOCK; ++f) {
diff --git a/sound/usb/usx2y/us144mkii_playback.c b/sound/usb/usx2y/us144mkii_playback.c
index 0df95dbf585d..e6266488cf32 100644
--- a/sound/usb/usx2y/us144mkii_playback.c
+++ b/sound/usb/usx2y/us144mkii_playback.c
@@ -166,6 +166,7 @@ void playback_urb_complete(struct urb *urb)
 	size_t total_bytes_for_urb = 0;
 	snd_pcm_uframes_t offset_frames;
 	snd_pcm_uframes_t frames_to_copy;
+	bool running;
 	int ret, i;
 
 	if (urb->status) {
@@ -207,13 +208,25 @@ void playback_urb_complete(struct urb *urb)
 		}
 		urb->transfer_buffer_length = total_bytes_for_urb;
 
-		offset_frames = tascam->driver_playback_pos;
-		frames_to_copy = bytes_to_frames(runtime, total_bytes_for_urb);
-		tascam->driver_playback_pos =
-			(offset_frames + frames_to_copy) % runtime->buffer_size;
+		/*
+		 * The URBs also run when only the capture stream was started;
+		 * the playback buffer is only valid while the playback stream
+		 * itself is running.  Send silence otherwise.
+		 */
+		running = snd_pcm_running(substream);
+		if (running) {
+			offset_frames = tascam->driver_playback_pos;
+			frames_to_copy = bytes_to_frames(runtime,
+							 total_bytes_for_urb);
+			tascam->driver_playback_pos =
+				(offset_frames + frames_to_copy) %
+				runtime->buffer_size;
+		}
 	}
 
-	if (total_bytes_for_urb > 0) {
+	if (!running) {
+		memset(urb->transfer_buffer, 0, total_bytes_for_urb);
+	} else if (total_bytes_for_urb > 0) {
 		u8 *dst_buf = urb->transfer_buffer;
 
 		/* Handle ring buffer wrap-around */

[PATCH 4/5] ALSA: usx2y: us144mkii: wait for the URB completion handlers on stop and close

[Alexandre Becholey]

The playback, feedback and capture URBs are started and stopped together,
so they keep running while either PCM stream is running, also when the
other substream is closed. playback_urb_complete() and
feedback_urb_complete() read tascam->playback_substream (and
capture_substream) and its runtime without holding tascam->lock, then
use runtime->rate, runtime->period_size and runtime->buffer_size under
the lock, copy from runtime->dma_area and call snd_pcm_period_elapsed()
after dropping it. Nothing waits for a completion handler that has
already loaded these pointers: the PCM .close callbacks only clear them
and the stop path only schedules stop_work.

So when a PCM is closed while the URBs are running, the managed buffer
is freed by do_hw_free() and the runtime by snd_pcm_detach_substream()
while a completion handler may still be using them. A device can make
the window wide: a feedback completion that prints "Fatal: Feedback
sync lost" does so between two uses of the playback runtime. With
KASAN, on torvalds/master 7e1392a42ea2:

  BUG: KASAN: slab-use-after-free in feedback_urb_complete+0xe64/0x1174
  Read of size 8 at addr ffff0000132ba078 by task repro/82

The object is the snd_pcm_runtime, freed by snd_pcm_detach_substream()
from snd_pcm_release().

Read the substream pointers inside an RCU read-side section in both
completion handlers and keep it until they no longer use the substream.
Call synchronize_rcu() in the .close callbacks, after the pointer was
cleared, and in a new .sync_stop callback, which the PCM core calls
after a stop before the buffer is freed or reallocated. The capture
work, which writes to the capture buffer with tascam->lock held, is
covered by the same grace period; on close it is already cancelled.

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.

Reported-by: Hanxiang Xu <xuhx@hust.edu.cn>
Closes: https://lore.kernel.org/all/12a3a26a.26361.1a1d4e1ad57.Coremail.xuhx@hust.edu.cn/
Fixes: a2a2210f2c2e ("ALSA: usb-audio: us144mkii: Implement audio playback and feedback")
Fixes: c1bb0c13e430 ("ALSA: usb-audio: us144mkii: Implement audio capture and decoding")
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 (the
URBs are isochronous), QEMU arm64, KASAN. The device completes the
probe and then sends empty feedback packets. The program starts the
capture PCM, which starts all URBs, while the playback PCM is only
prepared, and closes the playback PCM when the driver logs "Feedback
sync lost" (a close of a PCM that is not running does not stop the
URBs). A variant does the same with the capture runtime (line 414).
Two more programs serve the device over USB/IP (vhci_hcd) and close
the PCMs at a high rate. The reproducers are available to maintainers
on request.

Decoded, torvalds/master 7e1392a42ea2 (the object is the kmalloc-2k
snd_pcm_runtime, allocated by snd_pcm_attach_substream() on open):

  BUG: KASAN: slab-use-after-free in feedback_urb_complete (sound/usb/usx2y/us144mkii_playback.c:402)
  Read of size 8 at addr ffff0000132ba078 by task repro/82
  Call trace:
   feedback_urb_complete (sound/usb/usx2y/us144mkii_playback.c:402)
   __usb_hcd_giveback_urb (drivers/usb/core/hcd.c:1657)
   usb_giveback_urb_bh (drivers/usb/core/hcd.c:1691)
   process_one_work (kernel/workqueue.c:3495)
   bh_worker (kernel/workqueue.c:3578 kernel/workqueue.c:3885)
  Freed by task 83:
   kfree (include/linux/kasan.h:235 mm/slub.c:2780 mm/slub.c:6630 mm/slub.c:6923)
   snd_pcm_detach_substream (sound/core/pcm.c:1014)
   snd_pcm_release_substream.part.0 (sound/core/pcm_native.c:2858)
   snd_pcm_release (sound/core/pcm_native.c:2840 sound/core/pcm_native.c:3037)
   __fput (fs/file_table.c:512)

Line 402 is the playback_rt->period_size read after the locked
section's dev_err(); line 414 (capture_rt->period_size) was hit by the
capture variant.

Testing, KASAN: without the patch the programs crashed in
feedback_urb_complete() on v7.3.1 (3/3 in this test, :402 and :414)
and on 7e1392a42ea2 (4/4 in the verification). With the whole series
applied, no us144mkii report in 27 runs: v7.3.1 15, 97d73c825122 7,
and 5 runs on v7.3.1 with a debug patch that holds the race window
open for 20 ms (181 windows held with a close pending; the same debug
kernel without this patch crashed 3/3 at the first window).

Four of the 27 runs (USB/IP programs) ended in a KASAN use-after-free
in vhci_urb_dequeue(), a race in the vhci_hcd test harness between
vhci_rx and usb_kill_anchored_urbs(), not in this driver; the same
programs with KASAN multi-shot and no panic ran the full time with no
report (3 runs).

Cost: one RCU grace period in each .close and in .sync_stop (prepare,
hw_params or hw_free after a stop). The capture work is covered by
.sync_stop because synchronize_rcu() also waits for regions with
interrupts disabled; an explicit rcu_read_lock() in
tascam_capture_work_handler() is a two-line alternative.

sound/usb/usx2y/us144mkii*.c are identical in v7.3.1, 7e1392a42ea2,
9d66f26fefac and 97d73c825122.

Thanks,
Alexandre Becholey

 sound/usb/usx2y/us144mkii_capture.c  |  3 +++
 sound/usb/usx2y/us144mkii_pcm.c      | 14 ++++++++++++++
 sound/usb/usx2y/us144mkii_pcm.h      |  8 ++++++++
 sound/usb/usx2y/us144mkii_playback.c | 23 ++++++++++++++++++-----
 4 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/sound/usb/usx2y/us144mkii_capture.c b/sound/usb/usx2y/us144mkii_capture.c
index 9c142a85e8b2..fa7b0cd94c71 100644
--- a/sound/usb/usx2y/us144mkii_capture.c
+++ b/sound/usb/usx2y/us144mkii_capture.c
@@ -40,6 +40,8 @@ static int tascam_capture_close(struct snd_pcm_substream *substream)
 		tascam->capture_substream = NULL;
 	/* The stop work may still be using this substream; wait for it. */
 	flush_work(&tascam->stop_pcm_work);
+	/* So may the URB completion handlers, see tascam_pcm_sync_stop(). */
+	synchronize_rcu();
 	/*
 	 * The capture work keeps a pointer to the runtime, which is freed
 	 * when this callback returns. The capture URBs (and so the work) keep
@@ -113,6 +115,7 @@ const struct snd_pcm_ops tascam_capture_ops = {
 	.ioctl = snd_pcm_lib_ioctl,
 	.hw_params = tascam_pcm_hw_params,
 	.hw_free = tascam_pcm_hw_free,
+	.sync_stop = tascam_pcm_sync_stop,
 	.prepare = tascam_capture_prepare,
 	.trigger = tascam_pcm_trigger,
 	.pointer = tascam_capture_pointer,
diff --git a/sound/usb/usx2y/us144mkii_pcm.c b/sound/usb/usx2y/us144mkii_pcm.c
index 03dfb1f38801..5add9c1b253a 100644
--- a/sound/usb/usx2y/us144mkii_pcm.c
+++ b/sound/usb/usx2y/us144mkii_pcm.c
@@ -259,6 +259,20 @@ int tascam_pcm_hw_free(struct snd_pcm_substream *substream)
 	return 0;
 }
 
+int tascam_pcm_sync_stop(struct snd_pcm_substream *substream)
+{
+	/*
+	 * The URBs of both streams keep running as long as one of them does.
+	 * The playback URB completion handler uses the substream inside an RCU
+	 * read-side section, and the capture work writes to the buffer with
+	 * tascam->lock held (interrupts disabled), which synchronize_rcu()
+	 * also waits for.  Wait for both before the buffer is freed or
+	 * reallocated.
+	 */
+	synchronize_rcu();
+	return 0;
+}
+
 int tascam_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 {
 	struct tascam_card *tascam = snd_pcm_substream_chip(substream);
diff --git a/sound/usb/usx2y/us144mkii_pcm.h b/sound/usb/usx2y/us144mkii_pcm.h
index a130678405e3..22c05c61fdd8 100644
--- a/sound/usb/usx2y/us144mkii_pcm.h
+++ b/sound/usb/usx2y/us144mkii_pcm.h
@@ -139,6 +139,14 @@ int tascam_pcm_hw_params(struct snd_pcm_substream *substream,
  */
 int tascam_pcm_hw_free(struct snd_pcm_substream *substream);
 
+/**
+ * tascam_pcm_sync_stop() - Waits for the URB completion handlers after a stop.
+ * @substream: The ALSA PCM substream.
+ *
+ * Return: 0.
+ */
+int tascam_pcm_sync_stop(struct snd_pcm_substream *substream);
+
 /**
  * tascam_pcm_trigger() - Triggers the start or stop of PCM streams.
  * @substream: The ALSA PCM substream.
diff --git a/sound/usb/usx2y/us144mkii_playback.c b/sound/usb/usx2y/us144mkii_playback.c
index e6266488cf32..36d5417d9975 100644
--- a/sound/usb/usx2y/us144mkii_playback.c
+++ b/sound/usb/usx2y/us144mkii_playback.c
@@ -40,6 +40,8 @@ static int tascam_playback_close(struct snd_pcm_substream *substream)
 		tascam->playback_substream = NULL;
 	/* The stop work may still be using this substream; wait for it. */
 	flush_work(&tascam->stop_pcm_work);
+	/* So may the URB completion handlers, see tascam_pcm_sync_stop(). */
+	synchronize_rcu();
 
 	return 0;
 }
@@ -153,6 +155,7 @@ const struct snd_pcm_ops tascam_playback_ops = {
 	.ioctl = snd_pcm_lib_ioctl,
 	.hw_params = tascam_pcm_hw_params,
 	.hw_free = tascam_pcm_hw_free,
+	.sync_stop = tascam_pcm_sync_stop,
 	.prepare = tascam_playback_prepare,
 	.trigger = tascam_pcm_trigger,
 	.pointer = tascam_playback_pointer,
@@ -180,9 +183,13 @@ void playback_urb_complete(struct urb *urb)
 	if (!tascam || !atomic_read(&tascam->playback_active))
 		goto out;
 
-	substream = tascam->playback_substream;
-	if (!substream || !substream->runtime)
+	/* Pairs with synchronize_rcu() in close and sync_stop. */
+	rcu_read_lock();
+	substream = READ_ONCE(tascam->playback_substream);
+	if (!substream || !substream->runtime) {
+		rcu_read_unlock();
 		goto out;
+	}
 	runtime = substream->runtime;
 
 	scoped_guard(spinlock_irqsave, &tascam->lock) {
@@ -252,6 +259,7 @@ void playback_urb_complete(struct urb *urb)
 		process_playback_routing_us144mkii(tascam, dst_buf, dst_buf,
 						   frames_to_copy);
 	}
+	rcu_read_unlock();
 
 	urb->dev = tascam->dev;
 	usb_get_urb(urb);
@@ -295,12 +303,16 @@ void feedback_urb_complete(struct urb *urb)
 	if (!tascam || !atomic_read(&tascam->playback_active))
 		goto out;
 
-	playback_ss = tascam->playback_substream;
-	if (!playback_ss || !playback_ss->runtime)
+	/* Pairs with synchronize_rcu() in close and sync_stop. */
+	rcu_read_lock();
+	playback_ss = READ_ONCE(tascam->playback_substream);
+	if (!playback_ss || !playback_ss->runtime) {
+		rcu_read_unlock();
 		goto out;
+	}
 	playback_rt = playback_ss->runtime;
 
-	capture_ss = tascam->capture_substream;
+	capture_ss = READ_ONCE(tascam->capture_substream);
 	capture_rt = capture_ss ? capture_ss->runtime : NULL;
 
 	scoped_guard(spinlock_irqsave, &tascam->lock) {
@@ -444,6 +456,7 @@ void feedback_urb_complete(struct urb *urb)
 		snd_pcm_period_elapsed(playback_ss);
 	if (capture_period_elapsed)
 		snd_pcm_period_elapsed(capture_ss);
+	rcu_read_unlock();
 
 	urb->dev = tascam->dev;
 	usb_get_urb(urb);

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

[Alexandre Becholey]

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;
 }

Re: [PATCH 3/5] ALSA: usx2y: us144mkii: only touch the buffer of a running PCM stream

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ALSA: usx2y: us144mkii: pointer desync when streams start
  sequentially
--

commit e23f2c6273b24e6b8b4a5dbee2dc9fa0b6647cd7
Author: Alexandre Becholey <alex@becholey.ch>

ALSA: usx2y: us144mkii: only touch the buffer of a running PCM stream

This commit gates DMA buffer accesses in tascam_capture_work_handler() and
playback_urb_complete() on snd_pcm_running() so that starting one PCM
direction does not read or write the other substream's buffer before it is
running.

> diff --git a/sound/usb/usx2y/us144mkii_capture.c b/sound/usb/usx2y/us144mkii_capture.c
> index 973bed60bfcd1..9c142a85e8b21 100644
> --- a/sound/usb/usx2y/us144mkii_capture.c
> +++ b/sound/usb/usx2y/us144mkii_capture.c
> @@ -231,7 +231,13 @@ void tascam_capture_work_handler(struct work_struct *work)
>  						  routed_block);
>  
>  		scoped_guard(spinlock_irqsave, &tascam->lock) {
> -			if (atomic_read(&tascam->capture_active)) {
> +			/*
> +			 * capture_active is also set when only the playback
> +			 * stream was started; the capture buffer is only valid
> +			 * while the capture stream itself is running.
> +			 */
> +			if (atomic_read(&tascam->capture_active) &&
> +			    snd_pcm_running(substream)) {
>  				int f;
>  
>  				for (f = 0; f < FRAMES_PER_DECODE_BLOCK; ++f) {
> diff --git a/sound/usb/usx2y/us144mkii_playback.c b/sound/usb/usx2y/us144mkii_playback.c
> index 0df95dbf585db..e6266488cf325 100644
> --- a/sound/usb/usx2y/us144mkii_playback.c
> +++ b/sound/usb/usx2y/us144mkii_playback.c
> @@ -166,6 +166,7 @@ void playback_urb_complete(struct urb *urb)
>  	size_t total_bytes_for_urb = 0;
>  	snd_pcm_uframes_t offset_frames;
>  	snd_pcm_uframes_t frames_to_copy;
> +	bool running;
>  	int ret, i;
>  
>  	if (urb->status) {
> @@ -207,13 +208,25 @@ void playback_urb_complete(struct urb *urb)
>  		}
>  		urb->transfer_buffer_length = total_bytes_for_urb;
>  
> -		offset_frames = tascam->driver_playback_pos;
> -		frames_to_copy = bytes_to_frames(runtime, total_bytes_for_urb);
> -		tascam->driver_playback_pos =
> -			(offset_frames + frames_to_copy) % runtime->buffer_size;
> +		/*
> +		 * The URBs also run when only the capture stream was started;
> +		 * the playback buffer is only valid while the playback stream
> +		 * itself is running.  Send silence otherwise.
> +		 */
> +		running = snd_pcm_running(substream);
> +		if (running) {
> +			offset_frames = tascam->driver_playback_pos;
> +			frames_to_copy = bytes_to_frames(runtime,
> +							 total_bytes_for_urb);
> +			tascam->driver_playback_pos =
> +				(offset_frames + frames_to_copy) %
> +				runtime->buffer_size;
> +		}
>  	}

[Severity: Medium]
Does holding driver_playback_pos and driver_capture_pos at 0 while the
substream is not running desynchronize them from playback_frames_consumed
and capture_frames_processed?

feedback_urb_complete() still advances both frame counters unconditionally
while the URBs are active:

sound/usb/usx2y/us144mkii_playback.c:feedback_urb_complete() {
    ...
		if (total_frames_in_urb > 0) {
			tascam->playback_frames_consumed += total_frames_in_urb;
			if (atomic_read(&tascam->capture_active))
				tascam->capture_frames_processed +=
					total_frames_in_urb;
		}
    ...
}

When both substreams are prepared and one is started before the other,
feedback_urb_complete() increments the prepared substream's frame counter
while its driver_*_pos stays at 0.  Starting the second substream does not
call .prepare() again, so tascam_playback_pointer() or
tascam_capture_pointer() reports a hardware pointer ahead of where the
driver reads or writes in runtime->dma_area, causing misaligned audio or
xruns.  Should feedback_urb_complete() also gate each stream's counter and
period updates on snd_pcm_running()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114172806.88710-1-alex@becholey.ch?part=3

Re: [PATCH 4/5] ALSA: usx2y: us144mkii: wait for the URB completion handlers on stop and close

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ALSA: usx2y: us144mkii: missing rcu_assign_pointer() in PCM
  open callbacks
--

commit 6a69f01bf459c14f59000c815994e8cebf887174
Author: Alexandre Becholey <alex@becholey.ch>

ALSA: usx2y: us144mkii: wait for the URB completion handlers on stop and close

This commit protects the playback and capture substream accesses in the
URB completion handlers with RCU read-side critical sections and waits for
grace periods in the PCM .close and .sync_stop callbacks.

> diff --git a/sound/usb/usx2y/us144mkii_playback.c b/sound/usb/usx2y/us144mkii_playback.c
> index e6266488cf325..36d5417d9975a 100644
> --- a/sound/usb/usx2y/us144mkii_playback.c
> +++ b/sound/usb/usx2y/us144mkii_playback.c
[ ... ]
> @@ -180,9 +183,13 @@ void playback_urb_complete(struct urb *urb)
>  	if (!tascam || !atomic_read(&tascam->playback_active))
>  		goto out;
>  
> -	substream = tascam->playback_substream;
> -	if (!substream || !substream->runtime)
> +	/* Pairs with synchronize_rcu() in close and sync_stop. */
> +	rcu_read_lock();
> +	substream = READ_ONCE(tascam->playback_substream);
> +	if (!substream || !substream->runtime) {
> +		rcu_read_unlock();
>  		goto out;
> +	}
>  	runtime = substream->runtime;
[ ... ]
> @@ -295,12 +303,16 @@ void feedback_urb_complete(struct urb *urb)
>  	if (!tascam || !atomic_read(&tascam->playback_active))
>  		goto out;
>  
> -	playback_ss = tascam->playback_substream;
> -	if (!playback_ss || !playback_ss->runtime)
> +	/* Pairs with synchronize_rcu() in close and sync_stop. */
> +	rcu_read_lock();
> +	playback_ss = READ_ONCE(tascam->playback_substream);
> +	if (!playback_ss || !playback_ss->runtime) {
> +		rcu_read_unlock();
>  		goto out;
> +	}
>  	playback_rt = playback_ss->runtime;
>  
> -	capture_ss = tascam->capture_substream;
> +	capture_ss = READ_ONCE(tascam->capture_substream);
>  	capture_rt = capture_ss ? capture_ss->runtime : NULL;

[Severity: Medium]
Can playback_urb_complete() and feedback_urb_complete() observe a non-NULL
playback_substream or capture_substream before substream->runtime and the
newly allocated struct snd_pcm_runtime are initialized?

tascam_playback_open() and tascam_capture_open() publish
tascam->playback_substream and tascam->capture_substream with plain stores
without a release barrier:

sound/usb/usx2y/us144mkii_playback.c:tascam_playback_open() {
	struct tascam_card *tascam = snd_pcm_substream_chip(substream);

	substream->runtime->hw = tascam_pcm_hw;
	tascam->playback_substream = substream;
	atomic_set(&tascam->playback_active, 0);

Because struct snd_pcm_substream is preallocated when the card is created,
there is no address dependency from the runtime allocation in
snd_pcm_attach_substream() to the substream pointer itself.  If one
substream is opened while the other direction already has the URBs running,
the completion handlers on a weakly-ordered CPU can observe the published
substream pointer while seeing either a NULL substream->runtime (causing
playback_urb_complete() or feedback_urb_complete() to bail out without
resubmitting the URB) or stale fields in a reused struct snd_pcm_runtime
before its zero-initialization becomes visible.

Using rcu_assign_pointer() in tascam_playback_open() and
tascam_capture_open() (paired with rcu_dereference() here, and NULLing the
pointers with RCU_INIT_POINTER() in the .close callbacks) would order the
runtime initialization before the substream pointer publication.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114172806.88710-1-alex@becholey.ch?part=4