Re: Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git

Re: Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git

[Takashi Iwai]

At Thu, 10 Jan 2013 09:23:48 +0100,
Jens Axboe wrote:
> 
> Hi,
> 
> I have a USB DAC that I use for music. Just upgraded my workstation from
> 3.7-rc7 to 3.8-rc3 this morning, and when the DAC is switched on, I get
> an immediate oops. I have attached the picture. It's crashing here:
> 
> (gdb) l *snd_usb_pcm_prepare+0x153
> 0x161c is in snd_usb_pcm_prepare (sound/usb/pcm.c:470).
> 465		snd_pcm_format_t pcm_format)
> 466	{
> 467		int i;
> 468		int score = 0;
> 469	
> 470		if (fp->channels < 1) {
> 471			snd_printdd("%s: (fmt @%p) no channels\n", __func__, fp);
> 472			return 0;
> 473		}
> 474	
> 
> 'fp' is clearly NULL.

Hmm... it's a function called only from a single place in
configure_sync_endpoint() and it's in list_for_each_entry():

	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
			subs->cur_rate, subs->pcm_format);

so it must be hardly NULL in normal situations.
The only scenario I can imagine right now is that the whole structure
is still uninitialized while it's called.  If so, it's a race problem,
and shouldn't be new to 3.8.  A patch like below might paper over the
problem although it's far from perfect.


> Let me know if you want a bisect.

Yeah, that'll be really helpful.

Or, at least, could you try to copy sound/usb/* from 3.7 to 3.8 and
see whether it's really a regression there?


thanks,

Takashi

---
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index ad181d5..3bfc564 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -81,8 +81,7 @@ static void snd_usb_audio_pcm_free(struct snd_pcm *pcm)
  */
 
 static void snd_usb_init_substream(struct snd_usb_stream *as,
-				   int stream,
-				   struct audioformat *fp)
+				   int stream)
 {
 	struct snd_usb_substream *subs = &as->substream[stream];
 
@@ -96,6 +95,13 @@ static void snd_usb_init_substream(struct snd_usb_stream *as,
 	subs->speed = snd_usb_get_speed(subs->dev);
 
 	snd_usb_set_pcm_ops(as->pcm, stream);
+}
+
+static void snd_usb_add_substream_format(struct snd_usb_stream *as,
+				   int stream,
+				   struct audioformat *fp)
+{
+	struct snd_usb_substream *subs = &as->substream[stream];
 
 	list_add_tail(&fp->list, &subs->fmt_list);
 	subs->formats |= fp->formats;
@@ -342,7 +348,7 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
 		err = snd_pcm_new_stream(as->pcm, stream, 1);
 		if (err < 0)
 			return err;
-		snd_usb_init_substream(as, stream, fp);
+		snd_usb_add_substream_format(as, stream, fp);
 		return add_chmap(as->pcm, stream, subs);
 	}
 
@@ -370,7 +376,9 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
 	else
 		strcpy(pcm->name, "USB Audio");
 
-	snd_usb_init_substream(as, stream, fp);
+	snd_usb_init_substream(as, SNDRV_PCM_STREAM_PLAYBACK);
+	snd_usb_init_substream(as, SNDRV_PCM_STREAM_CAPTURE);
+	snd_usb_add_substream_format(as, stream, fp);
 
 	list_add(&as->list, &chip->pcm_list);
 	chip->pcm_devs++;

Re: Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git

[Jens Axboe]

On 2013-01-10 11:21, Takashi Iwai wrote:
> At Thu, 10 Jan 2013 09:23:48 +0100,
> Jens Axboe wrote:
>>
>> Hi,
>>
>> I have a USB DAC that I use for music. Just upgraded my workstation from
>> 3.7-rc7 to 3.8-rc3 this morning, and when the DAC is switched on, I get
>> an immediate oops. I have attached the picture. It's crashing here:
>>
>> (gdb) l *snd_usb_pcm_prepare+0x153
>> 0x161c is in snd_usb_pcm_prepare (sound/usb/pcm.c:470).
>> 465		snd_pcm_format_t pcm_format)
>> 466	{
>> 467		int i;
>> 468		int score = 0;
>> 469	
>> 470		if (fp->channels < 1) {
>> 471			snd_printdd("%s: (fmt @%p) no channels\n", __func__, fp);
>> 472			return 0;
>> 473		}
>> 474	
>>
>> 'fp' is clearly NULL.
> 
> Hmm... it's a function called only from a single place in
> configure_sync_endpoint() and it's in list_for_each_entry():
> 
> 	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
> 		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
> 			subs->cur_rate, subs->pcm_format);
> 
> so it must be hardly NULL in normal situations.
> The only scenario I can imagine right now is that the whole structure
> is still uninitialized while it's called.  If so, it's a race problem,
> and shouldn't be new to 3.8.  A patch like below might paper over the
> problem although it's far from perfect.
> 
> 
>> Let me know if you want a bisect.
> 
> Yeah, that'll be really helpful.

