Re: [PATCH] ALSA: usx2y: Initialize idle URBs before submitting streams

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: usx2y: race in usb_stream_start() when URB 0 completes before URB 1
--

commit 136073d2c8fceba09fe79c9e8359e6b5813457a2
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

ALSA: usx2y: Initialize idle URBs before submitting streams

This commit moves the initialization of sk->idle_inurb, sk->idle_outurb,
sk->completed_inurb, and sk->completed_outurb before the URB submission loop
in usb_stream_start() so that early URB completion callbacks do not
dereference NULL pointers.

> diff --git a/sound/usb/usx2y/usb_stream.c b/sound/usb/usx2y/usb_stream.c
> index 3122cf653273d..7d621280fba33 100644
> --- a/sound/usb/usx2y/usb_stream.c
> +++ b/sound/usb/usx2y/usb_stream.c
> @@ -682,6 +682,10 @@ int usb_stream_start(struct usb_stream_kernel *sk)
>  	s->sync_packet = -1;
>  	s->inpacket_head = -1;
>  	sk->iso_frame_balance = 0;
> +	sk->idle_inurb = sk->inurb[USB_STREAM_NURBS - 2];
> +	sk->idle_outurb = sk->outurb[USB_STREAM_NURBS - 2];
> +	sk->completed_inurb = sk->inurb[USB_STREAM_NURBS - 1];
> +	sk->completed_outurb = sk->outurb[USB_STREAM_NURBS - 1];
>  	++try;
>  	for (u = 0; u < 2; u++) {
>  		struct urb *inurb = sk->inurb[u];

[Severity: Medium]
If both sk->inurb[0] and sk->outurb[0] complete in softirq context after
iteration u = 0 submits them, but before iteration u = 1 finishes in process
context, can stream_start() race with usb_stream_start() and submit URB 2
ahead of URB 1?

In sound/usb/usx2y/usb_stream.c, iteration u = 0 prepares sk->outurb[0] and
sk->inurb[0] with equal number_of_packets and submits both via
usb_submit_urb(). When both complete in softirq context via
i_capture_start() and i_playback_start(), balance_capture() and
balance_playback() bring sk->iso_frame_balance back to 0 and call
stream_start():

sound/usb/usx2y/usb_stream.c:stream_start() {
	...
	} else
		playback_prep_freqn(sk, sk->idle_outurb);

	if (submit_urbs(sk, inurb, outurb) < 0)
		return;
	...
}

While s->state is below usb_stream_sync1, stream_start() calls
playback_prep_freqn(sk, sk->idle_outurb) on sk->outurb[2] in softirq context
at the same time usb_stream_start() is running playback_prep_freqn(sk,
sk->outurb[1]) for u = 1 in process context:

sound/usb/usx2y/usb_stream.c:playback_prep_freqn() {
	struct usb_stream *s = sk->s;
	int pack, lb = 0;

	for (pack = 0; pack < sk->n_o_ps; pack++) {
		int l = usb_stream_next_packet_size(sk);

		if (s->idle_outsize + lb + l > s->period_size)
			goto check;

		sk->out_phase = sk->out_phase_peeked;
		urb->iso_frame_desc[pack].offset = lb;
		urb->iso_frame_desc[pack].length = l;
		lb += l;
	}

check:
	urb->number_of_packets = pack;
	urb->transfer_buffer_length = lb;
	s->idle_outsize += lb - s->period_size;
}

Because there is no synchronization between process context and the softirq
completion handler, both calls concurrently read and update sk->out_phase,
sk->out_phase_peeked, and s->idle_outsize. Immediately afterwards,
stream_start() calls submit_urbs(), which submits sk->inurb[2] and
sk->outurb[2] and rotates the idle/completed URB pointers before iteration
u = 1 has submitted sk->inurb[1] and sk->outurb[1].

Should both sk->outurb[0] and sk->outurb[1] be prepared before submitting the
initial URBs, or should softirqs be disabled around the initial preparation
and submission loop in usb_stream_start()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028103158.22996-1-25031212351@stu.xidian.edu.cn?part=1