[PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable

[PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable

[Jakob Koschel]

If the list does not exit early then data == NULL and 'module' does not
point to a valid list element.
Using 'module' in such a case is not valid and was therefore removed.

In preparation to limit the scope of the list iterator to the list
traversal loop, use a dedicated pointer pointing to the found element [1].

Link: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/staging/greybus/audio_codec.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index b589cf6b1d03..0f50d1e51e2c 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -497,7 +497,7 @@ static int gbcodec_prepare(struct snd_pcm_substream *substream,
 			   struct snd_soc_dai *dai)
 {
 	int ret;
-	struct gbaudio_module_info *module;
+	struct gbaudio_module_info *module = NULL, *iter;
 	struct gbaudio_data_connection *data;
 	struct gb_bundle *bundle;
 	struct gbaudio_codec_info *codec = dev_get_drvdata(dai->dev);
@@ -511,11 +511,13 @@ static int gbcodec_prepare(struct snd_pcm_substream *substream,
 		return -ENODEV;
 	}

-	list_for_each_entry(module, &codec->module_list, list) {
+	list_for_each_entry(iter, &codec->module_list, list) {
 		/* find the dai */
-		data = find_data(module, dai->id);
-		if (data)
+		data = find_data(iter, dai->id);
+		if (data) {
+			module = iter;
 			break;
+		}
 	}
 	if (!data) {
 		dev_err(dai->dev, "DATA connection missing\n");
@@ -563,7 +565,7 @@ static int gbcodec_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
 {
 	int ret;
 	struct gbaudio_data_connection *data;
-	struct gbaudio_module_info *module;
+	struct gbaudio_module_info *module = NULL, *iter;
 	struct gb_bundle *bundle;
 	struct gbaudio_codec_info *codec = dev_get_drvdata(dai->dev);
 	struct gbaudio_stream_params *params;
@@ -592,15 +594,17 @@ static int gbcodec_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
 		return ret;
 	}

-	list_for_each_entry(module, &codec->module_list, list) {
+	list_for_each_entry(iter, &codec->module_list, list) {
 		/* find the dai */
-		data = find_data(module, dai->id);
-		if (data)
+		data = find_data(iter, dai->id);
+		if (data) {
+			module = iter;
 			break;
+		}
 	}
 	if (!data) {
-		dev_err(dai->dev, "%s:%s DATA connection missing\n",
-			dai->name, module->name);
+		dev_err(dai->dev, "%s DATA connection missing\n",
+			dai->name);
 		mutex_unlock(&codec->lock);
 		return -ENODEV;
 	}

base-commit: 34e047aa16c0123bbae8e2f6df33e5ecc1f56601
--
2.25.1

Re: [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable

[Dan Carpenter]

The subject says that it fixes a bug but it does not.

On Sat, Mar 19, 2022 at 09:20:58PM +0100, Jakob Koschel wrote:
> If the list does not exit early then data == NULL and 'module' does not
> point to a valid list element.
> Using 'module' in such a case is not valid and was therefore removed.

This paragraph is confusing jumble words.  Just say: "This code is fine".

> 
> In preparation to limit the scope of the list iterator to the list
> traversal loop, use a dedicated pointer pointing to the found element [1].

This paragraph is the information we need.  Just add something like
"This patch has no effect on runtime".

regards,
dan carpenter

Re: [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable

[Jakob Koschel]

> On 21. Mar 2022, at 09:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> The subject says that it fixes a bug but it does not.

Thank you for your review!

I don't agree that this doesn't fix a bug:

> +		}
> 	}
> 	if (!data) {
> -		dev_err(dai->dev, "%s:%s DATA connection missing\n",
> -			dai->name, module->name);

Using 'module' when data == NULL is *guaranteed* to be a type confused
bogus pointer. It fundamentally can never be correct.

If I should still change the wording please let me know.

> +		dev_err(dai->dev, "%s DATA connection missing\n",
> +			dai->name);
> 		mutex_unlock(&codec->lock);
> 		return -ENODEV;
> 	}


> 
> On Sat, Mar 19, 2022 at 09:20:58PM +0100, Jakob Koschel wrote:
>> If the list does not exit early then data == NULL and 'module' does not
>> point to a valid list element.
>> Using 'module' in such a case is not valid and was therefore removed.
> 
> This paragraph is confusing jumble words.  Just say: "This code is fine".
> 
>> 
>> In preparation to limit the scope of the list iterator to the list
>> traversal loop, use a dedicated pointer pointing to the found element [1].
> 
> This paragraph is the information we need.  Just add something like
> "This patch has no effect on runtime".

As mentioned above, this code effects runtime (in one out of the two cases).

> 
> regards,
> dan carpenter

Thanks,
Jakob

Re: [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable

[Dan Carpenter]

On Mon, Mar 21, 2022 at 10:06:13AM +0100, Jakob Koschel wrote:
> 
> > On 21. Mar 2022, at 09:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > 
> > The subject says that it fixes a bug but it does not.
> 
> Thank you for your review!
> 
> I don't agree that this doesn't fix a bug:
> 
> > +		}
> > 	}
> > 	if (!data) {
> > -		dev_err(dai->dev, "%s:%s DATA connection missing\n",
> > -			dai->name, module->name);
> 
> Using 'module' when data == NULL is *guaranteed* to be a type confused
> bogus pointer. It fundamentally can never be correct.

Ah.  I did not read all the way to the end of the patch.

The bugfix needs to be sent as it's own patch.  Just the one liner.  It
needs a fixes tag as well.

[PATCH] staging: greybus: fix Oops in error message

The "module" pointer is invalid here.  It's the list iterator and we
exited the loop without finding a valid entry.

Fixes: 6dd67645f22c ("greybus: audio: Use single codec driver registration")
Signed-off-by: You

 	if (!data) {
-		dev_err(dai->dev, "%s:%s DATA connection missing\n",
-			dai->name, module->name);
+		dev_err(dai->dev, "%s DATA connection missing\n",
+			dai->name);
 		mutex_unlock(&codec->lock);

We're happy to apply the other stuff as well, but we don't mix cleanups
and bug fixes like that.

regards,
dan carpenter

Re: [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable

[Jakob Koschel]

> On 21. Mar 2022, at 10:21, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> On Mon, Mar 21, 2022 at 10:06:13AM +0100, Jakob Koschel wrote:
>> 
>>> On 21. Mar 2022, at 09:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>> 
>>> The subject says that it fixes a bug but it does not.
>> 
>> Thank you for your review!
>> 
>> I don't agree that this doesn't fix a bug:
>> 
>>> +		}
>>> 	}
>>> 	if (!data) {
>>> -		dev_err(dai->dev, "%s:%s DATA connection missing\n",
>>> -			dai->name, module->name);
>> 
>> Using 'module' when data == NULL is *guaranteed* to be a type confused
>> bogus pointer. It fundamentally can never be correct.
> 
> Ah.  I did not read all the way to the end of the patch.
> 
> The bugfix needs to be sent as it's own patch.  Just the one liner.  It
> needs a fixes tag as well.
> 
> [PATCH] staging: greybus: fix Oops in error message
> 
> The "module" pointer is invalid here.  It's the list iterator and we
> exited the loop without finding a valid entry.
> 
> Fixes: 6dd67645f22c ("greybus: audio: Use single codec driver registration")
> Signed-off-by: You
> 
> 	if (!data) {
> -		dev_err(dai->dev, "%s:%s DATA connection missing\n",
> -			dai->name, module->name);
> +		dev_err(dai->dev, "%s DATA connection missing\n",
> +			dai->name);
> 		mutex_unlock(&codec->lock);
> 
> We're happy to apply the other stuff as well, but we don't mix cleanups
> and bug fixes like that.

ok great, I'll separate and resubmit both. Thanks!

> 
> regards,
> dan carpenter

	Jakob

Re: [greybus-dev] Re: [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable

[Alex Elder]

On 3/21/22 4:27 AM, Jakob Koschel wrote:
> 

I just released some messages that were marked as spam.  So
this looks to me like it's already been seen on the list.
I'm not sure why this happens but if it seems like deja vu,
you're not imagining things.  Please know that this could
happen from time to time, but I'll see if I can find out
how to avoid it.

					-Alex

>> On 21. Mar 2022, at 10:21, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>
>> On Mon, Mar 21, 2022 at 10:06:13AM +0100, Jakob Koschel wrote:
>>>
>>>> On 21. Mar 2022, at 09:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>>>
>>>> The subject says that it fixes a bug but it does not.
>>>
>>> Thank you for your review!
>>>
>>> I don't agree that this doesn't fix a bug:
>>>
>>>> +		}
>>>> 	}
>>>> 	if (!data) {
>>>> -		dev_err(dai->dev, "%s:%s DATA connection missing\n",
>>>> -			dai->name, module->name);
>>>
>>> Using 'module' when data == NULL is *guaranteed* to be a type confused
>>> bogus pointer. It fundamentally can never be correct.
>>
>> Ah.  I did not read all the way to the end of the patch.
>>
>> The bugfix needs to be sent as it's own patch.  Just the one liner.  It
>> needs a fixes tag as well.
>>
>> [PATCH] staging: greybus: fix Oops in error message
>>
>> The "module" pointer is invalid here.  It's the list iterator and we
>> exited the loop without finding a valid entry.
>>
>> Fixes: 6dd67645f22c ("greybus: audio: Use single codec driver registration")
>> Signed-off-by: You
>>
>> 	if (!data) {
>> -		dev_err(dai->dev, "%s:%s DATA connection missing\n",
>> -			dai->name, module->name);
>> +		dev_err(dai->dev, "%s DATA connection missing\n",
>> +			dai->name);
>> 		mutex_unlock(&codec->lock);
>>
>> We're happy to apply the other stuff as well, but we don't mix cleanups
>> and bug fixes like that.
> 
> ok great, I'll separate and resubmit both. Thanks!
> 
>>
>> regards,
>> dan carpenter
> 
> 	Jakob
> 
> _______________________________________________
> greybus-dev mailing list -- greybus-dev@lists.linaro.org
> To unsubscribe send an email to greybus-dev-leave@lists.linaro.org