Here it is, it's from the one introducing the audioformat lookup.
Confirmed that 3.8-rc3 with this backed out works fine, too. So should
be fairly confident in that result.


commit 0d9741c0e058e2857fe3fed37975515dc8dcd21d
Author: Eldad Zack <eldad@fogrefinery.com>
Date:   Mon Dec 3 20:30:09 2012 +0100

    ALSA: usb-audio: sync ep init fix for audioformat mismatch
    
    Commit 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb:
    properly initialize the sync endpoint", while correcting the
    initialization of the sync endpoint when opening just the data
    endpoint, prevents devices that has a sync endpoint, with a channel
    number different than that of the data endpoint, from functioning.
    Due to a different channel and period bytes count, attempting to
    initialize the sync endpoint will fail at the usb host driver.
    For example, when using xhci:
    
     cannot submit urb 0, error -90: internal error
    
    With this patch, if a sync endpoint has multiple audioformats, a
    matching audioformat is preferred. An audioformat must be found
    with at least one channel and support the requested sample rate
    and PCM format, otherwise the stream will not be opened.
    
    If the number of channels differ between the selected audioformat
    and the requested format, adjust the period bytes count accordingly.
    It is safe to perform the calculation on the basis of the channel
    count, since the requested PCM audio format and the rate must be
    supported by the selected audioformat.
    
    Cc: Jeffrey Barish <jeff_barish@earthlink.net>
    Cc: Daniel Mack <zonque@gmail.com>
    Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
    Signed-off-by: Takashi Iwai <tiwai@suse.de>

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 769821c..c659310 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -454,6 +454,103 @@ add_sync_ep:
 }
 
 /*
+ * Return the score of matching two audioformats.
+ * Veto the audioformat if:
+ * - It has no channels for some reason.
+ * - Requested PCM format is not supported.
+ * - Requested sample rate is not supported.
+ */
+static int match_endpoint_audioformats(struct audioformat *fp,
+	struct audioformat *match, int rate,
+	snd_pcm_format_t pcm_format)
+{
+	int i;
+	int score = 0;
+
+	if (fp->channels < 1) {
+		snd_printdd("%s: (fmt @%p) no channels\n", __func__, fp);
+		return 0;
+	}
+
+	if (!(fp->formats & (1ULL << pcm_format))) {
+		snd_printdd("%s: (fmt @%p) no match for format %d\n", __func__,
+			fp, pcm_format);
+		return 0;
+	}
+
+	for (i = 0; i < fp->nr_rates; i++) {
+		if (fp->rate_table[i] == rate) {
+			score++;
+			break;
+		}
+	}
+	if (!score) {
+		snd_printdd("%s: (fmt @%p) no match for rate %d\n", __func__,
+			fp, rate);
+		return 0;
+	}
+
+	if (fp->channels == match->channels)
+		score++;
+
+	snd_printdd("%s: (fmt @%p) score %d\n", __func__, fp, score);
+
+	return score;
+}
+
+/*
+ * Configure the sync ep using the rate and pcm format of the data ep.
+ */
+static int configure_sync_endpoint(struct snd_usb_substream *subs)
+{
+	int ret;
+	struct audioformat *fp;
+	struct audioformat *sync_fp = NULL;
+	int cur_score = 0;
+	int sync_period_bytes = subs->period_bytes;
+	struct snd_usb_substream *sync_subs =
+		&subs->stream->substream[subs->direction ^ 1];
+
+	/* Try to find the best matching audioformat. */
+	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
+		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
+			subs->cur_rate, subs->pcm_format);
+
+		if (score > cur_score) {
+			sync_fp = fp;
+			cur_score = score;
+		}
+	}
+
+	if (unlikely(sync_fp == NULL)) {
+		snd_printk(KERN_ERR "%s: no valid audioformat for sync ep %x found\n",
+			__func__, sync_subs->ep_num);
+		return -EINVAL;
+	}
+
+	/*
+	 * Recalculate the period bytes if channel number differ between
+	 * data and sync ep audioformat.
+	 */
+	if (sync_fp->channels != subs->channels) {
+		sync_period_bytes = (subs->period_bytes / subs->channels) *
+			sync_fp->channels;
+		snd_printdd("%s: adjusted sync ep period bytes (%d -> %d)\n",
+			__func__, subs->period_bytes, sync_period_bytes);
+	}
+
+	ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
+					  subs->pcm_format,
+					  sync_fp->channels,
+					  sync_period_bytes,
+					  subs->cur_rate,
+					  sync_fp,
+					  NULL);
+
+	return ret;
+}
+
+/*
  * configure endpoint params
  *
  * called  during initial setup and upon resume
@@ -475,13 +572,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
 		return ret;
 
 	if (subs->sync_endpoint)
-		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
-						  subs->pcm_format,
-						  subs->channels,
-						  subs->period_bytes,
-						  subs->cur_rate,
-						  subs->cur_audiofmt,
-						  NULL);
+		ret = configure_sync_endpoint(subs);
+
 	return ret;
 }
 

-- 
Jens Axboe

Re: Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git

[Takashi Iwai]

At Thu, 10 Jan 2013 13:49:22 +0100,
Jens Axboe wrote:
> 
> On 2013-01-10 11:21, Takashi Iwai wrote:
> > At Thu, 10 Jan 2013 09:23:48 +0100,
> > Jens Axboe wrote:
> >>
> >> Hi,
> >>
> >> I have a USB DAC that I use for music. Just upgraded my workstation from
> >> 3.7-rc7 to 3.8-rc3 this morning, and when the DAC is switched on, I get
> >> an immediate oops. I have attached the picture. It's crashing here:
> >>
> >> (gdb) l *snd_usb_pcm_prepare+0x153
> >> 0x161c is in snd_usb_pcm_prepare (sound/usb/pcm.c:470).
> >> 465		snd_pcm_format_t pcm_format)
> >> 466	{
> >> 467		int i;
> >> 468		int score = 0;
> >> 469	
> >> 470		if (fp->channels < 1) {
> >> 471			snd_printdd("%s: (fmt @%p) no channels\n", __func__, fp);
> >> 472			return 0;
> >> 473		}
> >> 474	
> >>
> >> 'fp' is clearly NULL.
> > 
> > Hmm... it's a function called only from a single place in
> > configure_sync_endpoint() and it's in list_for_each_entry():
> > 
> > 	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
> > 		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
> > 			subs->cur_rate, subs->pcm_format);
> > 
> > so it must be hardly NULL in normal situations.
> > The only scenario I can imagine right now is that the whole structure
> > is still uninitialized while it's called.  If so, it's a race problem,
> > and shouldn't be new to 3.8.  A patch like below might paper over the
> > problem although it's far from perfect.
> > 
> > 
> >> Let me know if you want a bisect.
> > 
> > Yeah, that'll be really helpful.
> 
> Here it is, it's from the one introducing the audioformat lookup.
> Confirmed that 3.8-rc3 with this backed out works fine, too. So should
> be fairly confident in that result.

Thanks!  I'm slowly understanding what's happening there.
Could you try the patch below?


Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: usb-audio: Fix NULL dereference by access to
 non-existing substream

The commit [0d9741c0: ALSA: usb-audio: sync ep init fix for
audioformat mismatch] introduced the correction of parameters to be
set for sync EP.  But since the new code assumes that the sync EP is
always paired with the data EP of another direction, it triggers Oops
when a device only with a single direction is used.

This patch adds a proper check of sync EP type and the presence of the
paired substream for avoiding the crash.

Reported-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/pcm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index c659310..21c0001 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -511,6 +511,17 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs)
 	struct snd_usb_substream *sync_subs =
 		&subs->stream->substream[subs->direction ^ 1];
 
+	if (subs->sync_endpoint->type != SND_USB_ENDPOINT_TYPE_DATA ||
+	    !subs->stream) {
+		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
+						  subs->pcm_format,
+						  subs->channels,
+						  subs->period_bytes,
+						  subs->cur_rate,
+						  subs->cur_audiofmt,
+						  NULL);
+	}
+
 	/* Try to find the best matching audioformat. */
 	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
 		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
