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 mail@karthek.com --- 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++;
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
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 mail@karthek.com
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-greyb... base: 88084a3df1672e131ddc1b4e39eeacfd39864acf config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220708/202207080123.R3ePp3SE-lkp@i...) 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/bc295082ef055003c6018b57d3c56c... 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 lkp@intel.com
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
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 mail@karthek.com
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-greyb... base: 88084a3df1672e131ddc1b4e39eeacfd39864acf config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220708/202207080535.tr2i6TxR-lkp@i...) 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/bc295082ef055003c6018b57d3c56c... 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 lkp@intel.com
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
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-greyb... base: 88084a3df1672e131ddc1b4e39eeacfd39864acf config: arm-randconfig-r014-20220707 (https://download.01.org/0day-ci/archive/20220708/202207081340.OdVTd0BF-lkp@i...) 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/bc295082ef055003c6018b57d3c56c... 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 lkp@intel.com
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
-
Dan Carpenter -
Greg Kroah-Hartman -
Karthik Alapati -
kernel test robot