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

[Hans Verkuil <hverkuil+cisco@kernel.org>]

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