-- 
1.8.1

Re: Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git

[Eldad Zack]

On Thu, 10 Jan 2013, Takashi Iwai wrote:

> At Thu, 10 Jan 2013 13:49:22 +0100,
> Jens Axboe wrote:
> > 
> > Here it is, it's from the one introducing the audioformat lookup.
> > Confirmed that 3.8-rc3 with this backed out works fine, too. So should
> > be fairly confident in that result.

> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: usb-audio: Fix NULL dereference by access to
>  non-existing substream
> 
> The commit [0d9741c0: ALSA: usb-audio: sync ep init fix for
> audioformat mismatch] introduced the correction of parameters to be
> set for sync EP.  But since the new code assumes that the sync EP is
> always paired with the data EP of another direction, it triggers Oops
> when a device only with a single direction is used.

Yes - sorry, I didn't consider this at all.

> This patch adds a proper check of sync EP type and the presence of the
> paired substream for avoiding the crash.
> 
> Reported-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/usb/pcm.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index c659310..21c0001 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -511,6 +511,17 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs)
>  	struct snd_usb_substream *sync_subs =
>  		&subs->stream->substream[subs->direction ^ 1];
>  
> +	if (subs->sync_endpoint->type != SND_USB_ENDPOINT_TYPE_DATA ||
> +	    !subs->stream) {
> +		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> +						  subs->pcm_format,
> +						  subs->channels,
> +						  subs->period_bytes,
> +						  subs->cur_rate,
> +						  subs->cur_audiofmt,
> +						  NULL);
> +	}
> +

