Date:   Mon, 6 Dec 2021 18:01:05 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Ameer Hamza <amhamza.mgc@gmail.com>
Cc:     vkuznets@redhat.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, pbonzini@redhat.com,
        wanpengli@tencent.com, jmattson@google.com, joro@8bytes.org,
        tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com
Subject: Re: [PATCH v3] KVM: x86: fix for missing initialization of return
 status variable
Message-ID: <Ya5P4WWsgCyQZvBH@google.com>
References: <20211206160813.GA37599@hamza-OptiPlex-7040>
 <20211206164503.135917-1-amhamza.mgc@gmail.com>
 <Ya5CCU0zf+MzMwcX@google.com>
 <20211206172746.GA141396@hamza-OptiPlex-7040>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211206172746.GA141396@hamza-OptiPlex-7040>
Precedence: bulk

On Mon, Dec 06, 2021, Ameer Hamza wrote:
> On Mon, Dec 06, 2021 at 05:02:01PM +0000, Sean Christopherson wrote:
> > On Mon, Dec 06, 2021, Ameer Hamza wrote:
> > > If undefined ioctl number is passed to the kvm_vcpu_ioctl_device_attr
> > > ioctl, we should trigger KVM_BUG_ON() and return with EIO to silent
> > > coverity warning.
> > > 
> > > Addresses-Coverity: 1494124 ("Uninitialized scalar variable")
> > > Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com>
> > > ---
> > > Changes in v3:
> > > Added KVM_BUG_ON() as default case and returned -EIO
> > > ---
> > >  arch/x86/kvm/x86.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index e0aa4dd53c7f..b37068f847ff 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -5019,6 +5019,9 @@ static int kvm_vcpu_ioctl_device_attr(struct kvm_vcpu *vcpu,
> > >  	case KVM_SET_DEVICE_ATTR:
> > >  		r = kvm_arch_tsc_set_attr(vcpu, &attr);
> > >  		break;
> > > +	default:
> > > +		KVM_BUG_ON(1, vcpu->kvm);
> > > +		r = -EIO;
> > 
> > At least have a
> > 
> > 		break;
> > 
> > if we're going to be pedantic about things.
> I just started as a contributer in this community and trying
> to fix issues found by static analyzer tools. If you think that's
> not necessary, its totally fine :)

(Most) Static analyzers are great, they definitely find real bugs.  But they also
have a fair number of false positives, e.g. this is a firmly a false positive, so
the results of any static analyzer needs to thought about critically, not blindly
followed.  It's completely understandable that Coverity got tripped up in this
case, but that's exactly why having a human vet the bug report is necessary.

There is arguably value in having a default statement to ensure future KVM code
doesn't end up adding a bad call, which is why I'm not completely opposed to the
above addition.

Where folks, myself included, get a bit grumpy is when patches are sent to "fix"
bug reports from static analyzers without evidence that the submitter has done
their due dilegence to understand the code they are changing, e.g. even without
any understanding of KVM, a search of kvm_vcpu_ioctl_device_attr() in the code
base and reading of the function would have shown that the report was a false
positive, albeit a somewhat odd one, and that returning -EINVAL was likely the
wrong thing to do.  If you're unsure if something is a real bug, please ask a
question.

Rapid firing patches at the list also makes reviewers grumpy as it again suggests
a lack of due dilegence, especially when the patches have typos ("EINV" in v2)
and/or have obvious shortcomings (missing "break" in v3).

TL;DR: I have no objection whatsover to fixing (potential) bugs found by static
analyzers, but please slow down and (a) make sure that it's actually a bug, (b)
ask if you're unsure, and (c) do your best to ensure that what you're sending is
an overall improvement.