| Message ID | 20240216100643.688590-1-aiswarya.cyriac@opensynergy.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. | expand |
On Fri, Feb 16, 2024 at 11:06:43AM +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 | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > index 0c6ac74aca1e..7aa79c05b464 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_msg_unref; > } > > sg_init_one(&sg, tlv, size); > @@ -281,14 +281,25 @@ 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 > + goto on_msg_unref; > + } else { > 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); > + goto on_msg_unref; > } > + kfree(tlv); > + return rc; > > +on_msg_unref: > + virtsnd_ctl_msg_unref(msg); > kfree(tlv); > > return rc; I don't really like adding code for a false-positive but ALSA maintainers seem to like this. If yes, this seems like as good a way as any to do it. Acked-by: Michael S. Tsirkin <mst@redhat.com> > -- > 2.43.2
On Fri, 16 Feb 2024 12:27:48 +0100, Michael S. Tsirkin wrote: > > On Fri, Feb 16, 2024 at 11:06:43AM +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 | 19 +++++++++++++++---- > > 1 file changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > > index 0c6ac74aca1e..7aa79c05b464 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_msg_unref; > > } > > > > sg_init_one(&sg, tlv, size); > > @@ -281,14 +281,25 @@ 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 > > + goto on_msg_unref; > > + } else { > > 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); > > + goto on_msg_unref; > > } > > + kfree(tlv); > > + return rc; > > > > +on_msg_unref: > > + virtsnd_ctl_msg_unref(msg); > > kfree(tlv); > > > > return rc; > > I don't really like adding code for a false-positive but ALSA > maintainers seem to like this. If yes, this seems like as good > a way as any to do it. Err, no, you misunderstood the situation. I took the v1 patch quickly because: - It was with Anton's SOB, who is another maintainer of the driver - I assumed you lost interest in this driver since you haven't reacted to the previous patches for long time - The change there was small and simple enough Now, it grows unnecessarily large, and yet you complained. Why should I take it, then? This is a subtle cosmetic issue that isn't worth for wasting too much time and energy. If we want to shut up the compile warning, and this is a case where it can't happen, just put the "default:" to the existing case. If you want to be user-friendly, put some comment. That's all. It'll be a one-liner. OTOH, if we do care and want to catch any potential logical mistake, you can put WARN(). But, this doesn't have to go out as an error. Simply putting WARN() for the default and going through would work, too. Or we can keep this lengthy changes if we want, too. So, I really don't mind which way to fix as long as it works correctly (and doesn't look too ugly). Please make agreement among you guys, and resubmit if needed. thanks, Takashi
On Fri, Feb 16, 2024 at 02:42:37PM +0100, Takashi Iwai wrote: > On Fri, 16 Feb 2024 12:27:48 +0100, > Michael S. Tsirkin wrote: > > > > On Fri, Feb 16, 2024 at 11:06:43AM +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 | 19 +++++++++++++++---- > > > 1 file changed, 15 insertions(+), 4 deletions(-) > > > > > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > > > index 0c6ac74aca1e..7aa79c05b464 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_msg_unref; > > > } > > > > > > sg_init_one(&sg, tlv, size); > > > @@ -281,14 +281,25 @@ 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 > > > + goto on_msg_unref; > > > + } else { > > > 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); > > > + goto on_msg_unref; > > > } > > > + kfree(tlv); > > > + return rc; > > > > > > +on_msg_unref: > > > + virtsnd_ctl_msg_unref(msg); > > > kfree(tlv); > > > > > > return rc; > > > > I don't really like adding code for a false-positive but ALSA > > maintainers seem to like this. If yes, this seems like as good > > a way as any to do it. > > Err, no, you misunderstood the situation. > > I took the v1 patch quickly because: > - It was with Anton's SOB, who is another maintainer of the driver > - I assumed you lost interest in this driver since you haven't reacted > to the previous patches for long time > - The change there was small and simple enough > > Now, it grows unnecessarily large, and yet you complained. Why should > I take it, then? > > This is a subtle cosmetic issue that isn't worth for wasting too much > time and energy. If we want to shut up the compile warning, and this > is a case where it can't happen, just put the "default:" to the > existing case. If you want to be user-friendly, put some comment. > That's all. It'll be a one-liner. > > OTOH, if we do care and want to catch any potential logical mistake, > you can put WARN(). But, this doesn't have to go out as an error. > Simply putting WARN() for the default and going through would work, > too. > > Or we can keep this lengthy changes if we want, too. > > So, I really don't mind which way to fix as long as it works correctly > (and doesn't look too ugly). Please make agreement among you guys, > and resubmit if needed. > > > thanks, > > Takashi OK sorry about too verbose. I mean since Anton wants it, I ack this. Acked-by: Michael S. Tsirkin <mst@redhat.com>
On Fri, 16 Feb 2024 14:48:10 +0100, Michael S. Tsirkin wrote: > > On Fri, Feb 16, 2024 at 02:42:37PM +0100, Takashi Iwai wrote: > > On Fri, 16 Feb 2024 12:27:48 +0100, > > Michael S. Tsirkin wrote: > > > > > > On Fri, Feb 16, 2024 at 11:06:43AM +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 | 19 +++++++++++++++---- > > > > 1 file changed, 15 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > > > > index 0c6ac74aca1e..7aa79c05b464 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_msg_unref; > > > > } > > > > > > > > sg_init_one(&sg, tlv, size); > > > > @@ -281,14 +281,25 @@ 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 > > > > + goto on_msg_unref; > > > > + } else { > > > > 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); > > > > + goto on_msg_unref; > > > > } > > > > + kfree(tlv); > > > > + return rc; > > > > > > > > +on_msg_unref: > > > > + virtsnd_ctl_msg_unref(msg); > > > > kfree(tlv); > > > > > > > > return rc; > > > > > > I don't really like adding code for a false-positive but ALSA > > > maintainers seem to like this. If yes, this seems like as good > > > a way as any to do it. > > > > Err, no, you misunderstood the situation. > > > > I took the v1 patch quickly because: > > - It was with Anton's SOB, who is another maintainer of the driver > > - I assumed you lost interest in this driver since you haven't reacted > > to the previous patches for long time > > - The change there was small and simple enough > > > > Now, it grows unnecessarily large, and yet you complained. Why should > > I take it, then? > > > > This is a subtle cosmetic issue that isn't worth for wasting too much > > time and energy. If we want to shut up the compile warning, and this > > is a case where it can't happen, just put the "default:" to the > > existing case. If you want to be user-friendly, put some comment. > > That's all. It'll be a one-liner. > > > > OTOH, if we do care and want to catch any potential logical mistake, > > you can put WARN(). But, this doesn't have to go out as an error. > > Simply putting WARN() for the default and going through would work, > > too. > > > > Or we can keep this lengthy changes if we want, too. > > > > So, I really don't mind which way to fix as long as it works correctly > > (and doesn't look too ugly). Please make agreement among you guys, > > and resubmit if needed. > > > > > > thanks, > > > > Takashi > > OK sorry about too verbose. I mean since Anton wants it, I ack this. > > Acked-by: Michael S. Tsirkin <mst@redhat.com> Alright, I applied this one. Thanks! Takashi
diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c index 0c6ac74aca1e..7aa79c05b464 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_msg_unref; } sg_init_one(&sg, tlv, size); @@ -281,14 +281,25 @@ 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 + goto on_msg_unref; + } else { 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); + goto on_msg_unref; } + kfree(tlv); + return rc; +on_msg_unref: + virtsnd_ctl_msg_unref(msg); kfree(tlv); return rc;