On Wed, Aug 29, 2018 at 06:42:42PM +0800, Wanpeng Li wrote:

On Wed, 29 Aug 2018 at 18:29, Dan Carpenter [off-list ref] wrote:

quoted

On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote:

quoted

On Wed, 29 Aug 2018 at 18:18, Dan Carpenter [off-list ref] wrote:

quoted

On Wed, Aug 29, 2018 at 01:12:05PM +0300, Dan Carpenter wrote:

quoted

On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:

quoted

quoted

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))

Wait, the (s32) cast doesn't make sense.  We want negative min values to
be treated as invalid.

In v2, how about:

if (unlikely(min > map->max_apic_id || (min + __fls(ipi_bitmap_low)) >
map->max_apic_id))
    goto out;

That works, too.  It still has the off by one and we should set

Sorry, why off by one?

Sorry, my bad.  I looked at the code and > is correct.  (At first, I
thought it should be >= but I hadn't looked at the context).

regards,
dan carpenter