Date:   Fri, 17 Mar 2023 14:21:29 +0800
From:   Yan Zhao <yan.y.zhao@intel.com>
To:     Sean Christopherson <seanjc@google.com>
CC:     Paolo Bonzini <pbonzini@redhat.com>,
        Zhenyu Wang <zhenyuw@linux.intel.com>,
        Zhi Wang <zhi.a.wang@intel.com>, <kvm@vger.kernel.org>,
        <intel-gvt-dev@lists.freedesktop.org>,
        <intel-gfx@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        Ben Gardon <bgardon@google.com>
Subject: Re: [PATCH v2 10/27] drm/i915/gvt: Protect gfn hash table with
 vgpu_lock
Message-ID: <ZBQG6SIS2a4WBjNc@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20230311002258.852397-1-seanjc@google.com>
 <20230311002258.852397-11-seanjc@google.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230311002258.852397-11-seanjc@google.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?pevretJm3gBlix0ZT28P42zZBizUM0TkTninuHuZz4aVHIty3fXt3UsWUnnE?=
 =?us-ascii?Q?1Tqm94D00g1aMKvN2CtEEA619fevEJ0k+NymjbZQY6e/99w5fiSYMP0W3eVa?=
 =?us-ascii?Q?TRPieSnjF2IdM1X88h4ZZVCUSTHZP2eoMu6mRT2fz23PRC+3IfrFAhTos90p?=
 =?us-ascii?Q?hVhFO2mwoUQrx9VTqA2KBaMokalil6EAhrLWn78WnAFHaLlZrwfqhl8pFwFo?=
 =?us-ascii?Q?yVGOVkLUajqy/IxbuLMmJ50FUr0HVAOLoeqsRYj/SZG2mCKgcfSSIEDHXavI?=
 =?us-ascii?Q?RMETBPe68H8wswG9mCjJcMc00OafOPXTYIlVi4dtJRBdLsZqCpXRw/MiAlTY?=
 =?us-ascii?Q?kggdVpLrmTy7U+DceTPJ/ZDDt8sCDVQ4+eBrQaUmouTvClIL6K8yTZbn/OWp?=
 =?us-ascii?Q?4Dac6pgf3GtH9jAxGcFMUxbHjrDsjNMMMqJfCIwqH4X4NaQNdhAqVAQT7Sl5?=
 =?us-ascii?Q?0/hsFXlvBDxea9eMaOAZ2K3j/YTSTQJCgchkVfWdO7P+k8okZH5xoOXwwIB/?=
 =?us-ascii?Q?dtxNUeNjNliJ7C6nSSFnZL94VHTDXNJ9QU127KKhLSv3Ke6E7rA3BMmiSmCT?=
 =?us-ascii?Q?pbaaBrKyPTTDDiXoHdRdCY3V9w+pNqfyUUO4n4/cNOHnulgT7nJ2aTJOXWE7?=
 =?us-ascii?Q?m8LOSqlVFpcdjuqf66gVjo+Kmu9BIuFeMxSzCgcLbFydEkdu88NVHwfJlpzE?=
 =?us-ascii?Q?kC+mqhQgCOtujOKuI0LBXCQLEe4go0cgRweOxzgqXtP/EpNSl/fzrhKg1u2K?=
 =?us-ascii?Q?2qn4y8GOezZw2KcdYPp3LIWfLuF77W9flnKLZCx/EtV7CJ4UcSPMCI2tKVh1?=
 =?us-ascii?Q?o2YhP47JYWMIvR2yf2LuitkVHS/Wlc1DfQr/QvDFHE6b0r703RAj/AsQzA5x?=
 =?us-ascii?Q?jSIhxEIWyvOHnFKV6JhWkuS8/JBsJFPx4+vOXXj23QLF0gPsasnjoG2nrMjX?=
 =?us-ascii?Q?9MsrXv1R5sHQu5pH0bmEfCwhsEh5G1Ub5DcuRBULyCgE6Mbj+BTpKtbbW9E5?=
 =?us-ascii?Q?MVj0cvLaxLGmoNWxL4umrnydxE/5ork+9LjSzFdHdfyhLa7cRBSFLyalGCNi?=
 =?us-ascii?Q?yExRtcfqkRg8z1HrD7zqsVM5LWLUYtGbzqaKASz1pfLp5TAFfvVUs7PNYeBl?=
 =?us-ascii?Q?1J+Iac9m0OPEsLBZTFMssz+JPZ9EgA45YTwxIz437/ZnkorChlZtUvIdw/u2?=
 =?us-ascii?Q?rIYNrhwfp0lKDDGaFe4kq/sYbxQtSMSVrhEy0YRsZ8CPUL+2XdjdMj7Tqiih?=
 =?us-ascii?Q?/97lTKQaqbd4wq8OawkLIbELBEZ1RxJWqi5JC7yVzKsGcxqwm3LHVOK3c8GH?=
 =?us-ascii?Q?xYZ9ZQleJOj0OKJO5KE2meKxY0sJqvi0qz1sGsAsxuvqbEozhQWPsbGsar4U?=
 =?us-ascii?Q?807ATigpM5croJsMZNPkiZw1odtTVjaN+XNV743wx3ki2X+K0rzynY4Ak3B6?=
 =?us-ascii?Q?S7mg5cNxDBjjVdvp4S2nSnRBvk7a1zziK1DgOlSx4xb4JMo4nYNMiB5FJy7b?=
 =?us-ascii?Q?VmDH7E6WQGTKN5w1UVyu354iVCPtMfhjx7H6RHwBo6olVAJfrX9BYo1Hc+sI?=
 =?us-ascii?Q?VXBWkSuzW+EVCkyWbQ0F+0Djtfrfkl64hVvJ9h+4Hm5AANU6HZKOqJzcBfZe?=
 =?us-ascii?Q?Sw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c179e12f-1b07-4070-3a79-08db26b34906
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2023 06:46:07.0609
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: x0hCsyxv9ehDoqNDgp0XHuUezXx+/y0aRC2y9VhnE+FJaZTBhx137Utpbe1Ov/ddXhmTDxckwhQ+nnhsiS04Mw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB8253
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Reviewed-by: Yan Zhao <yan.y.zhao@intel.com>