I think you want to return here, no?



Jens, could you please send me the device's descriptors (lsusb -v)?
I'd like to take a closer look at this.

Cheers,
Eldad

Re: Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git

[Eldad Zack]

On Thu, 10 Jan 2013, Takashi Iwai wrote:

> At Thu, 10 Jan 2013 13:49:22 +0100,
> Jens Axboe wrote:
> > 
> > Here it is, it's from the one introducing the audioformat lookup.
> > Confirmed that 3.8-rc3 with this backed out works fine, too. So should
> > be fairly confident in that result.

> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: usb-audio: Fix NULL dereference by access to
>  non-existing substream
> 
> The commit [0d9741c0: ALSA: usb-audio: sync ep init fix for
> audioformat mismatch] introduced the correction of parameters to be
> set for sync EP.  But since the new code assumes that the sync EP is
> always paired with the data EP of another direction, it triggers Oops
> when a device only with a single direction is used.

Yes - sorry, I didn't consider this at all.

> This patch adds a proper check of sync EP type and the presence of the
> paired substream for avoiding the crash.
> 
> Reported-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/usb/pcm.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index c659310..21c0001 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -511,6 +511,17 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs)
>  	struct snd_usb_substream *sync_subs =
>  		&subs->stream->substream[subs->direction ^ 1];
>  
> +	if (subs->sync_endpoint->type != SND_USB_ENDPOINT_TYPE_DATA ||
> +	    !subs->stream) {
> +		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> +						  subs->pcm_format,
> +						  subs->channels,
> +						  subs->period_bytes,
> +						  subs->cur_rate,
> +						  subs->cur_audiofmt,
> +						  NULL);
> +	}
> +

