linux-kernel - Re: [PATCH 07/12] KVM: X86/MMU: Remove the useless struct mmu_page

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 21 Jul 2022 15:25:57 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Lai Jiangshan <jiangshanlai@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "open list:KERNEL VIRTUAL MACHINE FOR MIPS (KVM/mips)" 
        <kvm@...r.kernel.org>, Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        Lai Jiangshan <jiangshan.ljs@...group.com>
Subject: Re: [PATCH 07/12] KVM: X86/MMU: Remove the useless struct
 mmu_page_path

On Thu, Jul 21, 2022, Lai Jiangshan wrote:
> On Wed, Jul 20, 2022 at 4:15 AM Sean Christopherson <seanjc@...gle.com> wrote:
> >
> > Nit, s/useless/now-unused, or "no longer used".  I associate "useless" in shortlogs
> > as "this <xyz> is pointless and always has been pointless", whereas "now-unused"
> > is likely to be interpreted as "remove <xyz> as it's no longer used after recent
> > changes".
> >
> > Alternatively, can this patch be squashed with the patch that removes
> > mmu_pages_clear_parents()?  Yeah, it'll be a (much?) larger patch, but leaving
> > dead code behind is arguably worse.
> 
> Defined by the C-language and the machine, struct mmu_page_path is used
> in for_each_sp() and the data is set and updated every iteration.
> 
> It is not really dead code.

I'm not talking about just "struct mmu_page_path", but also the pointless updates
in for_each_sp().  And I think even if we're being super pedantic, it _is_ dead
code because C99 allows the compiler to drop code that the compiler can prove has
no side effects.  I learned this the hard way by discovering that an asm() blob
with an output constraint will be elided if the output isn't consumed and the asm()
blob isn't tagged volatile.

  In the abstract machine, all expressions are evaluated as specified by the
  semantics. An actual implementation need not evaluate part of an expression if
  it can deduce that its value is not used and that no needed side effects are
  produced (including any caused by calling a function or accessing a volatile object)

I don't see any advantage to separating this from mmu_pages_clear_parents().  It
doesn't make the code any easier to review.  I'd argue it does the opposite because
it makes it harder to see that mmu_pages_clear_parents() was the only user, i.e.
squashing this would provide further justification for dropping mmu_pages_clear_parents().