[PATCH] staging: greybus: don't use index pointer after iter

[PATCH] staging: greybus: don't use index pointer after iter

[Karthik Alapati]

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++;
-- 
2.36.1

Re: [PATCH] staging: greybus: don't use index pointer after iter

[Greg Kroah-Hartman]

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

Re: [PATCH] staging: greybus: don't use index pointer after iter

[Dan Carpenter]

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

Re: [PATCH] staging: greybus: don't use index pointer after iter

[kernel test robot]

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/202207080123.R3ePp3SE-lkp@intel.com/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 <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	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH] staging: greybus: don't use index pointer after iter

[Greg Kroah-Hartman]

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

Re: [PATCH] staging: greybus: don't use index pointer after iter

[kernel test robot]

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/202207080535.tr2i6TxR-lkp@intel.com/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 <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	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH] staging: greybus: don't use index pointer after iter

[kernel test robot]

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/202207081340.OdVTd0BF-lkp@intel.com/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 <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	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp