Re: [PATCH v4 05/16] KVM: Annotate -EFAULTs from kvm_vcpu_write_guest_page()

[Sean Christopherson <seanjc@google.com>]

On Fri, Jun 02, 2023, Anish Moorthy wrote:
> Implement KVM_CAP_MEMORY_FAULT_INFO for uaccess failures in
> kvm_vcpu_write_guest_page()
> 
> Signed-off-by: Anish Moorthy <amoorthy@google.com>
> ---
>  virt/kvm/kvm_main.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d9c0fa7c907f..ea27a8178f1a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3090,8 +3090,10 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_read_guest_atomic);
>  
>  /*
>   * Copy 'len' bytes from 'data' into guest memory at '(gfn * PAGE_SIZE) + offset'
> + * If 'vcpu' is non-null, then may fill its run struct for a
> + * KVM_EXIT_MEMORY_FAULT on uaccess failure.
>   */
> -static int __kvm_write_guest_page(struct kvm *kvm,
> +static int __kvm_write_guest_page(struct kvm *kvm, struct kvm_vcpu *vcpu,
>  				  struct kvm_memory_slot *memslot, gfn_t gfn,
>  			          const void *data, int offset, int len)
>  {
> @@ -3102,8 +3104,13 @@ static int __kvm_write_guest_page(struct kvm *kvm,
>  	if (kvm_is_error_hva(addr))
>  		return -EFAULT;
>  	r = __copy_to_user((void __user *)addr + offset, data, len);
> -	if (r)
> +	if (r) {
> +		if (vcpu)

As mentioned in a previous mail, put this in the (one) caller.  If more callers
come along, which is highly unlikely, we can revisit that decision.  Right now,
it just adds noise, both here and in the helper.

> +			kvm_populate_efault_info(vcpu, gfn * PAGE_SIZE + offset,
> +						 len,
> +						 KVM_MEMORY_FAULT_FLAG_WRITE);

For future reference, the 80 char limit is a soft limit, and with a lot of
subjectivity, can be breached.  In this case, this

			kvm_populate_efault_info(vcpu, gfn * PAGE_SIZE + offset,
						 len, KVM_MEMORY_FAULT_FLAG_WRITE);

is subjectively more readable than

			kvm_populate_efault_info(vcpu, gfn * PAGE_SIZE + offset,
						 len,
						 KVM_MEMORY_FAULT_FLAG_WRITE);
>  		return -EFAULT;
> +	}
>  	mark_page_dirty_in_slot(kvm, memslot, gfn);
>  	return 0;
>  }
> @@ -3113,7 +3120,7 @@ int kvm_write_guest_page(struct kvm *kvm, gfn_t gfn,
>  {
>  	struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
>  
> -	return __kvm_write_guest_page(kvm, slot, gfn, data, offset, len);
> +	return __kvm_write_guest_page(kvm, NULL, slot, gfn, data, offset, len);
>  }
>  EXPORT_SYMBOL_GPL(kvm_write_guest_page);
>  
> @@ -3121,8 +3128,8 @@ int kvm_vcpu_write_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn,
>  			      const void *data, int offset, int len)
>  {
>  	struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
> -

Newline after variable declarations.  Double demerits for breaking what was
originally correct :-)

> -	return __kvm_write_guest_page(vcpu->kvm, slot, gfn, data, offset, len);
> +	return __kvm_write_guest_page(vcpu->kvm, vcpu, slot, gfn, data,
> +				      offset, len);

With my various suggestions, something like

	struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
	int r;

	r = __kvm_write_guest_page(vcpu->kvm, slot, gfn, data, offset, len);
	if (r)
		kvm_handle_guest_uaccess_fault(...);
	return r;

Side topic, "uaccess", and thus any "userfault" variants, is probably a bad name
for the API, because that will fall apart when guest private memory comes along.