[BUG] ALSA: usb-audio: drain may fail with multi-port close race

[BUG] ALSA: usb-audio: drain may fail with multi-port close race

[John Keeping]

I'm seeing a bug where data sometimes fails to send on USB MIDI devices
with multiple ports which seems to be a result of a race around closing
ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
pending work at closing a MIDI substream").

The scenario is essentially this program:

	snd_rawmidi_t *port0, *port1;
	snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
	snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);

	snd_rawmidi_write(port0, data, len);

	snd_rawmidi_close(port1);
	snd_rawmidi_close(port0);

What happens seems to be the following:

	write(port0)
	`- snd_usbmidi_output_trigger
	   `- queue_work()
        close(port1)
	`- snd_usbmidi_output_close
	   `- cancel_work_sync()	# Work has not yet started here
	close(port0)
	`- snd_rawmidi_drain_output
	   # Times out because nothing is processing outbound data!

The two ports interact like this because they are on the same endpoint,
so should the work only be cancelled when the last endpoint is closed?


Thanks,
John

Re: [BUG] ALSA: usb-audio: drain may fail with multi-port close race

[Takashi Iwai]

On Mon, 17 Feb 2025 12:16:46 +0100,
John Keeping wrote:
> 
> I'm seeing a bug where data sometimes fails to send on USB MIDI devices
> with multiple ports which seems to be a result of a race around closing
> ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
> pending work at closing a MIDI substream").
> 
> The scenario is essentially this program:
> 
> 	snd_rawmidi_t *port0, *port1;
> 	snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
> 	snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);
> 
> 	snd_rawmidi_write(port0, data, len);
> 
> 	snd_rawmidi_close(port1);
> 	snd_rawmidi_close(port0);
> 
> What happens seems to be the following:
> 
> 	write(port0)
> 	`- snd_usbmidi_output_trigger
> 	   `- queue_work()
>         close(port1)
> 	`- snd_usbmidi_output_close
> 	   `- cancel_work_sync()	# Work has not yet started here
> 	close(port0)
> 	`- snd_rawmidi_drain_output
> 	   # Times out because nothing is processing outbound data!
> 
> The two ports interact like this because they are on the same endpoint,
> so should the work only be cancelled when the last endpoint is closed?

How about the following patch work?
It's a band-aid, but should suffice.  The callback is already
protected with rawmidi open_mutex.


thanks,

Takashi

-- 8< --
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -1144,8 +1144,11 @@ static int snd_usbmidi_output_open(struct snd_rawmidi_substream *substream)
 static int snd_usbmidi_output_close(struct snd_rawmidi_substream *substream)
 {
 	struct usbmidi_out_port *port = substream->runtime->private_data;
+	struct snd_usb_midi *umidi = substream->rmidi->private_data;
 
-	cancel_work_sync(&port->ep->work);
+	/* cancel at the last close */
+	if (umidi->opened[0] == 1)
+		cancel_work_sync(&port->ep->work);
 	return substream_open(substream, 0, 0);
 }
 

Re: [BUG] ALSA: usb-audio: drain may fail with multi-port close race

[John Keeping]

