From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756043AbbLQKgm (ORCPT ); Thu, 17 Dec 2015 05:36:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47805 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755141AbbLQKgk (ORCPT ); Thu, 17 Dec 2015 05:36:40 -0500 Subject: Re: [PATCH RESEND] kvm:x86:Fix error handling in the function kvm_write_wall_clock To: Nicholas Krause , gleb@kernel.org References: <1450319418-29032-1-git-send-email-xerofoify@gmail.com> Cc: x86@kernel.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org From: Paolo Bonzini Message-ID: <56729033.80706@redhat.com> Date: Thu, 17 Dec 2015 11:36:35 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1450319418-29032-1-git-send-email-xerofoify@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@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) >