There are some usages of index pointer of list(w) which may not point to
the right entry when the required entry is not found and the list traversal
completes with index pointer pointing to the last entry. So, use w_found
flag to track the case where the entry is found.
Currently, When the condition (w->dapm != dapm) is true the loop continues
and when it is not then it compares the name strings and breaks out of the
loop if they match with w pointing to the right entry and it also breaks
out of loop if they didn't match by additionally setting w to NULL. But
what if the condition (w->dapm != dapm) is never false and the list
traversal completes with w pointing to last entry then usage of it after
the iter may not be correct. And there is no way to know whether the entry
is found. So, if we introduce w_found to track when the entry is found
then we can account for the case where the entry is not actually found and
the list traversal completes.
Fixes coccinelle error:
drivers/staging/greybus/audio_helper.c:135:7-8: ERROR:
invalid reference to the index variable of the iterator on line 127
Signed-off-by: Karthik Alapati <[email protected]>
---
drivers/staging/greybus/audio_helper.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
index 843760675876..7c04897a22a2 100644
--- a/drivers/staging/greybus/audio_helper.c
+++ b/drivers/staging/greybus/audio_helper.c
@@ -116,6 +116,7 @@ int gbaudio_dapm_free_controls(struct snd_soc_dapm_context *dapm,
{
int i;
struct snd_soc_dapm_widget *w, *next_w;
+ bool w_found = false;
#ifdef CONFIG_DEBUG_FS
struct dentry *parent = dapm->debugfs_dapm;
struct dentry *debugfs_w = NULL;
@@ -124,15 +125,18 @@ int gbaudio_dapm_free_controls(struct snd_soc_dapm_context *dapm,
mutex_lock(&dapm->card->dapm_mutex);
for (i = 0; i < num; i++) {
/* below logic can be optimized to identify widget pointer */
+ w_found = false
list_for_each_entry_safe(w, next_w, &dapm->card->widgets,
list) {
if (w->dapm != dapm)
continue;
- if (!strcmp(w->name, widget->name))
+ if (!strcmp(w->name, widget->name)) {
+ w_found = true;
break;
+ }
w = NULL;
}
- if (!w) {
+ if (!w_found) {
dev_err(dapm->dev, "%s: widget not found\n",
widget->name);
widget++;
--
2.36.1
On Thu, Jul 07, 2022 at 03:59:54PM +0530, Karthik Alapati wrote:
> There are some usages of index pointer of list(w) which may not point to
> the right entry when the required entry is not found and the list traversal
> completes with index pointer pointing to the last entry. So, use w_found
> flag to track the case where the entry is found.
That is already being done here with the use of the w variable.
Look at commit 80c968a04a38 ("staging: greybus: audio: fix loop cursor
use after iteration") and then d2b47721a100 ("staging: greybus: audio:
replace safe list iteration").
> diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
> index 843760675876..7c04897a22a2 100644
> --- a/drivers/staging/greybus/audio_helper.c
> +++ b/drivers/staging/greybus/audio_helper.c
> @@ -116,6 +116,7 @@ int gbaudio_dapm_free_controls(struct snd_soc_dapm_context *dapm,
> {
> int i;
> struct snd_soc_dapm_widget *w, *next_w;
> + bool w_found = false;
> #ifdef CONFIG_DEBUG_FS
> struct dentry *parent = dapm->debugfs_dapm;
> struct dentry *debugfs_w = NULL;
> @@ -124,15 +125,18 @@ int gbaudio_dapm_free_controls(struct snd_soc_dapm_context *dapm,
> mutex_lock(&dapm->card->dapm_mutex);
> for (i = 0; i < num; i++) {
> /* below logic can be optimized to identify widget pointer */
> + w_found = false
> list_for_each_entry_safe(w, next_w, &dapm->card->widgets,
> list) {
You are working off of an old kernel version here, please see the above
commits which do not seem to be in your tree. Always work on linux-next
for issues so that you do not duplicate work that others have completed.
thanks,
greg k-h
On Thu, Jul 07, 2022 at 03:59:54PM +0530, Karthik Alapati wrote:
> There are some usages of index pointer of list(w) which may not point to
> the right entry when the required entry is not found and the list traversal
> completes with index pointer pointing to the last entry. So, use w_found
> flag to track the case where the entry is found.
>
> Currently, When the condition (w->dapm != dapm) is true the loop continues
> and when it is not then it compares the name strings and breaks out of the
> loop if they match with w pointing to the right entry and it also breaks
> out of loop if they didn't match by additionally setting w to NULL. But
> what if the condition (w->dapm != dapm) is never false and the list
> traversal completes with w pointing to last entry then usage of it after
> the iter may not be correct. And there is no way to know whether the entry
> is found. So, if we introduce w_found to track when the entry is found
> then we can account for the case where the entry is not actually found and
> the list traversal completes.
>
> Fixes coccinelle error:
> drivers/staging/greybus/audio_helper.c:135:7-8: ERROR:
> invalid reference to the index variable of the iterator on line 127
>
> Signed-off-by: Karthik Alapati <[email protected]>
> ---
Already fixed a month ago. Please always work against staging-next or
linux-next.
regards,
dan carpenter
Hi Karthik,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on v5.19-rc5]
[also build test WARNING on linus/master]
[cannot apply to staging/staging-testing next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Karthik-Alapati/staging-greybus-don-t-use-index-pointer-after-iter/20220707-183311
base: 88084a3df1672e131ddc1b4e39eeacfd39864acf
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220708/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/bc295082ef055003c6018b57d3c56c5aefcb65c5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Karthik-Alapati/staging-greybus-don-t-use-index-pointer-after-iter/20220707-183311
git checkout bc295082ef055003c6018b57d3c56c5aefcb65c5
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/staging/greybus/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/staging/greybus/audio_helper.c: In function 'gbaudio_dapm_free_controls':
drivers/staging/greybus/audio_helper.c:128:32: error: expected ';' before 'for'
128 | w_found = false
| ^
| ;
>> drivers/staging/greybus/audio_helper.c:119:14: warning: variable 'w_found' set but not used [-Wunused-but-set-variable]
119 | bool w_found = false;
| ^~~~~~~
>> drivers/staging/greybus/audio_helper.c:118:41: warning: unused variable 'next_w' [-Wunused-variable]
118 | struct snd_soc_dapm_widget *w, *next_w;
| ^~~~~~
vim +/w_found +119 drivers/staging/greybus/audio_helper.c
112
113 int gbaudio_dapm_free_controls(struct snd_soc_dapm_context *dapm,
114 const struct snd_soc_dapm_widget *widget,
115 int num)
116 {
117 int i;
> 118 struct snd_soc_dapm_widget *w, *next_w;
> 119 bool w_found = false;
120 #ifdef CONFIG_DEBUG_FS
121 struct dentry *parent = dapm->debugfs_dapm;
122 struct dentry *debugfs_w = NULL;
123 #endif
124
125 mutex_lock(&dapm->card->dapm_mutex);
126 for (i = 0; i < num; i++) {
127 /* below logic can be optimized to identify widget pointer */
128 w_found = false
129 list_for_each_entry_safe(w, next_w, &dapm->card->widgets,
130 list) {
131 if (w->dapm != dapm)
132 continue;
133 if (!strcmp(w->name, widget->name)) {
134 w_found = true;
135 break;
136 }
137 w = NULL;
138 }
139 if (!w_found) {
140 dev_err(dapm->dev, "%s: widget not found\n",
141 widget->name);
142 widget++;
143 continue;
144 }
145 widget++;
146 #ifdef CONFIG_DEBUG_FS
147 if (!parent)
148 debugfs_w = debugfs_lookup(w->name, parent);
149 debugfs_remove(debugfs_w);
150 debugfs_w = NULL;
151 #endif
152 gbaudio_dapm_free_widget(w);
153 }
154 mutex_unlock(&dapm->card->dapm_mutex);
155 return 0;
156 }
157
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Thu, Jul 07, 2022 at 03:59:54PM +0530, Karthik Alapati wrote:
> There are some usages of index pointer of list(w) which may not point to
> the right entry when the required entry is not found and the list traversal
> completes with index pointer pointing to the last entry. So, use w_found
> flag to track the case where the entry is found.
>
> Currently, When the condition (w->dapm != dapm) is true the loop continues
> and when it is not then it compares the name strings and breaks out of the
> loop if they match with w pointing to the right entry and it also breaks
> out of loop if they didn't match by additionally setting w to NULL. But
> what if the condition (w->dapm != dapm) is never false and the list
> traversal completes with w pointing to last entry then usage of it after
> the iter may not be correct. And there is no way to know whether the entry
> is found. So, if we introduce w_found to track when the entry is found
> then we can account for the case where the entry is not actually found and
> the list traversal completes.
>
> Fixes coccinelle error:
> drivers/staging/greybus/audio_helper.c:135:7-8: ERROR:
> invalid reference to the index variable of the iterator on line 127
>
> Signed-off-by: Karthik Alapati <[email protected]>
Also, always at the very least, test-build your patches to ensure that
they pass that step. This patch fails to build :(
greg k-h
Hi Karthik,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.19-rc5]
[also build test ERROR on linus/master]
[cannot apply to staging/staging-testing next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Karthik-Alapati/staging-greybus-don-t-use-index-pointer-after-iter/20220707-183311
base: 88084a3df1672e131ddc1b4e39eeacfd39864acf
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220708/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/bc295082ef055003c6018b57d3c56c5aefcb65c5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Karthik-Alapati/staging-greybus-don-t-use-index-pointer-after-iter/20220707-183311
git checkout bc295082ef055003c6018b57d3c56c5aefcb65c5
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/staging/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/staging/greybus/audio_helper.c: In function 'gbaudio_dapm_free_controls':
>> drivers/staging/greybus/audio_helper.c:128:32: error: expected ';' before 'for'
128 | w_found = false
| ^
| ;
drivers/staging/greybus/audio_helper.c:119:14: warning: variable 'w_found' set but not used [-Wunused-but-set-variable]
119 | bool w_found = false;
| ^~~~~~~
drivers/staging/greybus/audio_helper.c:118:41: warning: unused variable 'next_w' [-Wunused-variable]
118 | struct snd_soc_dapm_widget *w, *next_w;
| ^~~~~~
vim +128 drivers/staging/greybus/audio_helper.c
124
125 mutex_lock(&dapm->card->dapm_mutex);
126 for (i = 0; i < num; i++) {
127 /* below logic can be optimized to identify widget pointer */
> 128 w_found = false
129 list_for_each_entry_safe(w, next_w, &dapm->card->widgets,
130 list) {
131 if (w->dapm != dapm)
132 continue;
133 if (!strcmp(w->name, widget->name)) {
134 w_found = true;
135 break;
136 }
137 w = NULL;
138 }
139 if (!w_found) {
140 dev_err(dapm->dev, "%s: widget not found\n",
141 widget->name);
142 widget++;
143 continue;
144 }
145 widget++;
146 #ifdef CONFIG_DEBUG_FS
147 if (!parent)
148 debugfs_w = debugfs_lookup(w->name, parent);
149 debugfs_remove(debugfs_w);
150 debugfs_w = NULL;
151 #endif
152 gbaudio_dapm_free_widget(w);
153 }
154 mutex_unlock(&dapm->card->dapm_mutex);
155 return 0;
156 }
157
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Karthik,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.19-rc5]
[also build test ERROR on linus/master]
[cannot apply to staging/staging-testing next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Karthik-Alapati/staging-greybus-don-t-use-index-pointer-after-iter/20220707-183311
base: 88084a3df1672e131ddc1b4e39eeacfd39864acf
config: arm-randconfig-r014-20220707 (https://download.01.org/0day-ci/archive/20220708/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 562c3467a6738aa89203f72fc1d1343e5baadf3c)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/bc295082ef055003c6018b57d3c56c5aefcb65c5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Karthik-Alapati/staging-greybus-don-t-use-index-pointer-after-iter/20220707-183311
git checkout bc295082ef055003c6018b57d3c56c5aefcb65c5
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/staging/greybus/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/staging/greybus/audio_helper.c:128:18: error: expected ';' after expression
w_found = false
^
;
1 error generated.
vim +128 drivers/staging/greybus/audio_helper.c
124
125 mutex_lock(&dapm->card->dapm_mutex);
126 for (i = 0; i < num; i++) {
127 /* below logic can be optimized to identify widget pointer */
> 128 w_found = false
129 list_for_each_entry_safe(w, next_w, &dapm->card->widgets,
130 list) {
131 if (w->dapm != dapm)
132 continue;
133 if (!strcmp(w->name, widget->name)) {
134 w_found = true;
135 break;
136 }
137 w = NULL;
138 }
139 if (!w_found) {
140 dev_err(dapm->dev, "%s: widget not found\n",
141 widget->name);
142 widget++;
143 continue;
144 }
145 widget++;
146 #ifdef CONFIG_DEBUG_FS
147 if (!parent)
148 debugfs_w = debugfs_lookup(w->name, parent);
149 debugfs_remove(debugfs_w);
150 debugfs_w = NULL;
151 #endif
152 gbaudio_dapm_free_widget(w);
153 }
154 mutex_unlock(&dapm->card->dapm_mutex);
155 return 0;
156 }
157
--
0-DAY CI Kernel Test Service
https://01.org/lkp