Received-SPF: pass (google.com: domain of linux-kernel+bounces-68655-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 16 Feb 2024 14:42:37 +0100
Message-ID: <87eddcikz6.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com>,
	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 <anton.yakovlev@opensynergy.com>,
	coverity-bot <keescook+coverity-bot@chromium.org>
Subject: Re: [v4 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning.
In-Reply-To: <20240216062630-mutt-send-email-mst@kernel.org>
References: <20240216100643.688590-1-aiswarya.cyriac@opensynergy.com>
	<20240216062630-mutt-send-email-mst@kernel.org>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII

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