linux-kernel - Re: arch/x86/kvm/svm/sev.c:1605:30: warning: parameter 'role' set but not used

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <YqddYy6nO0+G/kVK@google.com>
Date:   Mon, 13 Jun 2022 15:53:07 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Peter Gonda <pgonda@...gle.com>
Cc:     kernel test robot <lkp@...el.com>, llvm@...ts.linux.dev,
        kbuild-all@...ts.01.org, LKML <linux-kernel@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        David Rientjes <rientjes@...gle.com>
Subject: Re: arch/x86/kvm/svm/sev.c:1605:30: warning: parameter 'role' set
 but not used

On Mon, Jun 13, 2022, Peter Gonda wrote:
> On Mon, Jun 13, 2022 at 5:03 AM kernel test robot <lkp@...el.com> wrote:
> >   1603
> >   1604  static int sev_lock_vcpus_for_migration(struct kvm *kvm,
> > > 1605                                          enum sev_migration_role role)
> >   1606  {
> >   1607          struct kvm_vcpu *vcpu;
> >   1608          unsigned long i, j;
> >   1609          bool first = true;
> >   1610
> >   1611          kvm_for_each_vcpu(i, vcpu, kvm) {
> >   1612                  if (mutex_lock_killable_nested(&vcpu->mutex, role))
> >   1613                          goto out_unlock;
> >   1614
> >
> 
> I am confused about this warning. |role| is used on this line above.
> Is this because CONFIG_DEBUG_LOCK_ALLOC the subclass argument is
> dropped in the macro?

Yep, at that point the compiler can easily detect that there's no actual usage of
the parameter.

There's no need for the "first" variable, it's the same as "i == 0" and "j == 0".
With that out of the way, the role and mutex_release/mutex_acquire shenanigans can
be wrapped with ifdeffery.  Not the prettiest thing, but I actually like the #ifdefs
because they effectively document that KVM is playing games to make lockdep happy.

I'll send this as a formal patch, I think it'll make clang-15 happy.

---
 arch/x86/kvm/svm/sev.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 51fd985cf21d..593c61683484 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1606,38 +1606,35 @@ static int sev_lock_vcpus_for_migration(struct kvm *kvm,
 {
 	struct kvm_vcpu *vcpu;
 	unsigned long i, j;
-	bool first = true;

 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		if (mutex_lock_killable_nested(&vcpu->mutex, role))
 			goto out_unlock;

-		if (first) {
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+		if (!i)
 			/*
 			 * Reset the role to one that avoids colliding with
 			 * the role used for the first vcpu mutex.
 			 */
 			role = SEV_NR_MIGRATION_ROLES;
-			first = false;
-		} else {
+		else
 			mutex_release(&vcpu->mutex.dep_map, _THIS_IP_);
-		}
+#endif
 	}

 	return 0;

 out_unlock:

-	first = true;
 	kvm_for_each_vcpu(j, vcpu, kvm) {
 		if (i == j)
 			break;

-		if (first)
-			first = false;
-		else
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+		if (j)
 			mutex_acquire(&vcpu->mutex.dep_map, role, 0, _THIS_IP_);
-
+#endif

 		mutex_unlock(&vcpu->mutex);
 	}

base-commit: 8a6d3a6ec6a821c5ddb3972fdbad9c4149eabf1e
--