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.yakov...@opensynergy.com>
> > > > Signed-off-by: Aiswarya Cyriac <aiswarya.cyr...@opensynergy.com>
> > > > Reported-by: coverity-bot <keescook+coverity-...@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 <m...@redhat.com>
Alright, I applied this one. Thanks!
Takashi