I think you want to return here, no?



Jens, could you please send me the device's descriptors (lsusb -v)?
I'd like to take a closer look at this.

Cheers,
Eldad

Re: Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git

[Takashi Iwai]

At Thu, 10 Jan 2013 20:45:02 +0100 (CET),
Eldad Zack wrote:
> 
> 
> On Thu, 10 Jan 2013, Takashi Iwai wrote:
> 
> > At Thu, 10 Jan 2013 13:49:22 +0100,
> > Jens Axboe wrote:
> > > 
> > > Here it is, it's from the one introducing the audioformat lookup.
> > > Confirmed that 3.8-rc3 with this backed out works fine, too. So should
> > > be fairly confident in that result.
> 
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] ALSA: usb-audio: Fix NULL dereference by access to
> >  non-existing substream
> > 
> > The commit [0d9741c0: ALSA: usb-audio: sync ep init fix for
> > audioformat mismatch] introduced the correction of parameters to be
> > set for sync EP.  But since the new code assumes that the sync EP is
> > always paired with the data EP of another direction, it triggers Oops
> > when a device only with a single direction is used.
> 
> Yes - sorry, I didn't consider this at all.
> 
> > This patch adds a proper check of sync EP type and the presence of the
> > paired substream for avoiding the crash.
> > 
> > Reported-by: Jens Axboe <axboe@kernel.dk>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  sound/usb/pcm.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> > index c659310..21c0001 100644
> > --- a/sound/usb/pcm.c
> > +++ b/sound/usb/pcm.c
> > @@ -511,6 +511,17 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs)
> >  	struct snd_usb_substream *sync_subs =
> >  		&subs->stream->substream[subs->direction ^ 1];
> >  
> > +	if (subs->sync_endpoint->type != SND_USB_ENDPOINT_TYPE_DATA ||
> > +	    !subs->stream) {
> > +		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> > +						  subs->pcm_format,
> > +						  subs->channels,
> > +						  subs->period_bytes,
> > +						  subs->cur_rate,
> > +						  subs->cur_audiofmt,
> > +						  NULL);
> > +	}
> > +
> 
> I think you want to return here, no?

Ah, yes, good catch.  It was dropped during rebasing and rewriting.
Below is the revised patch.


thanks,

Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] ALSA: usb-audio: Fix NULL dereference by access to
 non-existing substream

The commit [0d9741c0: ALSA: usb-audio: sync ep init fix for
audioformat mismatch] introduced the correction of parameters to be
set for sync EP.  But since the new code assumes that the sync EP is
always paired with the data EP of another direction, it triggers Oops
when a device only with a single direction is used.

This patch adds a proper check of sync EP type and the presence of the
paired substream for avoiding the crash.

Reported-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/pcm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index c659310..d82e378 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -511,6 +511,16 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs)
 	struct snd_usb_substream *sync_subs =
 		&subs->stream->substream[subs->direction ^ 1];
 
+	if (subs->sync_endpoint->type != SND_USB_ENDPOINT_TYPE_DATA ||
+	    !subs->stream)
+		return snd_usb_endpoint_set_params(subs->sync_endpoint,
+						   subs->pcm_format,
+						   subs->channels,
+						   subs->period_bytes,
+						   subs->cur_rate,
+						   subs->cur_audiofmt,
+						   NULL);
+
 	/* Try to find the best matching audioformat. */
 	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
 		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
-- 
1.8.1

Re: Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git

[Jens Axboe]

