Re: [PATCH RESEND] kvm:x86:Fix error handling in the function kvm_write_wall_clock

Re: [PATCH RESEND] kvm:x86:Fix error handling in the function kvm_write_wall_clock

[Paolo Bonzini]

On 17/12/2015 03:30, Nicholas Krause wrote:
> This fixes error handling in the function kvm_write_wall_clock
> by checking if any of the calls to kvm_write_guest have failed
> inside this paricutlar function and if so print to the console
> with pr_err that we are unable to write the data to the guest
> system to warn the user of this failure before directly returning
> to the caller of the function kvm_write_wall_check as we cannot
> continue the function to this function after a failed call to
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>

I think I have explained before why this is mostly unnecessary, but the
patch can be saved.  I'll go through the problems again first.

> -	kvm_write_guest(kvm, wall_clock, &version, sizeof(version));
> +	if (kvm_write_guest(kvm, wall_clock, &version, sizeof(version))) {
> +		pr_err("Unable to correctly write data to guest system\n");
> +		return;
> +	}

You've written a message to the log that can be triggered by the guest
(by writing an invalid value to the wall clock MSR).  We don't let the
guest spam the host logs.

>  	/*
>  	 * The guest calculates current wall clock time by adding
> @@ -1168,10 +1171,16 @@ static void kvm_write_wall_clock(struct kvm *kvm, gpa_t wall_clock)
>  	wc.nsec = boot.tv_nsec;
>  	wc.version = version;
>  
> -	kvm_write_guest(kvm, wall_clock, &wc, sizeof(wc));
> +	if (kvm_write_guest(kvm, wall_clock, &wc, sizeof(wc))) {
> +		pr_err("Unable to correctly write data to guest system\n");
> +		return;
> +	}

The other kvm_write_guest will probably also fail but, if it doesn't,
you've left an odd version in the data structure. The guest will loop
forever waiting for the even value.

Plus, same problem with logs.

>  	version++;
> -	kvm_write_guest(kvm, wall_clock, &version, sizeof(version));
> +	if (kvm_write_guest(kvm, wall_clock, &version, sizeof(version))) {
> +		pr_err("Unable to correctly write data to guest system\n");
> +		return;
> +	}
>  }

Same problem with logs, and the return is not useful.

Can you send a patch that only adds a return if the *first*
kvm_write_guest fails?  You can leave aside the others, and not add any
pr_err.

Thanks,

Paolo

>  static uint32_t div_frac(uint32_t dividend, uint32_t divisor)
>