[PATCH] media: cx231xx-audio: gate wq_trigger on an audio-local teardown flag

[PATCH] media: cx231xx-audio: gate wq_trigger on an audio-local teardown flag

[Fan Wu]

audio_trigger() is deferred work (dev->wq_trigger) armed from
snd_cx231xx_capture_trigger() on every PCM START/STOP; it dereferences
dev->adev state and may free the URBs via cx231xx_isoc_audio_deinit().
cx231xx_audio_fini() tore down that state (snd_card_free_when_closed,
alt_max_pkt_size) without draining wq_trigger, so work armed before or
racing fini ran against freed state.

Adding cancel_work_sync() alone is insufficient: in capture_trigger() the
DEV_DISCONNECTED test and schedule_work() were not atomic, and
DEV_DISCONNECTED is only set on USB disconnect, but fini also runs on
cx231xx-alsa module unload (cx231xx_unregister_extension()), which never
sets it.  A trigger that passed the check could still queue work after
fini's cancel returned an empty queue.

Add an audio-local teardown gate (dev->adev.teardown): fini raises it under
adev.slock, releases the lock, then calls cancel_work_sync() outside the
spinlock.  Both arm sites perform the teardown check and schedule_work()
inside one adev.slock section, so once the gate is visible no new work can
arm after cancel returns.  Initialize the lock, work and gate at the top of
cx231xx_audio_init(), before any fallible allocation, and clear the
partially-built audio state on its error path, so fini is safe even if a
later step fails.

This issue was found by an in-house static analysis tool.

Fixes: 61b04cb24a12 ("[media] cx231xx-audio: fix some locking issues")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.5
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/media/usb/cx231xx/cx231xx-audio.c | 42 +++++++++++++++++++----
 drivers/media/usb/cx231xx/cx231xx.h       |  1 +
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/cx231xx/cx231xx-audio.c b/drivers/media/usb/cx231xx/cx231xx-audio.c
index 9c71b32552df..44ca75b18a5d 100644
--- a/drivers/media/usb/cx231xx/cx231xx-audio.c
+++ b/drivers/media/usb/cx231xx/cx231xx-audio.c
@@ -441,6 +441,7 @@ static int snd_cx231xx_capture_open(struct snd_pcm_substream *substream)
 static int snd_cx231xx_pcm_close(struct snd_pcm_substream *substream)
 {
 	int ret;
+	unsigned long flags;
 	struct cx231xx *dev = snd_pcm_substream_chip(substream);
 
 	dev_dbg(dev->dev, "closing device\n");
@@ -470,7 +471,11 @@ static int snd_cx231xx_pcm_close(struct snd_pcm_substream *substream)
 		dev_dbg(dev->dev, "released lock\n");
 		if (atomic_read(&dev->stream_started) > 0) {
 			atomic_set(&dev->stream_started, 0);
-			schedule_work(&dev->wq_trigger);
+
+			spin_lock_irqsave(&dev->adev.slock, flags);
+			if (!dev->adev.teardown)
+				schedule_work(&dev->wq_trigger);
+			spin_unlock_irqrestore(&dev->adev.slock, flags);
 		}
 	}
 	return 0;