On 2013-01-10 21:19, Takashi Iwai wrote:
> At Thu, 10 Jan 2013 20:45:02 +0100 (CET),
> Eldad Zack wrote:
>>
>>
>> On Thu, 10 Jan 2013, Takashi Iwai wrote:
>>
>>> At Thu, 10 Jan 2013 13:49:22 +0100,
>>> Jens Axboe wrote:
>>>>
>>>> Here it is, it's from the one introducing the audioformat lookup.
>>>> Confirmed that 3.8-rc3 with this backed out works fine, too. So should
>>>> be fairly confident in that result.
>>
>>> From: Takashi Iwai <tiwai@suse.de>
>>> Subject: [PATCH] ALSA: usb-audio: Fix NULL dereference by access to
>>>  non-existing substream
>>>
>>> The commit [0d9741c0: ALSA: usb-audio: sync ep init fix for
>>> audioformat mismatch] introduced the correction of parameters to be
>>> set for sync EP.  But since the new code assumes that the sync EP is
>>> always paired with the data EP of another direction, it triggers Oops
>>> when a device only with a single direction is used.
>>
>> Yes - sorry, I didn't consider this at all.
>>
>>> This patch adds a proper check of sync EP type and the presence of the
>>> paired substream for avoiding the crash.
>>>
>>> Reported-by: Jens Axboe <axboe@kernel.dk>
>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>> ---
>>>  sound/usb/pcm.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
>>> index c659310..21c0001 100644
>>> --- a/sound/usb/pcm.c
>>> +++ b/sound/usb/pcm.c
>>> @@ -511,6 +511,17 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs)
>>>  	struct snd_usb_substream *sync_subs =
>>>  		&subs->stream->substream[subs->direction ^ 1];
>>>  
>>> +	if (subs->sync_endpoint->type != SND_USB_ENDPOINT_TYPE_DATA ||
>>> +	    !subs->stream) {
>>> +		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
>>> +						  subs->pcm_format,
>>> +						  subs->channels,
>>> +						  subs->period_bytes,
>>> +						  subs->cur_rate,
>>> +						  subs->cur_audiofmt,
>>> +						  NULL);
>>> +	}
>>> +
>>
>> I think you want to return here, no?
> 
> Ah, yes, good catch.  It was dropped during rebasing and rewriting.
> Below is the revised patch.

Thanks, I'll give it a go and report back.

-- 
Jens Axboe

Re: Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git

[Jens Axboe]

On 2013-01-10 21:19, Takashi Iwai wrote:
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH v2] ALSA: usb-audio: Fix NULL dereference by access to
>  non-existing substream
> 
> The commit [0d9741c0: ALSA: usb-audio: sync ep init fix for
> audioformat mismatch] introduced the correction of parameters to be
> set for sync EP.  But since the new code assumes that the sync EP is
> always paired with the data EP of another direction, it triggers Oops
> when a device only with a single direction is used.
> 
> This patch adds a proper check of sync EP type and the presence of the
> paired substream for avoiding the crash.
> 
> Reported-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Confirmed, it works. You can add my tested-by too. Thanks Takashi!

-- 
Jens Axboe

Re: Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git

[Takashi Iwai]

At Fri, 11 Jan 2013 08:47:25 +0100,
Jens Axboe wrote:
> 
> On 2013-01-10 21:19, Takashi Iwai wrote:
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH v2] ALSA: usb-audio: Fix NULL dereference by access to
> >  non-existing substream
> > 
> > The commit [0d9741c0: ALSA: usb-audio: sync ep init fix for
> > audioformat mismatch] introduced the correction of parameters to be
> > set for sync EP.  But since the new code assumes that the sync EP is
> > always paired with the data EP of another direction, it triggers Oops
> > when a device only with a single direction is used.
> > 
> > This patch adds a proper check of sync EP type and the presence of the
> > paired substream for avoiding the crash.
> > 
> > Reported-by: Jens Axboe <axboe@kernel.dk>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Confirmed, it works. You can add my tested-by too. Thanks Takashi!

Thanks for quick testing.  Applied it now.


Takashi

Re: Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git

[Jens Axboe]

On 2013-01-10 20:45, Eldad Zack wrote:
> Jens, could you please send me the device's descriptors (lsusb -v)?
> I'd like to take a closer look at this.

Below.

