[PATCH 1/2] ALSA: seq: oss: Serialize readq reset state with q->lock

[PATCH 1/2] ALSA: seq: oss: Serialize readq reset state with q->lock

[Cen Zhang]

snd_seq_oss_readq_clear() resets qlen, head, and tail without
q->lock even though the normal reader and producer paths serialize the
same ring state under that spinlock. A reset can therefore race
snd_seq_oss_readq_free() or snd_seq_oss_readq_put_event() and leave
stale records in the queue, drop freshly queued ones, or report the
wrong readiness after wakeup. KCSAN reports a data race between
snd_seq_oss_readq_clear() and snd_seq_oss_readq_free().

Take q->lock while clearing the ring and resetting input_time. Factor
the enqueue logic into a caller-locked helper so
snd_seq_oss_readq_put_timestamp() updates its suppression state under
the same lock instead of racing the reset path.

The buggy scenario involves two paths, with each column showing the
order within that path:

reset path:                      locked readq updater:
1. snd_seq_oss_reset() or        1. A reader or callback producer
   release reaches                  takes q->lock on the same queue.
   snd_seq_oss_readq_clear().
2. snd_seq_oss_readq_clear()     2. The updater tests or modifies
   resets qlen, head, tail,         qlen, head, and tail.
   and input_time.
3. snd_seq_oss_readq_clear()     3. The updater completes its
   wakes sleepers on                read-modify-write sequence.
   q->midi_sleep.
4. Without q->lock, the reset    4. The resulting ring state drives
   can overlap the locked           later reads and readiness.
   update.

Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 sound/core/seq/oss/seq_oss_readq.c | 77 ++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 25 deletions(-)