@@ -509,11 +514,14 @@ static int snd_cx231xx_capture_trigger(struct snd_pcm_substream *substream,
 {
 	struct cx231xx *dev = snd_pcm_substream_chip(substream);
 	int retval = 0;
+	unsigned long flags;
 
-	if (dev->state & DEV_DISCONNECTED)
+	spin_lock_irqsave(&dev->adev.slock, flags);
+	if (dev->adev.teardown || (dev->state & DEV_DISCONNECTED)) {
+		spin_unlock_irqrestore(&dev->adev.slock, flags);
 		return -ENODEV;
+	}
 
-	spin_lock(&dev->adev.slock);
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 		atomic_set(&dev->stream_started, 1);
@@ -525,10 +533,10 @@ static int snd_cx231xx_capture_trigger(struct snd_pcm_substream *substream,
 		retval = -EINVAL;
 		break;
 	}
-	spin_unlock(&dev->adev.slock);
 
 	schedule_work(&dev->wq_trigger);
 
+	spin_unlock_irqrestore(&dev->adev.slock, flags);
 	return retval;
 }
 
@@ -576,12 +584,20 @@ static int cx231xx_audio_init(struct cx231xx *dev)
 	dev_dbg(dev->dev,
 		"probing for cx231xx non standard usbaudio\n");
 
+	/*
+	 * Extension init errors are ignored by the cx231xx core, so fini()
+	 * must be safe even if initialization fails part way through.
+	 */
+	spin_lock_init(&adev->slock);
+	INIT_WORK(&dev->wq_trigger, audio_trigger);
+	adev->teardown = false;
+	atomic_set(&dev->stream_started, 0);
+
 	err = snd_card_new(dev->dev, index[devnr], "Cx231xx Audio",
 			   THIS_MODULE, 0, &card);
 	if (err < 0)
 		return err;
 
-	spin_lock_init(&adev->slock);
 	err = snd_pcm_new(card, "Cx231xx Audio", 0, 0, 1, &pcm);
 	if (err < 0)
 		goto err_free_card;
@@ -596,8 +612,6 @@ static int cx231xx_audio_init(struct cx231xx *dev)
 	strscpy(card->shortname, "Cx231xx Audio", sizeof(card->shortname));
 	strscpy(card->longname, "Conexant cx231xx Audio", sizeof(card->longname));
 
-	INIT_WORK(&dev->wq_trigger, audio_trigger);
-
 	err = snd_card_register(card);
 	if (err < 0)
 		goto err_free_card;
@@ -651,14 +665,18 @@ static int cx231xx_audio_init(struct cx231xx *dev)
 
 err_free_pkt_size:
 	kfree(adev->alt_max_pkt_size);
+	adev->alt_max_pkt_size = NULL;
 err_free_card:
 	snd_card_free(card);
+	adev->sndcard = NULL;
 
 	return err;
 }
 
 static int cx231xx_audio_fini(struct cx231xx *dev)
 {
+	unsigned long flags;
+
 	if (dev == NULL)
 		return 0;
 
@@ -669,6 +687,16 @@ static int cx231xx_audio_fini(struct cx231xx *dev)
 		return 0;
 	}
 