On Fri, Mar 10, 2023 at 04:22:41PM -0800, Sean Christopherson wrote:
> Use vgpu_lock instead of KVM's mmu_lock to protect accesses to the hash
> table used to track which gfns are write-protected when shadowing the
> guest's GTT, and hoist the acquisition of vgpu_lock from
> intel_vgpu_page_track_handler() out to its sole caller,
> kvmgt_page_track_write().
> 
> This fixes a bug where kvmgt_page_track_write(), which doesn't hold
> kvm->mmu_lock, could race with intel_gvt_page_track_remove() and trigger
> a use-after-free.
> 
> Fixing kvmgt_page_track_write() by taking kvm->mmu_lock is not an option
> as mmu_lock is a r/w spinlock, and intel_vgpu_page_track_handler() might
> sleep when acquiring vgpu->cache_lock deep down the callstack:
> 
>   intel_vgpu_page_track_handler()
>   |
>   |->  page_track->handler / ppgtt_write_protection_handler()
>        |
>        |-> ppgtt_handle_guest_write_page_table_bytes()
>            |
>            |->  ppgtt_handle_guest_write_page_table()
>                 |
>                 |-> ppgtt_handle_guest_entry_removal()
>                     |
>                     |-> ppgtt_invalidate_pte()
>                         |
>                         |-> intel_gvt_dma_unmap_guest_page()
>                             |
>                             |-> mutex_lock(&vgpu->cache_lock);
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c      | 55 +++++++++++++++------------
>  drivers/gpu/drm/i915/gvt/page_track.c | 10 +----
>  2 files changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 68be66395598..9824d075562e 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -366,6 +366,8 @@ __kvmgt_protect_table_find(struct intel_vgpu *info, gfn_t gfn)
>  {
>  	struct kvmgt_pgfn *p, *res = NULL;
>  
> +	lockdep_assert_held(&info->vgpu_lock);
> +
>  	hash_for_each_possible(info->ptable, p, hnode, gfn) {
>  		if (gfn == p->gfn) {
>  			res = p;
> @@ -1567,6 +1569,9 @@ int intel_gvt_page_track_add(struct intel_vgpu *info, u64 gfn)
>  	if (!test_bit(INTEL_VGPU_STATUS_ATTACHED, info->status))
>  		return -ESRCH;
>  
> +	if (kvmgt_gfn_is_write_protected(info, gfn))
> +		return 0;
> +
>  	idx = srcu_read_lock(&kvm->srcu);
>  	slot = gfn_to_memslot(kvm, gfn);
>  	if (!slot) {
> @@ -1575,16 +1580,12 @@ int intel_gvt_page_track_add(struct intel_vgpu *info, u64 gfn)
>  	}
>  
>  	write_lock(&kvm->mmu_lock);
> -
> -	if (kvmgt_gfn_is_write_protected(info, gfn))
> -		goto out;
> -
>  	kvm_slot_page_track_add_page(kvm, slot, gfn, KVM_PAGE_TRACK_WRITE);
> +	write_unlock(&kvm->mmu_lock);
> +
> +	srcu_read_unlock(&kvm->srcu, idx);
> +
>  	kvmgt_protect_table_add(info, gfn);
> -
> -out:
> -	write_unlock(&kvm->mmu_lock);
> -	srcu_read_unlock(&kvm->srcu, idx);
>  	return 0;
>  }
>  
> @@ -1597,24 +1598,22 @@ int intel_gvt_page_track_remove(struct intel_vgpu *info, u64 gfn)
>  	if (!test_bit(INTEL_VGPU_STATUS_ATTACHED, info->status))
>  		return -ESRCH;
>  
> -	idx = srcu_read_lock(&kvm->srcu);
> -	slot = gfn_to_memslot(kvm, gfn);
> -	if (!slot) {
> -		srcu_read_unlock(&kvm->srcu, idx);
> -		return -EINVAL;
> -	}
> -
> -	write_lock(&kvm->mmu_lock);
> -
>  	if (!kvmgt_gfn_is_write_protected(info, gfn))
> -		goto out;
> +		return 0;
>  
> +	idx = srcu_read_lock(&kvm->srcu);
> +	slot = gfn_to_memslot(kvm, gfn);
> +	if (!slot) {
> +		srcu_read_unlock(&kvm->srcu, idx);
> +		return -EINVAL;
> +	}
> +
> +	write_lock(&kvm->mmu_lock);
>  	kvm_slot_page_track_remove_page(kvm, slot, gfn, KVM_PAGE_TRACK_WRITE);
> +	write_unlock(&kvm->mmu_lock);
> +	srcu_read_unlock(&kvm->srcu, idx);
> +
>  	kvmgt_protect_table_del(info, gfn);
> -
> -out:
> -	write_unlock(&kvm->mmu_lock);
> -	srcu_read_unlock(&kvm->srcu, idx);
>  	return 0;
>  }
>  
> @@ -1625,9 +1624,13 @@ static void kvmgt_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa,
>  	struct intel_vgpu *info =
>  		container_of(node, struct intel_vgpu, track_node);
>  
> +	mutex_lock(&info->vgpu_lock);
> +
>  	if (kvmgt_gfn_is_write_protected(info, gpa_to_gfn(gpa)))
>  		intel_vgpu_page_track_handler(info, gpa,
>  						     (void *)val, len);
> +
> +	mutex_unlock(&info->vgpu_lock);
>  }
>  
>  static void kvmgt_page_track_flush_slot(struct kvm *kvm,
> @@ -1639,16 +1642,20 @@ static void kvmgt_page_track_flush_slot(struct kvm *kvm,
>  	struct intel_vgpu *info =
>  		container_of(node, struct intel_vgpu, track_node);
>  
> -	write_lock(&kvm->mmu_lock);
> +	mutex_lock(&info->vgpu_lock);
> +
>  	for (i = 0; i < slot->npages; i++) {
>  		gfn = slot->base_gfn + i;
>  		if (kvmgt_gfn_is_write_protected(info, gfn)) {
> +			write_lock(&kvm->mmu_lock);
>  			kvm_slot_page_track_remove_page(kvm, slot, gfn,
>  						KVM_PAGE_TRACK_WRITE);
> +			write_unlock(&kvm->mmu_lock);
> +
>  			kvmgt_protect_table_del(info, gfn);
>  		}
>  	}
> -	write_unlock(&kvm->mmu_lock);
> +	mutex_unlock(&info->vgpu_lock);
>  }
>  
>  void intel_vgpu_detach_regions(struct intel_vgpu *vgpu)
> diff --git a/drivers/gpu/drm/i915/gvt/page_track.c b/drivers/gpu/drm/i915/gvt/page_track.c
> index df34e73cba41..60a65435556d 100644
> --- a/drivers/gpu/drm/i915/gvt/page_track.c
> +++ b/drivers/gpu/drm/i915/gvt/page_track.c
> @@ -162,13 +162,9 @@ int intel_vgpu_page_track_handler(struct intel_vgpu *vgpu, u64 gpa,
>  	struct intel_vgpu_page_track *page_track;
>  	int ret = 0;
>  
> -	mutex_lock(&vgpu->vgpu_lock);
> -
>  	page_track = intel_vgpu_find_page_track(vgpu, gpa >> PAGE_SHIFT);
> -	if (!page_track) {
> -		ret = -ENXIO;
> -		goto out;
> -	}
> +	if (!page_track)
> +		return -ENXIO;
>  
>  	if (unlikely(vgpu->failsafe)) {
>  		/* Remove write protection to prevent furture traps. */
> @@ -179,7 +175,5 @@ int intel_vgpu_page_track_handler(struct intel_vgpu *vgpu, u64 gpa,
>  			gvt_err("guest page write error, gpa %llx\n", gpa);
>  	}
>  
> -out:
> -	mutex_unlock(&vgpu->vgpu_lock);
>  	return ret;
>  }
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
>