Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 29 Aug 2018 13:12:05 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Liran Alon <liran.alon@oracle.com>
Cc:     Wanpeng Li <kernellwp@gmail.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Subject: Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
Message-ID: <20180829101205.jsp53e2wq7fc6ukd@mwanda>
References: <1535521943-5547-1-git-send-email-wanpengli@tencent.com>
 <A523C29C-C3E8-4FEF-8620-96365F774582@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <A523C29C-C3E8-4FEF-8620-96365F774582@oracle.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:
> > arch/x86/kvm/lapic.c | 17 +++++++++++++----
> > 1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 0cefba2..86e933c 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > 	rcu_read_lock();
> > 	map = rcu_dereference(kvm->arch.apic_map);
> > 
> > +	if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
> > +		goto out;
> 
> I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
> But that’s just a matter of taste :)

That's an integer overflow.

But I do prefer to put the variable on the left.  The truth is that some
Smatch checks just ignore code which is backwards written because
otherwise you have to write duplicate code and the most code is written
with the variable on the left.

	if (min > (s32)(map->max_apic_id - __fls(ipi_bitmap_low))

Shouldn't this be >= instead?  It looks off by one.

regards,
dan carpenter