linux-kernel - Re: [PATCH] kvm: Avoid shadowing a local in search

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 26 Oct 2021 15:44:47 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Qian Cai <quic_qiancai@...cinc.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kvm: Avoid shadowing a local in search_memslots()

On Tue, Oct 26, 2021, Qian Cai wrote:
> It is less error-prone to use a different variable name from the existing
> one in a wider scope. This is also flagged by GCC (W=2):
> 
> ./include/linux/kvm_host.h: In function 'search_memslots':
> ./include/linux/kvm_host.h:1246:7: warning: declaration of 'slot' shadows a previous local [-Wshadow]
>  1246 |   int slot = start + (end - start) / 2;
>       |       ^~~~
> ./include/linux/kvm_host.h:1240:26: note: shadowed declaration is here
>  1240 |  struct kvm_memory_slot *slot;
>       |                          ^~~~
> 

Even though this doesn't need to go to stable, probably worth adding a Fixes: to
acknowledge that this was a recently introduced mess.

  Fixes: 0f22af940dc8 ("KVM: Move last_used_slot logic out of search_memslots")


> Signed-off-by: Qian Cai <quic_qiancai@...cinc.com>
> ---
>  include/linux/kvm_host.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 60a35d9fe259..1c1a36f658fe 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1243,12 +1243,12 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn, int *index)
>  		return NULL;
>  
>  	while (start < end) {
> -		int slot = start + (end - start) / 2;
> +		int new_slot = start + (end - start) / 2;

new_slot isn't a great name, the integer "slot" isn't directly connected to the
final memslot and may not be representative of the final memslot's index depending
on how the binary search resolves.

Maybe "pivot"?  Or just "tmp"?  I also vote to hoist the declaration out of the
loop precisely to avoid potential shadows, and to also associate the variable
with the "start" and "end" variables, e.g.

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 60a35d9fe259..663bdfa0983f 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1235,7 +1235,7 @@ try_get_memslot(struct kvm_memslots *slots, int slot_index, gfn_t gfn)
 static inline struct kvm_memory_slot *
 search_memslots(struct kvm_memslots *slots, gfn_t gfn, int *index)
 {
-       int start = 0, end = slots->used_slots;
+       int start = 0, end = slots->used_slots, pivot;
        struct kvm_memory_slot *memslots = slots->memslots;
        struct kvm_memory_slot *slot;

@@ -1243,12 +1243,11 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn, int *index)
                return NULL;

        while (start < end) {
-               int slot = start + (end - start) / 2;
-
-               if (gfn >= memslots[slot].base_gfn)
-                       end = slot;
+               pivot = start + (end - start) / 2;
+               if (gfn >= memslots[pivot].base_gfn)
+                       end = pivot;
                else
-                       start = slot + 1;
+                       start = pivot + 1;
        }

        slot = try_get_memslot(slots, start, gfn);