* [v3 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning.
@ 2024-02-14 14:01 ` Aiswarya Cyriac
0 siblings, 0 replies; 6+ messages in thread
From: Aiswarya Cyriac @ 2024-02-14 14:01 UTC (permalink / raw)
To: mst, jasowang, perex, tiwai, linux-kernel, alsa-devel,
virtualization, virtio-dev
Cc: Aiswarya Cyriac, Anton Yakovlev, coverity-bot
This commit fixes the following warning when building virtio_snd driver.
"
*** CID 1583619: Uninitialized variables (UNINIT)
sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op()
288
289 break;
290 }
291
292 kfree(tlv);
293
vvv CID 1583619: Uninitialized variables (UNINIT)
vvv Using uninitialized value "rc".
294 return rc;
295 }
296
297 /**
298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type.
299 * @snd: VirtIO sound device.
"
This warning is caused by the absence of the "default" branch in the
switch-block, and is a false positive because the kernel calls
virtsnd_kctl_tlv_op() only with values for op_flag processed in
this block.
Also, this commit unifies the cleanup path for all possible control
paths in the callback function.
Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com>
Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com>
Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1583619 ("Uninitialized variables")
Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls")
---
sound/virtio/virtio_kctl.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
index 0c6ac74aca1e..40606eb381af 100644
--- a/sound/virtio/virtio_kctl.c
+++ b/sound/virtio/virtio_kctl.c
@@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
tlv = kzalloc(size, GFP_KERNEL);
if (!tlv) {
- virtsnd_ctl_msg_unref(msg);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto on_cleanup;
}
sg_init_one(&sg, tlv, size);
@@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
case SNDRV_CTL_TLV_OP_READ:
hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ);
+ /* Since virtsnd_ctl_msg_send() drops the reference, we increase
+ * the counter to be consistent with the on_cleanup path.
+ */
+ virtsnd_ctl_msg_ref(msg);
+
rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false);
if (!rc) {
if (copy_to_user(utlv, tlv, size))
@@ -281,14 +286,26 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
hdr->hdr.code =
cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND);
- if (copy_from_user(tlv, utlv, size))
+ if (copy_from_user(tlv, utlv, size)) {
rc = -EFAULT;
- else
+ } else {
+ /* Same as the comment above */
+ virtsnd_ctl_msg_ref(msg);
+
rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false);
+ }
+
+ break;
+ default:
+ rc = -EINVAL;
+ WARN_ON(1);
break;
}
+on_cleanup:
+ virtsnd_ctl_msg_unref(msg);
+
kfree(tlv);
return rc;
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [virtio-dev] [v3 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. @ 2024-02-14 14:01 ` Aiswarya Cyriac 0 siblings, 0 replies; 6+ messages in thread From: Aiswarya Cyriac @ 2024-02-14 14:01 UTC (permalink / raw) To: mst, jasowang, perex, tiwai, linux-kernel, alsa-devel, virtualization, virtio-dev Cc: Aiswarya Cyriac, Anton Yakovlev, coverity-bot This commit fixes the following warning when building virtio_snd driver. " *** CID 1583619: Uninitialized variables (UNINIT) sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() 288 289 break; 290 } 291 292 kfree(tlv); 293 vvv CID 1583619: Uninitialized variables (UNINIT) vvv Using uninitialized value "rc". 294 return rc; 295 } 296 297 /** 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. 299 * @snd: VirtIO sound device. " This warning is caused by the absence of the "default" branch in the switch-block, and is a false positive because the kernel calls virtsnd_kctl_tlv_op() only with values for op_flag processed in this block. Also, this commit unifies the cleanup path for all possible control paths in the callback function. Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> Addresses-Coverity-ID: 1583619 ("Uninitialized variables") Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") --- sound/virtio/virtio_kctl.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c index 0c6ac74aca1e..40606eb381af 100644 --- a/sound/virtio/virtio_kctl.c +++ b/sound/virtio/virtio_kctl.c @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, tlv = kzalloc(size, GFP_KERNEL); if (!tlv) { - virtsnd_ctl_msg_unref(msg); - return -ENOMEM; + rc = -ENOMEM; + goto on_cleanup; } sg_init_one(&sg, tlv, size); @@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, case SNDRV_CTL_TLV_OP_READ: hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ); + /* Since virtsnd_ctl_msg_send() drops the reference, we increase + * the counter to be consistent with the on_cleanup path. + */ + virtsnd_ctl_msg_ref(msg); + rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false); if (!rc) { if (copy_to_user(utlv, tlv, size)) @@ -281,14 +286,26 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND); - if (copy_from_user(tlv, utlv, size)) + if (copy_from_user(tlv, utlv, size)) { rc = -EFAULT; - else + } else { + /* Same as the comment above */ + virtsnd_ctl_msg_ref(msg); + rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false); + } + + break; + default: + rc = -EINVAL; + WARN_ON(1); break; } +on_cleanup: + virtsnd_ctl_msg_unref(msg); + kfree(tlv); return rc; -- 2.43.0 --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [v3 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. 2024-02-14 14:01 ` [virtio-dev] " Aiswarya Cyriac @ 2024-02-14 15:36 ` Michael S. Tsirkin -1 siblings, 0 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2024-02-14 15:36 UTC (permalink / raw) To: Aiswarya Cyriac Cc: jasowang, perex, tiwai, linux-kernel, alsa-devel, virtualization, virtio-dev, Anton Yakovlev, coverity-bot On Wed, Feb 14, 2024 at 03:01:10PM +0100, Aiswarya Cyriac wrote: > This commit fixes the following warning when building virtio_snd driver. > > " > *** CID 1583619: Uninitialized variables (UNINIT) > sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() > 288 > 289 break; > 290 } > 291 > 292 kfree(tlv); > 293 > vvv CID 1583619: Uninitialized variables (UNINIT) > vvv Using uninitialized value "rc". > 294 return rc; > 295 } > 296 > 297 /** > 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. > 299 * @snd: VirtIO sound device. > " > > This warning is caused by the absence of the "default" branch in the > switch-block, and is a false positive because the kernel calls > virtsnd_kctl_tlv_op() only with values for op_flag processed in > this block. > > Also, this commit unifies the cleanup path for all possible control > paths in the callback function. > > Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> > Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> > Addresses-Coverity-ID: 1583619 ("Uninitialized variables") > Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") > --- > sound/virtio/virtio_kctl.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > index 0c6ac74aca1e..40606eb381af 100644 > --- a/sound/virtio/virtio_kctl.c > +++ b/sound/virtio/virtio_kctl.c > @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > > tlv = kzalloc(size, GFP_KERNEL); > if (!tlv) { > - virtsnd_ctl_msg_unref(msg); > - return -ENOMEM; > + rc = -ENOMEM; > + goto on_cleanup; > } > > sg_init_one(&sg, tlv, size); > @@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > case SNDRV_CTL_TLV_OP_READ: > hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ); > > + /* Since virtsnd_ctl_msg_send() drops the reference, we increase > + * the counter to be consistent with the on_cleanup path. > + */ This is not how multi-line comments should look. Adding overhead here is just a waste of cycles. Instead, separate error handling and normal exit paths. Then you will not need to increase the refcount here. > + virtsnd_ctl_msg_ref(msg); > + > rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false); > if (!rc) { > if (copy_to_user(utlv, tlv, size)) > @@ -281,14 +286,26 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > hdr->hdr.code = > cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND); > > - if (copy_from_user(tlv, utlv, size)) > + if (copy_from_user(tlv, utlv, size)) { > rc = -EFAULT; > - else > + } else { > + /* Same as the comment above */ Same thing. Besides, this kind of cross referencing breaks immediately when someone adds a comment in the middle. > + virtsnd_ctl_msg_ref(msg); > + > rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false); > + } > + > + break; > + default: > + rc = -EINVAL; /* We never get here - we listed all values for op_flag */ > + WARN_ON(1); > > break; > } > > +on_cleanup: > + virtsnd_ctl_msg_unref(msg); > + > kfree(tlv); > > return rc; on_cleanup is not informative, coding style says: "Choose label names which say what the goto does or why the goto exists." And saving on duplication here by paying elsewhere does not make sense. So you do this instead: kfree(tlv); return rc; on_error: virtsnd_ctl_msg_unref(msg); kfree(tlv); return rc; This is very ideomatic. > -- > 2.43.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [virtio-dev] Re: [v3 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. @ 2024-02-14 15:36 ` Michael S. Tsirkin 0 siblings, 0 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2024-02-14 15:36 UTC (permalink / raw) To: Aiswarya Cyriac Cc: jasowang, perex, tiwai, linux-kernel, alsa-devel, virtualization, virtio-dev, Anton Yakovlev, coverity-bot On Wed, Feb 14, 2024 at 03:01:10PM +0100, Aiswarya Cyriac wrote: > This commit fixes the following warning when building virtio_snd driver. > > " > *** CID 1583619: Uninitialized variables (UNINIT) > sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() > 288 > 289 break; > 290 } > 291 > 292 kfree(tlv); > 293 > vvv CID 1583619: Uninitialized variables (UNINIT) > vvv Using uninitialized value "rc". > 294 return rc; > 295 } > 296 > 297 /** > 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. > 299 * @snd: VirtIO sound device. > " > > This warning is caused by the absence of the "default" branch in the > switch-block, and is a false positive because the kernel calls > virtsnd_kctl_tlv_op() only with values for op_flag processed in > this block. > > Also, this commit unifies the cleanup path for all possible control > paths in the callback function. > > Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> > Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> > Addresses-Coverity-ID: 1583619 ("Uninitialized variables") > Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") > --- > sound/virtio/virtio_kctl.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > index 0c6ac74aca1e..40606eb381af 100644 > --- a/sound/virtio/virtio_kctl.c > +++ b/sound/virtio/virtio_kctl.c > @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > > tlv = kzalloc(size, GFP_KERNEL); > if (!tlv) { > - virtsnd_ctl_msg_unref(msg); > - return -ENOMEM; > + rc = -ENOMEM; > + goto on_cleanup; > } > > sg_init_one(&sg, tlv, size); > @@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > case SNDRV_CTL_TLV_OP_READ: > hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ); > > + /* Since virtsnd_ctl_msg_send() drops the reference, we increase > + * the counter to be consistent with the on_cleanup path. > + */ This is not how multi-line comments should look. Adding overhead here is just a waste of cycles. Instead, separate error handling and normal exit paths. Then you will not need to increase the refcount here. > + virtsnd_ctl_msg_ref(msg); > + > rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false); > if (!rc) { > if (copy_to_user(utlv, tlv, size)) > @@ -281,14 +286,26 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > hdr->hdr.code = > cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND); > > - if (copy_from_user(tlv, utlv, size)) > + if (copy_from_user(tlv, utlv, size)) { > rc = -EFAULT; > - else > + } else { > + /* Same as the comment above */ Same thing. Besides, this kind of cross referencing breaks immediately when someone adds a comment in the middle. > + virtsnd_ctl_msg_ref(msg); > + > rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false); > + } > + > + break; > + default: > + rc = -EINVAL; /* We never get here - we listed all values for op_flag */ > + WARN_ON(1); > > break; > } > > +on_cleanup: > + virtsnd_ctl_msg_unref(msg); > + > kfree(tlv); > > return rc; on_cleanup is not informative, coding style says: "Choose label names which say what the goto does or why the goto exists." And saving on duplication here by paying elsewhere does not make sense. So you do this instead: kfree(tlv); return rc; on_error: virtsnd_ctl_msg_unref(msg); kfree(tlv); return rc; This is very ideomatic. > -- > 2.43.0 --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v3 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. 2024-02-14 15:36 ` [virtio-dev] " Michael S. Tsirkin @ 2024-02-15 9:12 ` Aiswarya Cyriac -1 siblings, 0 replies; 6+ messages in thread From: Aiswarya Cyriac @ 2024-02-15 9:12 UTC (permalink / raw) To: Michael S. Tsirkin Cc: jasowang@redhat.com, perex@perex.cz, tiwai@suse.com, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org, virtualization@lists.linux-foundation.org, virtio-dev@lists.oasis-open.org, Anton Yakovlev, coverity-bot >>On Wed, Feb 14, 2024 at 03:01:10PM +0100, Aiswarya Cyriac wrote: >> This commit fixes the following warning when building virtio_snd driver. >> >> " >> *** CID 1583619: Uninitialized variables (UNINIT) >> sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() >> 288 >> 289 break; >> 290 } >> 291 >> 292 kfree(tlv); >> 293 >> vvv CID 1583619: Uninitialized variables (UNINIT) >> vvv Using uninitialized value "rc". >> 294 return rc; >> 295 } >> 296 >> 297 /** >> 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. >> 299 * @snd: VirtIO sound device. >> " >> >> This warning is caused by the absence of the "default" branch in the >> switch-block, and is a false positive because the kernel calls >> virtsnd_kctl_tlv_op() only with values for op_flag processed in >> this block. >> >> Also, this commit unifies the cleanup path for all possible control >> paths in the callback function. >> >> Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> >> Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> >> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> >> Addresses-Coverity-ID: 1583619 ("Uninitialized variables") >> Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") >> --- >> sound/virtio/virtio_kctl.c | 25 +++++++++++++++++++++---- >> 1 file changed, 21 insertions(+), 4 deletions(-) >> >> diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c >> index 0c6ac74aca1e..40606eb381af 100644 >> --- a/sound/virtio/virtio_kctl.c >> +++ b/sound/virtio/virtio_kctl.c >> @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, >> >> tlv = kzalloc(size, GFP_KERNEL); >> if (!tlv) { >> - virtsnd_ctl_msg_unref(msg); >> - return -ENOMEM; >> + rc = -ENOMEM; >> + goto on_cleanup; >> } >> >> sg_init_one(&sg, tlv, size); >> @@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, >> case SNDRV_CTL_TLV_OP_READ: >> hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ); >> >> + /* Since virtsnd_ctl_msg_send() drops the reference, we increase >> + * the counter to be consistent with the on_cleanup path. >> + */ > This is not how multi-line comments should look. > Adding overhead here is just a waste of cycles. > Instead, separate error handling and normal exit paths. > Then you will not need to increase the refcount here. Ok. >> + virtsnd_ctl_msg_ref(msg); >> + >> rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false); >> if (!rc) { >> if (copy_to_user(utlv, tlv, size)) >> @@ -281,14 +286,26 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, >> hdr->hdr.code = >> cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND); >> >> - if (copy_from_user(tlv, utlv, size)) >> + if (copy_from_user(tlv, utlv, size)) { >> rc = -EFAULT; >> - else >> + } else { >> + /* Same as the comment above */ > Same thing. > Besides, this kind of cross referencing breaks immediately when > someone adds a comment in the middle. I agree. I will update the patch >> + virtsnd_ctl_msg_ref(msg); >> + >> rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false); >> + } >> + >> + break; >> + default: >> + rc = -EINVAL; > /* We never get here - we listed all values for op_flag */ >> + WARN_ON(1); >> >> break; >> } >> >> +on_cleanup: >> + virtsnd_ctl_msg_unref(msg); >> + >> kfree(tlv); >> >> return rc; > on_cleanup is not informative, coding style says: > "Choose label names which say what the goto does or why the goto > exists." > And saving on duplication here by paying elsewhere does not make sense. > So you do this instead: > kfree(tlv); > return rc; > on_error: > virtsnd_ctl_msg_unref(msg); > kfree(tlv); > return rc; > This is very ideomatic. Thanks. I will separate error and non error cases and update patch Thanks, Aiswarya Cyriac Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin EMail: aiswarya.cyriac@opensynergy.com www.opensynergy.com Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B Geschäftsführer/Managing Director: Régis Adjamah ________________________________________ From: Michael S. Tsirkin <mst@redhat.com> Sent: Wednesday, February 14, 2024 4:36 PM To: Aiswarya Cyriac Cc: jasowang@redhat.com; perex@perex.cz; tiwai@suse.com; linux-kernel@vger.kernel.org; alsa-devel@alsa-project.org; virtualization@lists.linux-foundation.org; virtio-dev@lists.oasis-open.org; Anton Yakovlev; coverity-bot Subject: Re: [v3 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. On Wed, Feb 14, 2024 at 03:01:10PM +0100, Aiswarya Cyriac wrote: > This commit fixes the following warning when building virtio_snd driver. > > " > *** CID 1583619: Uninitialized variables (UNINIT) > sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() > 288 > 289 break; > 290 } > 291 > 292 kfree(tlv); > 293 > vvv CID 1583619: Uninitialized variables (UNINIT) > vvv Using uninitialized value "rc". > 294 return rc; > 295 } > 296 > 297 /** > 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. > 299 * @snd: VirtIO sound device. > " > > This warning is caused by the absence of the "default" branch in the > switch-block, and is a false positive because the kernel calls > virtsnd_kctl_tlv_op() only with values for op_flag processed in > this block. > > Also, this commit unifies the cleanup path for all possible control > paths in the callback function. > > Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> > Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> > Addresses-Coverity-ID: 1583619 ("Uninitialized variables") > Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") > --- > sound/virtio/virtio_kctl.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > index 0c6ac74aca1e..40606eb381af 100644 > --- a/sound/virtio/virtio_kctl.c > +++ b/sound/virtio/virtio_kctl.c > @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > > tlv = kzalloc(size, GFP_KERNEL); > if (!tlv) { > - virtsnd_ctl_msg_unref(msg); > - return -ENOMEM; > + rc = -ENOMEM; > + goto on_cleanup; > } > > sg_init_one(&sg, tlv, size); > @@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > case SNDRV_CTL_TLV_OP_READ: > hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ); > > + /* Since virtsnd_ctl_msg_send() drops the reference, we increase > + * the counter to be consistent with the on_cleanup path. > + */ This is not how multi-line comments should look. Adding overhead here is just a waste of cycles. Instead, separate error handling and normal exit paths. Then you will not need to increase the refcount here. > + virtsnd_ctl_msg_ref(msg); > + > rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false); > if (!rc) { > if (copy_to_user(utlv, tlv, size)) > @@ -281,14 +286,26 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > hdr->hdr.code = > cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND); > > - if (copy_from_user(tlv, utlv, size)) > + if (copy_from_user(tlv, utlv, size)) { > rc = -EFAULT; > - else > + } else { > + /* Same as the comment above */ Same thing. Besides, this kind of cross referencing breaks immediately when someone adds a comment in the middle. > + virtsnd_ctl_msg_ref(msg); > + > rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false); > + } > + > + break; > + default: > + rc = -EINVAL; /* We never get here - we listed all values for op_flag */ > + WARN_ON(1); > > break; > } > > +on_cleanup: > + virtsnd_ctl_msg_unref(msg); > + > kfree(tlv); > > return rc; on_cleanup is not informative, coding style says: "Choose label names which say what the goto does or why the goto exists." And saving on duplication here by paying elsewhere does not make sense. So you do this instead: kfree(tlv); return rc; on_error: virtsnd_ctl_msg_unref(msg); kfree(tlv); return rc; This is very ideomatic. > -- > 2.43.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [virtio-dev] Re: [v3 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. @ 2024-02-15 9:12 ` Aiswarya Cyriac 0 siblings, 0 replies; 6+ messages in thread From: Aiswarya Cyriac @ 2024-02-15 9:12 UTC (permalink / raw) To: Michael S. Tsirkin Cc: jasowang@redhat.com, perex@perex.cz, tiwai@suse.com, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org, virtualization@lists.linux-foundation.org, virtio-dev@lists.oasis-open.org, Anton Yakovlev, coverity-bot >>On Wed, Feb 14, 2024 at 03:01:10PM +0100, Aiswarya Cyriac wrote: >> This commit fixes the following warning when building virtio_snd driver. >> >> " >> *** CID 1583619: Uninitialized variables (UNINIT) >> sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() >> 288 >> 289 break; >> 290 } >> 291 >> 292 kfree(tlv); >> 293 >> vvv CID 1583619: Uninitialized variables (UNINIT) >> vvv Using uninitialized value "rc". >> 294 return rc; >> 295 } >> 296 >> 297 /** >> 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. >> 299 * @snd: VirtIO sound device. >> " >> >> This warning is caused by the absence of the "default" branch in the >> switch-block, and is a false positive because the kernel calls >> virtsnd_kctl_tlv_op() only with values for op_flag processed in >> this block. >> >> Also, this commit unifies the cleanup path for all possible control >> paths in the callback function. >> >> Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> >> Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> >> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> >> Addresses-Coverity-ID: 1583619 ("Uninitialized variables") >> Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") >> --- >> sound/virtio/virtio_kctl.c | 25 +++++++++++++++++++++---- >> 1 file changed, 21 insertions(+), 4 deletions(-) >> >> diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c >> index 0c6ac74aca1e..40606eb381af 100644 >> --- a/sound/virtio/virtio_kctl.c >> +++ b/sound/virtio/virtio_kctl.c >> @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, >> >> tlv = kzalloc(size, GFP_KERNEL); >> if (!tlv) { >> - virtsnd_ctl_msg_unref(msg); >> - return -ENOMEM; >> + rc = -ENOMEM; >> + goto on_cleanup; >> } >> >> sg_init_one(&sg, tlv, size); >> @@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, >> case SNDRV_CTL_TLV_OP_READ: >> hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ); >> >> + /* Since virtsnd_ctl_msg_send() drops the reference, we increase >> + * the counter to be consistent with the on_cleanup path. >> + */ > This is not how multi-line comments should look. > Adding overhead here is just a waste of cycles. > Instead, separate error handling and normal exit paths. > Then you will not need to increase the refcount here. Ok. >> + virtsnd_ctl_msg_ref(msg); >> + >> rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false); >> if (!rc) { >> if (copy_to_user(utlv, tlv, size)) >> @@ -281,14 +286,26 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, >> hdr->hdr.code = >> cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND); >> >> - if (copy_from_user(tlv, utlv, size)) >> + if (copy_from_user(tlv, utlv, size)) { >> rc = -EFAULT; >> - else >> + } else { >> + /* Same as the comment above */ > Same thing. > Besides, this kind of cross referencing breaks immediately when > someone adds a comment in the middle. I agree. I will update the patch >> + virtsnd_ctl_msg_ref(msg); >> + >> rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false); >> + } >> + >> + break; >> + default: >> + rc = -EINVAL; > /* We never get here - we listed all values for op_flag */ >> + WARN_ON(1); >> >> break; >> } >> >> +on_cleanup: >> + virtsnd_ctl_msg_unref(msg); >> + >> kfree(tlv); >> >> return rc; > on_cleanup is not informative, coding style says: > "Choose label names which say what the goto does or why the goto > exists." > And saving on duplication here by paying elsewhere does not make sense. > So you do this instead: > kfree(tlv); > return rc; > on_error: > virtsnd_ctl_msg_unref(msg); > kfree(tlv); > return rc; > This is very ideomatic. Thanks. I will separate error and non error cases and update patch Thanks, Aiswarya Cyriac Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin EMail: aiswarya.cyriac@opensynergy.com www.opensynergy.com Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B Geschäftsführer/Managing Director: Régis Adjamah ________________________________________ From: Michael S. Tsirkin <mst@redhat.com> Sent: Wednesday, February 14, 2024 4:36 PM To: Aiswarya Cyriac Cc: jasowang@redhat.com; perex@perex.cz; tiwai@suse.com; linux-kernel@vger.kernel.org; alsa-devel@alsa-project.org; virtualization@lists.linux-foundation.org; virtio-dev@lists.oasis-open.org; Anton Yakovlev; coverity-bot Subject: Re: [v3 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. On Wed, Feb 14, 2024 at 03:01:10PM +0100, Aiswarya Cyriac wrote: > This commit fixes the following warning when building virtio_snd driver. > > " > *** CID 1583619: Uninitialized variables (UNINIT) > sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() > 288 > 289 break; > 290 } > 291 > 292 kfree(tlv); > 293 > vvv CID 1583619: Uninitialized variables (UNINIT) > vvv Using uninitialized value "rc". > 294 return rc; > 295 } > 296 > 297 /** > 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. > 299 * @snd: VirtIO sound device. > " > > This warning is caused by the absence of the "default" branch in the > switch-block, and is a false positive because the kernel calls > virtsnd_kctl_tlv_op() only with values for op_flag processed in > this block. > > Also, this commit unifies the cleanup path for all possible control > paths in the callback function. > > Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> > Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> > Addresses-Coverity-ID: 1583619 ("Uninitialized variables") > Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") > --- > sound/virtio/virtio_kctl.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > index 0c6ac74aca1e..40606eb381af 100644 > --- a/sound/virtio/virtio_kctl.c > +++ b/sound/virtio/virtio_kctl.c > @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > > tlv = kzalloc(size, GFP_KERNEL); > if (!tlv) { > - virtsnd_ctl_msg_unref(msg); > - return -ENOMEM; > + rc = -ENOMEM; > + goto on_cleanup; > } > > sg_init_one(&sg, tlv, size); > @@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > case SNDRV_CTL_TLV_OP_READ: > hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ); > > + /* Since virtsnd_ctl_msg_send() drops the reference, we increase > + * the counter to be consistent with the on_cleanup path. > + */ This is not how multi-line comments should look. Adding overhead here is just a waste of cycles. Instead, separate error handling and normal exit paths. Then you will not need to increase the refcount here. > + virtsnd_ctl_msg_ref(msg); > + > rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false); > if (!rc) { > if (copy_to_user(utlv, tlv, size)) > @@ -281,14 +286,26 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > hdr->hdr.code = > cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND); > > - if (copy_from_user(tlv, utlv, size)) > + if (copy_from_user(tlv, utlv, size)) { > rc = -EFAULT; > - else > + } else { > + /* Same as the comment above */ Same thing. Besides, this kind of cross referencing breaks immediately when someone adds a comment in the middle. > + virtsnd_ctl_msg_ref(msg); > + > rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false); > + } > + > + break; > + default: > + rc = -EINVAL; /* We never get here - we listed all values for op_flag */ > + WARN_ON(1); > > break; > } > > +on_cleanup: > + virtsnd_ctl_msg_unref(msg); > + > kfree(tlv); > > return rc; on_cleanup is not informative, coding style says: "Choose label names which say what the goto does or why the goto exists." And saving on duplication here by paying elsewhere does not make sense. So you do this instead: kfree(tlv); return rc; on_error: virtsnd_ctl_msg_unref(msg); kfree(tlv); return rc; This is very ideomatic. > -- > 2.43.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-15 9:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-14 14:01 [v3 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning Aiswarya Cyriac 2024-02-14 14:01 ` [virtio-dev] " Aiswarya Cyriac 2024-02-14 15:36 ` Michael S. Tsirkin 2024-02-14 15:36 ` [virtio-dev] " Michael S. Tsirkin 2024-02-15 9:12 ` Aiswarya Cyriac 2024-02-15 9:12 ` [virtio-dev] " Aiswarya Cyriac
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.