diff --git a/sound/core/seq/oss/seq_oss_readq.c b/sound/core/seq/oss/seq_oss_readq.c
index c880d4771169..06e12707db21 100644
--- a/sound/core/seq/oss/seq_oss_readq.c
+++ b/sound/core/seq/oss/seq_oss_readq.c
@@ -73,13 +73,17 @@ snd_seq_oss_readq_delete(struct seq_oss_readq *q)
 void
 snd_seq_oss_readq_clear(struct seq_oss_readq *q)
 {
-	if (q->qlen) {
-		q->qlen = 0;
-		q->head = q->tail = 0;
+	scoped_guard(spinlock_irqsave, &q->lock) {
+		if (q->qlen) {
+			q->qlen = 0;
+			q->head = 0;
+			q->tail = 0;
+		}
+		q->input_time = (unsigned long)-1;
 	}
+
 	/* if someone sleeping, wake'em up */
 	wake_up(&q->midi_sleep);
-	q->input_time = (unsigned long)-1;
 }
 
 /*
@@ -136,11 +140,11 @@ int snd_seq_oss_readq_sysex(struct seq_oss_readq *q, int dev,
 /*
  * copy an event to input queue:
  * return zero if enqueued
+ * caller must hold lock
  */
-int
-snd_seq_oss_readq_put_event(struct seq_oss_readq *q, union evrec *ev)
+static int snd_seq_oss_readq_put_event_locked(struct seq_oss_readq *q,
+					      union evrec *ev)
 {
-	guard(spinlock_irqsave)(&q->lock);
 	if (q->qlen >= q->maxlen - 1)
 		return -ENOMEM;
 
@@ -148,12 +152,27 @@ snd_seq_oss_readq_put_event(struct seq_oss_readq *q, union evrec *ev)
 	q->tail = (q->tail + 1) % q->maxlen;
 	q->qlen++;
 
-	/* wake up sleeper */
-	wake_up(&q->midi_sleep);
-
 	return 0;
 }
 
+/*
+ * copy an event to input queue:
+ * return zero if enqueued
+ */
+int
+snd_seq_oss_readq_put_event(struct seq_oss_readq *q, union evrec *ev)
+{
+	int rc;
+
+	scoped_guard(spinlock_irqsave, &q->lock) {
+		rc = snd_seq_oss_readq_put_event_locked(q, ev);
+		if (!rc)
+			wake_up(&q->midi_sleep);
+	}
+
+	return rc;
+}
+
 
 /*
  * pop queue
@@ -209,23 +228,31 @@ snd_seq_oss_readq_poll(struct seq_oss_readq *q, struct file *file, poll_table *w
 int
 snd_seq_oss_readq_put_timestamp(struct seq_oss_readq *q, unsigned long curt, int seq_mode)
 {
-	if (curt != q->input_time) {
-		union evrec rec;
-		memset(&rec, 0, sizeof(rec));
-		switch (seq_mode) {
-		case SNDRV_SEQ_OSS_MODE_SYNTH:
-			rec.echo = (curt << 8) | SEQ_WAIT;
-			snd_seq_oss_readq_put_event(q, &rec);
-			break;
-		case SNDRV_SEQ_OSS_MODE_MUSIC:
-			rec.t.code = EV_TIMING;
-			rec.t.cmd = TMR_WAIT_ABS;
-			rec.t.time = curt;
-			snd_seq_oss_readq_put_event(q, &rec);
-			break;
+	int queued = 0;
+
+	scoped_guard(spinlock_irqsave, &q->lock) {
+		if (curt != q->input_time) {
+			union evrec rec;
+
+			memset(&rec, 0, sizeof(rec));
+			switch (seq_mode) {
+			case SNDRV_SEQ_OSS_MODE_SYNTH:
+				rec.echo = (curt << 8) | SEQ_WAIT;
+				queued = !snd_seq_oss_readq_put_event_locked(q, &rec);
+				break;
+			case SNDRV_SEQ_OSS_MODE_MUSIC:
+				rec.t.code = EV_TIMING;
+				rec.t.cmd = TMR_WAIT_ABS;
+				rec.t.time = curt;
+				queued = !snd_seq_oss_readq_put_event_locked(q, &rec);
+				break;
+			}
+			q->input_time = curt;
 		}
-		q->input_time = curt;
 	}
+	if (queued)
+		wake_up(&q->midi_sleep);
+
 	return 0;
 }
 
-- 
2.43.0

[PATCH 2/2] ALSA: seq: avoid stale FIFO cells during resize

[Cen Zhang]

snd_seq_fifo_resize() still needs to publish the replacement pool
before it waits for FIFO users. A blocking snd_seq_read() holds
f->use_lock while it sleeps, so concurrent senders must be able to
queue to the new pool and wake that reader instead of failing against a
closing old pool.

However, snd_seq_fifo_event_in() duplicates an event before it takes
f->lock, and snd_seq_read() can dequeue a cell and later call
snd_seq_fifo_cell_putback() if copy_to_user() or
snd_seq_expand_var_event() fails. If resize swaps f->pool and detaches
oldhead in between, either path can relink an old-pool cell after the
snapshot. That stale cell sits outside the drained oldhead list, keeps
oldpool->counter elevated, and can leave snd_seq_pool_delete() waiting
for the retired pool to drain.

Keep the existing swap-before-wait ordering in snd_seq_fifo_resize(),
but reject stale cells before any FIFO relink. Revalidate event-in cells
under f->lock and retry them against the published replacement pool, and
free stale putback cells instead of linking them back into the FIFO.

The buggy scenario involves two paths, with each column showing the
order within that path:

resize path:                    relink path:
1. Allocate newpool.             1. Take f->use_lock.
2. Swap f->pool to newpool and   2. Duplicate or dequeue an old-pool
   detach oldhead.                  cell before oldpool closes.
3. Mark oldpool closing and      3. Reach a later relink point after
   wait for FIFO users.             resize published newpool.
4. Free oldhead and delete       4. Relink the old-pool cell after
   oldpool.                         resize detached oldhead.
                                 5. Drop f->use_lock.

Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 sound/core/seq/seq_fifo.c | 52 ++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 15 deletions(-)

diff --git a/sound/core/seq/seq_fifo.c b/sound/core/seq/seq_fifo.c
index ebe1394c18a9..cffa8d430374 100644
--- a/sound/core/seq/seq_fifo.c
+++ b/sound/core/seq/seq_fifo.c
@@ -101,13 +101,17 @@ int snd_seq_fifo_event_in(struct snd_seq_fifo *f,
 			  struct snd_seq_event *event)
 {
 	struct snd_seq_event_cell *cell;
+	struct snd_seq_pool *pool;
+	bool linked;
 	int err;
 
 	if (snd_BUG_ON(!f))
 		return -EINVAL;
 
 	guard(snd_seq_fifo)(f);
-	err = snd_seq_event_dup(f->pool, event, &cell, 1, NULL, NULL); /* always non-blocking */
+retry:
+	pool = READ_ONCE(f->pool);
+	err = snd_seq_event_dup(pool, event, &cell, 1, NULL, NULL); /* always non-blocking */
 	if (err < 0) {
 		if ((err == -ENOMEM) || (err == -EAGAIN))
 			atomic_inc(&f->overflow);
@@ -115,14 +119,24 @@ int snd_seq_fifo_event_in(struct snd_seq_fifo *f,
 	}
 		
 	/* append new cells to fifo */
+	linked = false;
 	scoped_guard(spinlock_irqsave, &f->lock) {
-		if (f->tail != NULL)
-			f->tail->next = cell;
-		f->tail = cell;
-		if (f->head == NULL)
-			f->head = cell;
-		cell->next = NULL;
-		f->cells++;
+		if (cell->pool == f->pool) {
+			if (f->tail)
+				f->tail->next = cell;
+			f->tail = cell;
+			if (!f->head)
+				f->head = cell;
+			cell->next = NULL;
+			f->cells++;
+			linked = true;
+		}
+	}
+
+	if (!linked) {
+		/* Retry against the replacement pool after resize publishes it. */
+		snd_seq_cell_free(cell);
+		goto retry;
 	}
 
 	/* wakeup client */
@@ -194,13 +208,21 @@ int snd_seq_fifo_cell_out(struct snd_seq_fifo *f,
 void snd_seq_fifo_cell_putback(struct snd_seq_fifo *f,
 			       struct snd_seq_event_cell *cell)
 {
+	bool linked = false;
+
 	if (cell) {
-		guard(spinlock_irqsave)(&f->lock);
-		cell->next = f->head;
-		f->head = cell;
-		if (!f->tail)
-			f->tail = cell;
-		f->cells++;
+		scoped_guard(spinlock_irqsave, &f->lock) {
+			if (cell->pool == f->pool) {
+				cell->next = f->head;
+				f->head = cell;
+				if (!f->tail)
+					f->tail = cell;
+				f->cells++;
+				linked = true;
+			}
+		}
+		if (!linked)
+			snd_seq_cell_free(cell);
 	}
 }
 
@@ -237,7 +259,7 @@ int snd_seq_fifo_resize(struct snd_seq_fifo *f, int poolsize)
 		oldpool = f->pool;
 		oldhead = f->head;
 		/* exchange pools */
-		f->pool = newpool;
+		WRITE_ONCE(f->pool, newpool);
 		f->head = NULL;
 		f->tail = NULL;
 		f->cells = 0;
-- 
2.43.0