On Mon, Feb 17, 2025 at 06:06:16PM +0100, Takashi Iwai wrote:
> On Mon, 17 Feb 2025 12:16:46 +0100,
> John Keeping wrote:
> > 
> > I'm seeing a bug where data sometimes fails to send on USB MIDI devices
> > with multiple ports which seems to be a result of a race around closing
> > ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
> > pending work at closing a MIDI substream").
> > 
> > The scenario is essentially this program:
> > 
> > 	snd_rawmidi_t *port0, *port1;
> > 	snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
> > 	snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);
> > 
> > 	snd_rawmidi_write(port0, data, len);
> > 
> > 	snd_rawmidi_close(port1);
> > 	snd_rawmidi_close(port0);
> > 
> > What happens seems to be the following:
> > 
> > 	write(port0)
> > 	`- snd_usbmidi_output_trigger
> > 	   `- queue_work()
> >         close(port1)
> > 	`- snd_usbmidi_output_close
> > 	   `- cancel_work_sync()	# Work has not yet started here
> > 	close(port0)
> > 	`- snd_rawmidi_drain_output
> > 	   # Times out because nothing is processing outbound data!
> > 
> > The two ports interact like this because they are on the same endpoint,
> > so should the work only be cancelled when the last endpoint is closed?
> 
> How about the following patch work?
> It's a band-aid, but should suffice.  The callback is already
> protected with rawmidi open_mutex.

Yes, this patch fixes it and is

Tested-by: John Keeping <jkeeping@inmusicbrands.com>


Thanks for the quick response!

John

> -- 8< --
> --- a/sound/usb/midi.c
> +++ b/sound/usb/midi.c
> @@ -1144,8 +1144,11 @@ static int snd_usbmidi_output_open(struct snd_rawmidi_substream *substream)
>  static int snd_usbmidi_output_close(struct snd_rawmidi_substream *substream)
>  {
>  	struct usbmidi_out_port *port = substream->runtime->private_data;
> +	struct snd_usb_midi *umidi = substream->rmidi->private_data;
>  
> -	cancel_work_sync(&port->ep->work);
> +	/* cancel at the last close */
> +	if (umidi->opened[0] == 1)
> +		cancel_work_sync(&port->ep->work);
>  	return substream_open(substream, 0, 0);
>  }
>  

Re: [BUG] ALSA: usb-audio: drain may fail with multi-port close race

[Takashi Iwai]

On Mon, 17 Feb 2025 19:38:31 +0100,
John Keeping wrote:
> 
> On Mon, Feb 17, 2025 at 06:06:16PM +0100, Takashi Iwai wrote:
> > On Mon, 17 Feb 2025 12:16:46 +0100,
> > John Keeping wrote:
> > > 
> > > I'm seeing a bug where data sometimes fails to send on USB MIDI devices
> > > with multiple ports which seems to be a result of a race around closing
> > > ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
> > > pending work at closing a MIDI substream").
> > > 
> > > The scenario is essentially this program:
> > > 
> > > 	snd_rawmidi_t *port0, *port1;
> > > 	snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
> > > 	snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);
> > > 
> > > 	snd_rawmidi_write(port0, data, len);
> > > 
> > > 	snd_rawmidi_close(port1);
> > > 	snd_rawmidi_close(port0);
> > > 
> > > What happens seems to be the following:
> > > 
> > > 	write(port0)
> > > 	`- snd_usbmidi_output_trigger
> > > 	   `- queue_work()
> > >         close(port1)
> > > 	`- snd_usbmidi_output_close
> > > 	   `- cancel_work_sync()	# Work has not yet started here
> > > 	close(port0)
> > > 	`- snd_rawmidi_drain_output
> > > 	   # Times out because nothing is processing outbound data!
> > > 
> > > The two ports interact like this because they are on the same endpoint,
> > > so should the work only be cancelled when the last endpoint is closed?
> > 
> > How about the following patch work?
> > It's a band-aid, but should suffice.  The callback is already
> > protected with rawmidi open_mutex.
> 
> Yes, this patch fixes it and is
> 
> Tested-by: John Keeping <jkeeping@inmusicbrands.com>

Thank you for quick testing!

Looking at the code again, I think the suggested fix isn't right.
It still allows some pending work accessing the freed object.

Could you test the following one-liner instead?


Takashi

-- 8< --
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -1145,7 +1145,7 @@ static int snd_usbmidi_output_close(struct snd_rawmidi_substream *substream)
 {
 	struct usbmidi_out_port *port = substream->runtime->private_data;
 
-	cancel_work_sync(&port->ep->work);
+	flush_work(&port->ep->work);
 	return substream_open(substream, 0, 0);
 }
 

Re: [BUG] ALSA: usb-audio: drain may fail with multi-port close race

[John Keeping]

