* ALSA core info race condition @ 2017-06-27 19:12 b_lkasam 2017-06-28 5:36 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: b_lkasam @ 2017-06-27 19:12 UTC (permalink / raw) To: alsa-devel; +Cc: lkasam, b_lkasam hi ALSA team, there is a race condition in below API when accessing list API. In file sound/core/info.c: Added below patch to avoid list access of same parent node by two threads at same time causing list_debug crash. diff --git a/sound/core/info.c b/sound/core/info.c index b5158b5..c1fd671 100644 --- a/sound/core/info.c +++ b/sound/core/info.c @@ -747,8 +747,11 @@ snd_info_create_entry(const char *name, struct snd_info_entry *parent) INIT_LIST_HEAD(&entry->children); INIT_LIST_HEAD(&entry->list); entry->parent = parent; - if (parent) + if (parent) { + mutex_lock(&parent->access); list_add_tail(&entry->list, &parent->children); + mutex_unlock(&parent->access); + } return entry; } Please check above logic looks fine, and help comment accordingly. Thanks Kasam ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: ALSA core info race condition 2017-06-27 19:12 ALSA core info race condition b_lkasam @ 2017-06-28 5:36 ` Takashi Iwai 2017-06-28 5:58 ` b_lkasam 0 siblings, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2017-06-28 5:36 UTC (permalink / raw) To: b_lkasam; +Cc: alsa-devel, lkasam On Tue, 27 Jun 2017 21:12:18 +0200, b_lkasam@codeaurora.org wrote: > > hi ALSA team, > there is a race condition in below API when accessing list API. > > In file sound/core/info.c: > > Added below patch to avoid list access of same parent node > by two threads at same time causing list_debug crash. > > diff --git a/sound/core/info.c b/sound/core/info.c > index b5158b5..c1fd671 100644 > --- a/sound/core/info.c > +++ b/sound/core/info.c > @@ -747,8 +747,11 @@ snd_info_create_entry(const char *name, struct > snd_info_entry *parent) > INIT_LIST_HEAD(&entry->children); > INIT_LIST_HEAD(&entry->list); > entry->parent = parent; > - if (parent) > + if (parent) { > + mutex_lock(&parent->access); > list_add_tail(&entry->list, &parent->children); > + mutex_unlock(&parent->access); > + } > return entry; > } > > Please check above logic looks fine, and help comment accordingly. Have you ever got the actual crash? The function is supposed to be called only at probing, and its link base is the card object, so it's never called concurrently or the concurrency should be managed in the caller side. Your "fix" looks OK, but it's likely superfluous from the actual usage. Still it might be safer to add a protection, though. thanks, Takashi ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ALSA core info race condition 2017-06-28 5:36 ` Takashi Iwai @ 2017-06-28 5:58 ` b_lkasam 2017-06-28 6:26 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: b_lkasam @ 2017-06-28 5:58 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, lkasam On 2017-06-28 11:06, Takashi Iwai wrote: > On Tue, 27 Jun 2017 21:12:18 +0200, > b_lkasam@codeaurora.org wrote: >> >> hi ALSA team, >> there is a race condition in below API when accessing list API. >> >> In file sound/core/info.c: >> >> Added below patch to avoid list access of same parent node >> by two threads at same time causing list_debug crash. >> >> diff --git a/sound/core/info.c b/sound/core/info.c >> index b5158b5..c1fd671 100644 >> --- a/sound/core/info.c >> +++ b/sound/core/info.c >> @@ -747,8 +747,11 @@ snd_info_create_entry(const char *name, struct >> snd_info_entry *parent) >> INIT_LIST_HEAD(&entry->children); >> INIT_LIST_HEAD(&entry->list); >> entry->parent = parent; >> - if (parent) >> + if (parent) { >> + mutex_lock(&parent->access); >> list_add_tail(&entry->list, &parent->children); >> + mutex_unlock(&parent->access); >> + } >> return entry; >> } >> >> Please check above logic looks fine, and help comment accordingly. > > Have you ever got the actual crash? > The function is supposed to be called only at probing, and its link > base is the card object, so it's never called concurrently or the > concurrency should be managed in the caller side. > > Your "fix" looks OK, but it's likely superfluous from the actual > usage. Still it might be safer to add a protection, though. > > > thanks, > > Takashi Yes Takashi, we found this crash happened on Qualcomm platform. And as mentioned by you...it is used at bootup probe but from two different contexts, Even if I try to manage at client level ...it will be only workaround. so actual fix should be the patch I shared to avoid any similar issues in future. thanks Laxminath ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ALSA core info race condition 2017-06-28 5:58 ` b_lkasam @ 2017-06-28 6:26 ` Takashi Iwai 0 siblings, 0 replies; 4+ messages in thread From: Takashi Iwai @ 2017-06-28 6:26 UTC (permalink / raw) To: b_lkasam; +Cc: alsa-devel, lkasam On Wed, 28 Jun 2017 07:58:07 +0200, b_lkasam@codeaurora.org wrote: > > On 2017-06-28 11:06, Takashi Iwai wrote: > > On Tue, 27 Jun 2017 21:12:18 +0200, > > b_lkasam@codeaurora.org wrote: > >> > >> hi ALSA team, > >> there is a race condition in below API when accessing list API. > >> > >> In file sound/core/info.c: > >> > >> Added below patch to avoid list access of same parent node > >> by two threads at same time causing list_debug crash. > >> > >> diff --git a/sound/core/info.c b/sound/core/info.c > >> index b5158b5..c1fd671 100644 > >> --- a/sound/core/info.c > >> +++ b/sound/core/info.c > >> @@ -747,8 +747,11 @@ snd_info_create_entry(const char *name, struct > >> snd_info_entry *parent) > >> INIT_LIST_HEAD(&entry->children); > >> INIT_LIST_HEAD(&entry->list); > >> entry->parent = parent; > >> - if (parent) > >> + if (parent) { > >> + mutex_lock(&parent->access); > >> list_add_tail(&entry->list, &parent->children); > >> + mutex_unlock(&parent->access); > >> + } > >> return entry; > >> } > >> > >> Please check above logic looks fine, and help comment accordingly. > > > > Have you ever got the actual crash? > > The function is supposed to be called only at probing, and its link > > base is the card object, so it's never called concurrently or the > > concurrency should be managed in the caller side. > > > > Your "fix" looks OK, but it's likely superfluous from the actual > > usage. Still it might be safer to add a protection, though. > > > > > > thanks, > > > > Takashi > > > Yes Takashi, we found this crash happened on Qualcomm platform. > And as mentioned by you...it is used at bootup probe but from two > different contexts, > Even if I try to manage at client level ...it will be only > workaround. so actual fix > should be the patch I shared to avoid any similar issues in future. Hrm, it already smells suspicious. The situation where the proc file creation on the same root is called concurrently implies the racy registration, not only about the proc... But it makes no sense to discuss without the actual code, and the suggested change looks OK, as said. So please submit a proper patch in the usual manner. thanks, Takashi ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-28 6:26 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-06-27 19:12 ALSA core info race condition b_lkasam 2017-06-28 5:36 ` Takashi Iwai 2017-06-28 5:58 ` b_lkasam 2017-06-28 6:26 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.