From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH RESEND] kvm:x86:Fix error handling in the function kvm_write_wall_clock Date: Thu, 17 Dec 2015 11:36:35 +0100 Message-ID: <56729033.80706@redhat.com> References: <1450319418-29032-1-git-send-email-xerofoify@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: x86@kernel.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org To: Nicholas Krause , gleb@kernel.org Return-path: In-Reply-To: <1450319418-29032-1-git-send-email-xerofoify@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 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 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) >