Re: [PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt

[Sean Christopherson]

On Thu, Mar 11, 2021, Ben Gardon wrote:
> The root page table in the TDP MMU paging structure is not protected
> with RCU, but rather by the root_count in the associated SP. As a result
> it is safe for tdp_iter_root_pt to simply return a u64 *. This sidesteps
> the complexities assoicated with propagating the __rcu annotation
> around.
> 
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Signed-off-by: Ben Gardon <bgardon@xxxxxxxxxx>
> ---
>  arch/x86/kvm/mmu/tdp_iter.c | 10 ++++++++--
>  arch/x86/kvm/mmu/tdp_iter.h |  2 +-
>  arch/x86/kvm/mmu/tdp_mmu.c  |  4 ++--
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_iter.c b/arch/x86/kvm/mmu/tdp_iter.c
> index e5f148106e20..8e2c053533b6 100644
> --- a/arch/x86/kvm/mmu/tdp_iter.c
> +++ b/arch/x86/kvm/mmu/tdp_iter.c
> @@ -159,8 +159,14 @@ void tdp_iter_next(struct tdp_iter *iter)
>  	iter->valid = false;
>  }
>  
> -tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter)
> +u64 *tdp_iter_root_pt(struct tdp_iter *iter)
>  {
> -	return iter->pt_path[iter->root_level - 1];
> +	/*
> +	 * Though it is stored in an array of tdp_ptep_t for convenience,
> +	 * the root PT is not actually protected by RCU, but by the root
> +	 * count on the associated struct kvm_mmu_page. As a result it's
> +	 * safe to rcu_dereference and return the value here.

I'm not a big fan of this comment.  It implies that calling tdp_iter_root_pt()
without RCU protection is completely ok, but that's not true, as rcu_dereferecne()
will complain when CONFIG_PROVE_RCU=1.

There's also a good opportunity to streamline the the helper here, since both
callers use the root only to get to the associated shadow page, and that's only
done to get the as_id.  If we provide tdp_iter_as_id() then the need for a
comment goes away and we shave a few lines of code.

That being said, an even better option would be to store as_id in the TDP iter.
The cost on the stack is negligible, and while the early sptep->as_id lookup
will be unnecessary in some cases, it will be a net win when setting multiple
sptes, e.g. in mmu_notifier callbacks.

Compile tested only...