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

[sashiko-bot@kernel.org]

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