Re: [alsa-devel] Oops in sound/usb/pcm.c:match_endpoint_audioformats() in current -git
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,
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
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,
participants (2)
-
Eldad Zack -
Takashi Iwai