Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: reproducible BUG() in kvm_mmu_get_root() in TDP MMU
To:     Ben Gardon <bgardon@google.com>, leohou1402 <leohou1402@gmail.com>
Cc:     "maciej.szmigiero@oracle.com" <maciej.szmigiero@oracle.com>,
        "seanjc@google.com" <seanjc@google.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "cannonmatthews@google.com" <cannonmatthews@google.com>,
        "peterx@redhat.com" <peterx@redhat.com>,
        "pshier@google.com" <pshier@google.com>,
        "pfeiner@google.com" <pfeiner@google.com>,
        "junaids@google.com" <junaids@google.com>,
        "jmattson@google.com" <jmattson@google.com>,
        "yulei.kernel@gmail.com" <yulei.kernel@gmail.com>,
        "kernellwp@gmail.com" <kernellwp@gmail.com>,
        "vkuznets@redhat.com" <vkuznets@redhat.com>
References: <4bf6fcae-20e7-3eae-83ec-51fb52110487@oracle.com>
 <8A352C2E-E7D2-4873-807F-635A595DCAEF@gmail.com>
 <CANgfPd_cbBxWHmPsw0x5NfKrMXzij3YAAiaq665zxn5nnraPGg@mail.gmail.com>
 <CANgfPd8fFB6QM3bOhxQ0WPjw6f5FLqBm1ynCenAxymByq4Lz5g@mail.gmail.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <f360715b-e61b-7e68-1aa9-84df51331d95@redhat.com>
Date:   Tue, 5 Jan 2021 19:06:23 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.5.0
MIME-Version: 1.0
In-Reply-To: <CANgfPd8fFB6QM3bOhxQ0WPjw6f5FLqBm1ynCenAxymByq4Lz5g@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 05/01/21 18:49, Ben Gardon wrote:
> for_each_tdp_mmu_root(kvm, root) {
>          kvm_mmu_get_root(kvm, root);
>          <Do something, yield the MMU lock>
>          kvm_mmu_put_root(kvm, root);
> }
> 
> In these cases the get and put root calls are there to ensure that the
> root is not freed while the function is running, however they do this
> too well. If the put root call reduces the root's root_count to 0, it
> should be removed from the roots list and freed before the MMU lock is
> released. However the above pattern never bothers to free the root.
> The following would fix this bug:
> 
> -kvm_mmu_put_root(kvm, root);
> +if (kvm_mmu_put_root(kvm, root))
> +       kvm_tdp_mmu_free_root(kvm, root);

Is it worth writing a more complex iterator struct, so that 
for_each_tdp_mmu_root takes care of the get and put?

Paolo