+	/*
+	 * Block new trigger work before draining already queued work.
+	 * cancel_work_sync() may sleep, so it must run after dropping slock.
+	 */
+	spin_lock_irqsave(&dev->adev.slock, flags);
+	dev->adev.teardown = true;
+	spin_unlock_irqrestore(&dev->adev.slock, flags);
+
+	cancel_work_sync(&dev->wq_trigger);
+
 	if (dev->adev.sndcard) {
 		snd_card_free_when_closed(dev->adev.sndcard);
 		kfree(dev->adev.alt_max_pkt_size);
diff --git a/drivers/media/usb/cx231xx/cx231xx.h b/drivers/media/usb/cx231xx/cx231xx.h
index 19f5036a78d7..99c6a279944f 100644
--- a/drivers/media/usb/cx231xx/cx231xx.h
+++ b/drivers/media/usb/cx231xx/cx231xx.h
@@ -418,6 +418,7 @@ struct cx231xx_audio {
 	struct snd_card *sndcard;
 
 	int users, shutdown;
+	bool teardown;		/* audio fini in progress; set under slock */
 	/* locks */
 	spinlock_t slock;
 
-- 
2.34.1

Re: [PATCH] media: cx231xx-audio: gate wq_trigger on an audio-local teardown flag

[Hans Verkuil]

On 08/07/2026 16:16, Fan Wu wrote:
> audio_trigger() is deferred work (dev->wq_trigger) armed from
> snd_cx231xx_capture_trigger() on every PCM START/STOP; it dereferences
> dev->adev state and may free the URBs via cx231xx_isoc_audio_deinit().
> cx231xx_audio_fini() tore down that state (snd_card_free_when_closed,
> alt_max_pkt_size) without draining wq_trigger, so work armed before or
> racing fini ran against freed state.
> 
> Adding cancel_work_sync() alone is insufficient: in capture_trigger() the
> DEV_DISCONNECTED test and schedule_work() were not atomic, and
> DEV_DISCONNECTED is only set on USB disconnect, but fini also runs on
> cx231xx-alsa module unload (cx231xx_unregister_extension()), which never
> sets it.  A trigger that passed the check could still queue work after
> fini's cancel returned an empty queue.
> 
> Add an audio-local teardown gate (dev->adev.teardown): fini raises it under
> adev.slock, releases the lock, then calls cancel_work_sync() outside the
> spinlock.  Both arm sites perform the teardown check and schedule_work()
> inside one adev.slock section, so once the gate is visible no new work can
> arm after cancel returns.  Initialize the lock, work and gate at the top of
> cx231xx_audio_init(), before any fallible allocation, and clear the
> partially-built audio state on its error path, so fini is safe even if a
> later step fails.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 61b04cb24a12 ("[media] cx231xx-audio: fix some locking issues")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>  drivers/media/usb/cx231xx/cx231xx-audio.c | 42 +++++++++++++++++++----
>  drivers/media/usb/cx231xx/cx231xx.h       |  1 +
>  2 files changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/usb/cx231xx/cx231xx-audio.c b/drivers/media/usb/cx231xx/cx231xx-audio.c
> index 9c71b32552df..44ca75b18a5d 100644
> --- a/drivers/media/usb/cx231xx/cx231xx-audio.c
> +++ b/drivers/media/usb/cx231xx/cx231xx-audio.c
> @@ -441,6 +441,7 @@ static int snd_cx231xx_capture_open(struct snd_pcm_substream *substream)
>  static int snd_cx231xx_pcm_close(struct snd_pcm_substream *substream)
>  {
>  	int ret;
> +	unsigned long flags;
>  	struct cx231xx *dev = snd_pcm_substream_chip(substream);
>  
>  	dev_dbg(dev->dev, "closing device\n");
> @@ -470,7 +471,11 @@ static int snd_cx231xx_pcm_close(struct snd_pcm_substream *substream)
>  		dev_dbg(dev->dev, "released lock\n");
>  		if (atomic_read(&dev->stream_started) > 0) {
>  			atomic_set(&dev->stream_started, 0);
> -			schedule_work(&dev->wq_trigger);
> +
> +			spin_lock_irqsave(&dev->adev.slock, flags);
> +			if (!dev->adev.teardown)
> +				schedule_work(&dev->wq_trigger);
> +			spin_unlock_irqrestore(&dev->adev.slock, flags);
>  		}
>  	}
>  	return 0;
> @@ -509,11 +514,14 @@ static int snd_cx231xx_capture_trigger(struct snd_pcm_substream *substream,
>  {
>  	struct cx231xx *dev = snd_pcm_substream_chip(substream);
>  	int retval = 0;
> +	unsigned long flags;
>  
> -	if (dev->state & DEV_DISCONNECTED)
> +	spin_lock_irqsave(&dev->adev.slock, flags);
> +	if (dev->adev.teardown || (dev->state & DEV_DISCONNECTED)) {
> +		spin_unlock_irqrestore(&dev->adev.slock, flags);
>  		return -ENODEV;
> +	}
>  
> -	spin_lock(&dev->adev.slock);
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  		atomic_set(&dev->stream_started, 1);
> @@ -525,10 +533,10 @@ static int snd_cx231xx_capture_trigger(struct snd_pcm_substream *substream,
>  		retval = -EINVAL;
>  		break;
>  	}
> -	spin_unlock(&dev->adev.slock);
>  
>  	schedule_work(&dev->wq_trigger);
>  
> +	spin_unlock_irqrestore(&dev->adev.slock, flags);
>  	return retval;
>  }
>  
> @@ -576,12 +584,20 @@ static int cx231xx_audio_init(struct cx231xx *dev)
>  	dev_dbg(dev->dev,
>  		"probing for cx231xx non standard usbaudio\n");
>  
> +	/*
> +	 * Extension init errors are ignored by the cx231xx core, so fini()
> +	 * must be safe even if initialization fails part way through.
> +	 */
> +	spin_lock_init(&adev->slock);
> +	INIT_WORK(&dev->wq_trigger, audio_trigger);
> +	adev->teardown = false;
> +	atomic_set(&dev->stream_started, 0);
> +
>  	err = snd_card_new(dev->dev, index[devnr], "Cx231xx Audio",
>  			   THIS_MODULE, 0, &card);
>  	if (err < 0)
>  		return err;
>  
> -	spin_lock_init(&adev->slock);
>  	err = snd_pcm_new(card, "Cx231xx Audio", 0, 0, 1, &pcm);
>  	if (err < 0)
>  		goto err_free_card;
> @@ -596,8 +612,6 @@ static int cx231xx_audio_init(struct cx231xx *dev)
>  	strscpy(card->shortname, "Cx231xx Audio", sizeof(card->shortname));
>  	strscpy(card->longname, "Conexant cx231xx Audio", sizeof(card->longname));
>  
> -	INIT_WORK(&dev->wq_trigger, audio_trigger);
> -
>  	err = snd_card_register(card);
>  	if (err < 0)
>  		goto err_free_card;
> @@ -651,14 +665,18 @@ static int cx231xx_audio_init(struct cx231xx *dev)
>  
>  err_free_pkt_size:
>  	kfree(adev->alt_max_pkt_size);
> +	adev->alt_max_pkt_size = NULL;
>  err_free_card:
>  	snd_card_free(card);
> +	adev->sndcard = NULL;
>  
>  	return err;
>  }
>  
>  static int cx231xx_audio_fini(struct cx231xx *dev)
>  {
> +	unsigned long flags;
> +
>  	if (dev == NULL)
>  		return 0;
>  
> @@ -669,6 +687,16 @@ static int cx231xx_audio_fini(struct cx231xx *dev)
>  		return 0;
>  	}
>  
> +	/*
> +	 * Block new trigger work before draining already queued work.
> +	 * cancel_work_sync() may sleep, so it must run after dropping slock.
> +	 */
> +	spin_lock_irqsave(&dev->adev.slock, flags);
> +	dev->adev.teardown = true;
> +	spin_unlock_irqrestore(&dev->adev.slock, flags);
> +
> +	cancel_work_sync(&dev->wq_trigger);

If I am not mistaken, it should be enough to use disable_work_sync() instead of
cancel_work_sync(): this disables the workqueue as well, preventing new work from
being queued. That avoids all the fiddly 'teardown' bits.

Regards,

	Hans

> +
>  	if (dev->adev.sndcard) {
>  		snd_card_free_when_closed(dev->adev.sndcard);
>  		kfree(dev->adev.alt_max_pkt_size);
> diff --git a/drivers/media/usb/cx231xx/cx231xx.h b/drivers/media/usb/cx231xx/cx231xx.h
> index 19f5036a78d7..99c6a279944f 100644
> --- a/drivers/media/usb/cx231xx/cx231xx.h
> +++ b/drivers/media/usb/cx231xx/cx231xx.h
> @@ -418,6 +418,7 @@ struct cx231xx_audio {
>  	struct snd_card *sndcard;
>  
>  	int users, shutdown;
> +	bool teardown;		/* audio fini in progress; set under slock */
>  	/* locks */
>  	spinlock_t slock;
>  

[PATCH v2] media: cx231xx-audio: drain wq_trigger before audio fini

[Fan Wu]

audio_trigger() is deferred work (dev->wq_trigger) armed from
snd_cx231xx_capture_trigger() on every PCM START/STOP; it dereferences
dev->adev state and may free the URBs via cx231xx_isoc_audio_deinit().
cx231xx_audio_fini() tore down that state without draining wq_trigger,
so work armed before or racing fini ran against freed state.

Use disable_work_sync() in fini to drain the work and prevent further
queueing.  Initialize the work, lock and stream_started counter at the
top of cx231xx_audio_init(), before any fallible allocation, and clear
the partially-built audio state on its error path, so fini is safe even
if a later step fails.

This issue was found by an in-house static analysis tool.

Fixes: 61b04cb24a12 ("[media] cx231xx-audio: fix some locking issues")
Cc: stable@vger.kernel.org # needs adjustments for <= 6.6: use cancel_work_sync()
Link: https://lore.kernel.org/r/8c7e1294-b906-4636-890c-b64d03b0e1d0@kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
Changes in v2: use disable_work_sync() instead of a teardown flag +
cancel_work_sync(), as suggested by Hans Verkuil.  The init-ordering
fix and error-path cleanup from v1 are retained.
---
 drivers/media/platform/ti/vpe/vip.c   | 37 ++++++++++++++++++++++++---
 drivers/media/platform/ti/vpe/vpdma.c |  3 ++-
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/ti/vpe/vip.c b/drivers/media/platform/ti/vpe/vip.c
index cb0a5a07a3d4..30e9a85d4cf5 100644
--- a/drivers/media/platform/ti/vpe/vip.c
+++ b/drivers/media/platform/ti/vpe/vip.c
@@ -814,6 +814,22 @@ static void clear_irqs(struct vip_dev *dev, int irq_num, int list_num)
 	vpdma_clear_list_stat(dev->shared->vpdma, irq_num, dev->slice_id);
 }
 
+/*
+ * Quiesce recovery work and per-list IRQs before releasing stream resources.
+ * disable_work_sync() prevents the overflow handler from requeueing recovery
+ * work. Mask and synchronize IRQs afterwards because a running worker may
+ * have re-enabled them before exiting.
+ */
+static void vip_quiesce_stream(struct vip_stream *stream)
+{
+	struct vip_dev *dev = stream->port->dev;
+
+	disable_work_sync(&stream->recovery_work);
+	disable_irqs(dev, dev->slice_id, stream->list_num);
+	clear_irqs(dev, dev->slice_id, stream->list_num);
+	synchronize_irq(dev->irq);
+}
+
 static void populate_desc_list(struct vip_stream *stream)
 {
 	struct vip_port *port = stream->port;
@@ -2428,6 +2444,7 @@ static int vip_start_streaming(struct vb2_queue *vq, unsigned int count)
 		goto err;
 
 	stream->num_recovery = 0;
+	enable_work(&stream->recovery_work);
 
 	clear_irqs(dev, dev->slice_id, stream->list_num);
 	enable_irqs(dev, dev->slice_id, stream->list_num);
@@ -2452,13 +2469,17 @@ static void vip_stop_streaming(struct vb2_queue *vq)
 	struct vip_dev *dev = port->dev;
 	int ret;
 
+	/*
+	 * A running recovery worker may re-enable the parser, so quiesce it
+	 * and its IRQ handler before stopping the parser or releasing the
+	 * descriptor list.
+	 */
+	vip_quiesce_stream(stream);
+
 	vip_parser_stop_imm(port, true);
 	vip_enable_parser(port, false);
 	unset_fmt_params(stream);
 
-	disable_irqs(dev, dev->slice_id, stream->list_num);
-	clear_irqs(dev, dev->slice_id, stream->list_num);
-
 	if (port->subdev) {
 		ret = v4l2_subdev_call(port->subdev, video, s_stream, 0);
 		if (ret)
@@ -3074,6 +3095,8 @@ static int alloc_stream(struct vip_port *port, int stream_id, int vfl_type)
 		goto do_free_hwlist;
 
 	INIT_WORK(&stream->recovery_work, vip_overflow_recovery_work);
+	/* Start disabled; vip_start_streaming() enables it before IRQs. */
+	disable_work(&stream->recovery_work);
 
 	INIT_LIST_HEAD(&stream->vidq);
 
@@ -3139,6 +3162,13 @@ static void free_stream(struct vip_stream *stream)
 		return;
 
 	dev = stream->port->dev;
+	/*
+	 * Unpublish the stream and quiesce its IRQ handler and recovery worker
+	 * before releasing stream-owned resources.
+	 */
+	stream->port->cap_streams[stream->stream_id] = NULL;
+	vip_quiesce_stream(stream);
+
 	/* Free up the Drop queue */
 	list_for_each_safe(pos, q, &stream->dropq) {
 		buf = list_entry(pos,
@@ -3150,7 +3180,6 @@ static void free_stream(struct vip_stream *stream)
 
 	video_unregister_device(stream->vfd);
 	vpdma_hwlist_release(dev->shared->vpdma, stream->list_num);
-	stream->port->cap_streams[stream->stream_id] = NULL;
 	kfree(stream);
 }
 
diff --git a/drivers/media/platform/ti/vpe/vpdma.c b/drivers/media/platform/ti/vpe/vpdma.c
index 573aa83f62eb..f9f5b2f1ee1a 100644
--- a/drivers/media/platform/ti/vpe/vpdma.c
+++ b/drivers/media/platform/ti/vpe/vpdma.c
@@ -988,7 +988,8 @@ void *vpdma_hwlist_release(struct vpdma_data *vpdma, int list_num)
 
 	spin_lock_irqsave(&vpdma->lock, flags);
 	vpdma->hwlist_used[list_num] = false;
-	priv = vpdma->hwlist_priv;
+	priv = vpdma->hwlist_priv[list_num];
+	vpdma->hwlist_priv[list_num] = NULL;
 	spin_unlock_irqrestore(&vpdma->lock, flags);
 
 	return priv;
-- 
2.34.1