From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52794) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bz7QJ-00038l-7C for qemu-devel@nongnu.org; Tue, 25 Oct 2016 15:30:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bz7QE-0007Rf-AU for qemu-devel@nongnu.org; Tue, 25 Oct 2016 15:30:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50728) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bz7QE-0007RB-3Q for qemu-devel@nongnu.org; Tue, 25 Oct 2016 15:30:06 -0400 Date: Tue, 25 Oct 2016 17:30:02 -0200 From: Eduardo Habkost Message-ID: <20161025193002.GW5057@thinpad.lan.raisama.net> References: <20161024092151.32386-1-haozhong.zhang@intel.com> <20161024092151.32386-2-haozhong.zhang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161024092151.32386-2-haozhong.zhang@intel.com> Subject: Re: [Qemu-devel] [PATCH 1/2] exec.c: do not truncate non-empty memory backend file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Haozhong Zhang Cc: qemu-devel@nongnu.org, Igor Mammedov , Paolo Bonzini , Peter Crosthwaite , Richard Henderson , Xiao Guangrong On Mon, Oct 24, 2016 at 05:21:50PM +0800, Haozhong Zhang wrote: > For '-object memory-backend-file,mem-path=foo,size=xyz', if the size of > file 'foo' does not match the given size 'xyz', the current QEMU will > truncate the file to the given size, which may corrupt the existing data > in that file. To avoid such data corruption, this patch disables > truncating non-empty backend files. > > Signed-off-by: Haozhong Zhang > --- > exec.c | 37 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) > > diff --git a/exec.c b/exec.c > index e63c5a1..95983c9 100644 > --- a/exec.c > +++ b/exec.c > @@ -1188,6 +1188,15 @@ void qemu_mutex_unlock_ramlist(void) > } > > #ifdef __linux__ > +static int64_t get_file_size(int fd) > +{ > + int64_t size = lseek(fd, 0, SEEK_END); > + if (size < 0) { > + return -errno; > + } > + return size; > +} > + > static void *file_ram_alloc(RAMBlock *block, > ram_addr_t memory, > const char *path, > @@ -1199,6 +1208,7 @@ static void *file_ram_alloc(RAMBlock *block, > char *c; > void *area = MAP_FAILED; > int fd = -1; > + int64_t file_size; > > if (kvm_enabled() && !kvm_has_sync_mmu()) { > error_setg(errp, > @@ -1256,6 +1266,14 @@ static void *file_ram_alloc(RAMBlock *block, > block->page_size = qemu_fd_getpagesize(fd); > block->mr->align = MAX(block->page_size, QEMU_VMALLOC_ALIGN); > > + file_size = get_file_size(fd); > + if (file_size < 0) { > + error_setg_errno(errp, file_size, > + "can't get size of backing store %s", > + path); What about block devices or filesystems where lseek(SEEK_END) is not supported? They work today, and would break with this patch. I suggest just continuing without any errors (and not truncating the file) in case it is not possible to get the file size. > + goto error; > + } > + > if (memory < block->page_size) { > error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to " > "or larger than page size 0x%zx", > @@ -1266,12 +1284,29 @@ static void *file_ram_alloc(RAMBlock *block, > memory = ROUND_UP(memory, block->page_size); > > /* > + * Do not extend/shrink the backend file if it's not empty, or its > + * size does not match the aligned 'size=xxx' option. Otherwise, > + * it is possible to corrupt the existing data in the file. > + * > + * Disabling shrinking is not enough. For example, the current > + * vNVDIMM implementation stores the guest NVDIMM labels at the > + * end of the backend file. If the backend file is later extended, > + * QEMU will not be able to find those labels. Therefore, > + * extending the non-empty backend file is disabled as well. > + */ > + if (file_size && file_size != memory) { > + error_setg(errp, "backing store %s size %"PRId64 > + " does not math with aligned 'size' option %"PRIu64, Did you mean "specified 'size' option"? > + path, file_size, memory); We already support size being smaller than the backing file and people may rely on it, so we shouldn't change this behavior. This can be changed to: if (file_size > 0 && file_size < memory) I also suggest doing this check in a separate patch. The two changes (skipping truncation of non-empty files and exiting on size mismatch) don't depend on each other. > + goto error; > + } > + /* > * ftruncate is not supported by hugetlbfs in older > * hosts, so don't bother bailing out on errors. > * If anything goes wrong with it under other filesystems, > * mmap will fail. > */ > - if (ftruncate(fd, memory)) { > + if (!file_size && ftruncate(fd, memory)) { > perror("ftruncate"); > } > > -- > 2.10.1 > -- Eduardo