Bus 006 Device 010: ID 22e8:dac1  
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               1.00
  bDeviceClass            0 (Defined at Interface level)
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0        64
  idVendor           0x22e8 
  idProduct          0xdac1 
  bcdDevice            3.21
  iManufacturer           1 Cambridge Audio 
  iProduct                2 Cambridge Audio USB Audio 1.0
  iSerial                 3 0000
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength          128
    bNumInterfaces          2
    bConfigurationValue     1
    iConfiguration          0 
    bmAttributes         0x80
      (Bus Powered)
    MaxPower              500mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           0
      bInterfaceClass         1 Audio
      bInterfaceSubClass      1 Control Device
      bInterfaceProtocol      0 
      iInterface              2 Cambridge Audio USB Audio 1.0
      AudioControl Interface Descriptor:
        bLength                 9
        bDescriptorType        36
        bDescriptorSubtype      1 (HEADER)
        bcdADC               1.00
        wTotalLength           40
        bInCollection           1
        baInterfaceNr( 0)       1
      AudioControl Interface Descriptor:
        bLength                12
        bDescriptorType        36
        bDescriptorSubtype      2 (INPUT_TERMINAL)
        bTerminalID             1
        wTerminalType      0x0101 USB Streaming
        bAssocTerminal          0
        bNrChannels             2
        wChannelConfig     0x0003
          Left Front (L)
          Right Front (R)
        iChannelNames           0 
        iTerminal               6 Cambridge Audio Audio 1.0 Output
      AudioControl Interface Descriptor:
        bLength                10
        bDescriptorType        36
        bDescriptorSubtype      6 (FEATURE_UNIT)
        bUnitID                10
        bSourceID               1
        bControlSize            1
        bmaControls( 0)      0x01
          Mute Control
        bmaControls( 1)      0x01
          Mute Control
        bmaControls( 2)      0x01
          Mute Control
        iFeature                5 Cambridge Audio USB 1.0 Audio In
      AudioControl Interface Descriptor:
        bLength                 9
        bDescriptorType        36
        bDescriptorSubtype      3 (OUTPUT_TERMINAL)
        bTerminalID             6
        wTerminalType      0x0301 Speaker
        bAssocTerminal          0
        bSourceID              10
        iTerminal               0 
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       0
      bNumEndpoints           0
      bInterfaceClass         1 Audio
      bInterfaceSubClass      2 Streaming
      bInterfaceProtocol      0 
      iInterface              4 Cambridge Audio USB 1.0 Audio Out
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       1
      bNumEndpoints           2
      bInterfaceClass         1 Audio
      bInterfaceSubClass      2 Streaming
      bInterfaceProtocol      0 
      iInterface              4 Cambridge Audio USB 1.0 Audio Out
      AudioStreaming Interface Descriptor:
        bLength                 7
        bDescriptorType        36
        bDescriptorSubtype      1 (AS_GENERAL)
        bTerminalLink           1
        bDelay                  1 frames
        wFormatTag              1 PCM
      AudioStreaming Interface Descriptor:
        bLength                20
        bDescriptorType        36
        bDescriptorSubtype      2 (FORMAT_TYPE)
        bFormatType             1 (FORMAT_TYPE_I)
        bNrChannels             2
        bSubframeSize           3
        bBitResolution         24
        bSamFreqType            4 Discrete
        tSamFreq[ 0]        44100
        tSamFreq[ 1]        48000
        tSamFreq[ 2]        88200
        tSamFreq[ 3]        96000
      Endpoint Descriptor:
        bLength                 9
        bDescriptorType         5
        bEndpointAddress     0x01  EP 1 OUT
        bmAttributes            5
          Transfer Type            Isochronous
          Synch Type               Asynchronous
          Usage Type               Data
        wMaxPacketSize     0x0246  1x 582 bytes
        bInterval               1
        bRefresh                0
        bSynchAddress         129
        AudioControl Endpoint Descriptor:
          bLength                 7
          bDescriptorType        37
          bDescriptorSubtype      1 (EP_GENERAL)
          bmAttributes         0x01
            Sampling Frequency
          bLockDelayUnits         2 Decoded PCM samples
          wLockDelay              0 Decoded PCM samples
      Endpoint Descriptor:
        bLength                 9
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0003  1x 3 bytes
        bInterval               1
        bRefresh                4
        bSynchAddress           0
Device Status:     0x0000
  (Bus Powered)


-- 
Jens Axboe