linux-kernel - Re: [PATCH-next] KVM: x86/tdp_mmu: Fix redundant u16 compared to 0

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: <CAJ7bepLzHYypiNiqyfbSba4hRBMf0OFekVa--9Nm0nuW4D=jDg@mail.gmail.com>
Date: Tue, 12 Nov 2024 09:47:34 +0530
From: Advait Dhamorikar <advaitdhamorikar@...il.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, hpa@...or.com, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, 
	anupnewsmail@...il.com
Subject: Re: [PATCH-next] KVM: x86/tdp_mmu: Fix redundant u16 compared to 0

Hello Sean,

> NAK, the comparison is necessary as kvm_tdp_mmu_zap_leafs() deliberately invokes
> for_each_valid_tdp_mmu_root_yi
> eld_safe() => __for_each_tdp_mmu_root_yield_safe()
>with -1 to iterate over all address spaces.

> And I don't want to drop the check for __for_each_tdp_mmu_root(), even though
> there aren't any _current_ users that pass -1, as I want to keep the iterators
> symmetrical.

Understood, thanks for the feedback.

Best regards,
Advait

On Tue, 12 Nov 2024 at 00:26, Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Tue, Nov 12, 2024, Advait Dhamorikar wrote:
> > An unsigned value can never be negative,
> > so this test will always evaluate the same way.
> > `_as_id` a u16 is compared to 0.
>
> Please wrap changelogs at ~75 characters.
>
> > Signed-off-by: Advait Dhamorikar <advaitdhamorikar@...il.com>
> > ---
> >  arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > index 4508d868f1cd..b4e7b6a264d6 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -153,7 +153,7 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
> >       for (_root = tdp_mmu_next_root(_kvm, NULL, _only_valid);                \
> >            ({ lockdep_assert_held(&(_kvm)->mmu_lock); }), _root;              \
> >            _root = tdp_mmu_next_root(_kvm, _root, _only_valid))               \
> > -             if (_as_id >= 0 && kvm_mmu_page_as_id(_root) != _as_id) {       \
> > +             if (kvm_mmu_page_as_id(_root) != _as_id) {      \
>
> NAK, the comparison is necessary as kvm_tdp_mmu_zap_leafs() deliberately invokes
> for_each_valid_tdp_mmu_root_yield_safe() => __for_each_tdp_mmu_root_yield_safe()
> with -1 to iterate over all address spaces.
>
> And I don't want to drop the check for __for_each_tdp_mmu_root(), even though
> there aren't any _current_ users that pass -1, as I want to keep the iterators
> symmetrical.