On Tue, Feb 18, 2025 at 09:07:26AM +0100, Takashi Iwai wrote:
> On Mon, 17 Feb 2025 19:38:31 +0100,
> John Keeping wrote:
> > 
> > On Mon, Feb 17, 2025 at 06:06:16PM +0100, Takashi Iwai wrote:
> > > On Mon, 17 Feb 2025 12:16:46 +0100,
> > > John Keeping wrote:
> > > > 
> > > > I'm seeing a bug where data sometimes fails to send on USB MIDI devices
> > > > with multiple ports which seems to be a result of a race around closing
> > > > ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
> > > > pending work at closing a MIDI substream").
> > > > 
> > > > The scenario is essentially this program:
> > > > 
> > > > 	snd_rawmidi_t *port0, *port1;
> > > > 	snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
> > > > 	snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);
> > > > 
> > > > 	snd_rawmidi_write(port0, data, len);
> > > > 
> > > > 	snd_rawmidi_close(port1);
> > > > 	snd_rawmidi_close(port0);
> > > > 
> > > > What happens seems to be the following:
> > > > 
> > > > 	write(port0)
> > > > 	`- snd_usbmidi_output_trigger
> > > > 	   `- queue_work()
> > > >         close(port1)
> > > > 	`- snd_usbmidi_output_close
> > > > 	   `- cancel_work_sync()	# Work has not yet started here
> > > > 	close(port0)
> > > > 	`- snd_rawmidi_drain_output
> > > > 	   # Times out because nothing is processing outbound data!
> > > > 
> > > > The two ports interact like this because they are on the same endpoint,
> > > > so should the work only be cancelled when the last endpoint is closed?
> > > 
> > > How about the following patch work?
> > > It's a band-aid, but should suffice.  The callback is already
> > > protected with rawmidi open_mutex.
> > 
> > Yes, this patch fixes it and is
> > 
> > Tested-by: John Keeping <jkeeping@inmusicbrands.com>
> 
> Thank you for quick testing!
> 
> Looking at the code again, I think the suggested fix isn't right.
> It still allows some pending work accessing the freed object.
> 
> Could you test the following one-liner instead?

Tested-by: John Keeping <jkeeping@inmusicbrands.com>

The patch below also fixes the issue.  Thanks!


John

> -- 8< --
> --- a/sound/usb/midi.c
> +++ b/sound/usb/midi.c
> @@ -1145,7 +1145,7 @@ static int snd_usbmidi_output_close(struct snd_rawmidi_substream *substream)
>  {
>  	struct usbmidi_out_port *port = substream->runtime->private_data;
>  
> -	cancel_work_sync(&port->ep->work);
> +	flush_work(&port->ep->work);
>  	return substream_open(substream, 0, 0);
>  }
>  

Re: [BUG] ALSA: usb-audio: drain may fail with multi-port close race

[Takashi Iwai]

On Tue, 18 Feb 2025 11:56:19 +0100,
John Keeping wrote:
> 
> On Tue, Feb 18, 2025 at 09:07:26AM +0100, Takashi Iwai wrote:
> > On Mon, 17 Feb 2025 19:38:31 +0100,
> > John Keeping wrote:
> > > 
> > > On Mon, Feb 17, 2025 at 06:06:16PM +0100, Takashi Iwai wrote:
> > > > On Mon, 17 Feb 2025 12:16:46 +0100,
> > > > John Keeping wrote:
> > > > > 
> > > > > I'm seeing a bug where data sometimes fails to send on USB MIDI devices
> > > > > with multiple ports which seems to be a result of a race around closing
> > > > > ports introduced by commit 0125de38122f0 ("ALSA: usb-audio: Cancel
> > > > > pending work at closing a MIDI substream").
> > > > > 
> > > > > The scenario is essentially this program:
> > > > > 
> > > > > 	snd_rawmidi_t *port0, *port1;
> > > > > 	snd_rawmidi_open(NULL, &port0, "hw:0,0,0", 0);
> > > > > 	snd_rawmidi_open(NULL, &port1, "hw:0,0,1", 0);
> > > > > 
> > > > > 	snd_rawmidi_write(port0, data, len);
> > > > > 
> > > > > 	snd_rawmidi_close(port1);
> > > > > 	snd_rawmidi_close(port0);
> > > > > 
> > > > > What happens seems to be the following:
> > > > > 
> > > > > 	write(port0)
> > > > > 	`- snd_usbmidi_output_trigger
> > > > > 	   `- queue_work()
> > > > >         close(port1)
> > > > > 	`- snd_usbmidi_output_close
> > > > > 	   `- cancel_work_sync()	# Work has not yet started here
> > > > > 	close(port0)
> > > > > 	`- snd_rawmidi_drain_output
> > > > > 	   # Times out because nothing is processing outbound data!
> > > > > 
> > > > > The two ports interact like this because they are on the same endpoint,
> > > > > so should the work only be cancelled when the last endpoint is closed?
> > > > 
> > > > How about the following patch work?
> > > > It's a band-aid, but should suffice.  The callback is already
> > > > protected with rawmidi open_mutex.
> > > 
> > > Yes, this patch fixes it and is
> > > 
> > > Tested-by: John Keeping <jkeeping@inmusicbrands.com>
> > 
> > Thank you for quick testing!
> > 
> > Looking at the code again, I think the suggested fix isn't right.
> > It still allows some pending work accessing the freed object.
> > 
> > Could you test the following one-liner instead?
> 
> Tested-by: John Keeping <jkeeping@inmusicbrands.com>
> 
> The patch below also fixes the issue.  Thanks!

OK, I'm going to submit and merge the proper patch.


thanks,

Takashi