| Message ID | 20161027042300.5929-2-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 27, 2016 at 12:22:58PM +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 <haozhong.zhang@intel.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> But I would add comment near the get_file_size() call to indicate that not stopping on get_file_size() errors is on purpose and not a mistake. > --- > exec.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/exec.c b/exec.c > index 587b489..a2b371a 100644 > --- a/exec.c > +++ b/exec.c > @@ -1224,6 +1224,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, > @@ -1235,6 +1244,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, > @@ -1297,6 +1307,8 @@ static void *file_ram_alloc(RAMBlock *block, > } > #endif > > + file_size = get_file_size(fd); > + > 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", > @@ -1311,8 +1323,16 @@ static void *file_ram_alloc(RAMBlock *block, > * hosts, so don't bother bailing out on errors. > * If anything goes wrong with it under other filesystems, > * mmap will fail. > + * > + * Do not truncate the non-empty backend file to avoid corrupting > + * 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 (ftruncate(fd, memory)) { > + if (!file_size && ftruncate(fd, memory)) { > perror("ftruncate"); > } > > -- > 2.10.1 >
On 10/27/16 12:31 -0200, Eduardo Habkost wrote: >On Thu, Oct 27, 2016 at 12:22:58PM +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 <haozhong.zhang@intel.com> > >Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > >But I would add comment near the get_file_size() call to indicate >that not stopping on get_file_size() errors is on purpose and not >a mistake. > I'll add comments in the next version. Thanks, Haozhong >> --- >> exec.c | 22 +++++++++++++++++++++- >> 1 file changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/exec.c b/exec.c >> index 587b489..a2b371a 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -1224,6 +1224,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, >> @@ -1235,6 +1244,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, >> @@ -1297,6 +1307,8 @@ static void *file_ram_alloc(RAMBlock *block, >> } >> #endif >> >> + file_size = get_file_size(fd); >> + >> 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", >> @@ -1311,8 +1323,16 @@ static void *file_ram_alloc(RAMBlock *block, >> * hosts, so don't bother bailing out on errors. >> * If anything goes wrong with it under other filesystems, >> * mmap will fail. >> + * >> + * Do not truncate the non-empty backend file to avoid corrupting >> + * 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 (ftruncate(fd, memory)) { >> + if (!file_size && ftruncate(fd, memory)) { >> perror("ftruncate"); >> } >> >> -- >> 2.10.1 >> > >-- >Eduardo
On Fri, Oct 28, 2016 at 10:07:40AM +0800, Haozhong Zhang wrote: > On 10/27/16 12:31 -0200, Eduardo Habkost wrote: > > On Thu, Oct 27, 2016 at 12:22:58PM +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 <haozhong.zhang@intel.com> > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > But I would add comment near the get_file_size() call to indicate > > that not stopping on get_file_size() errors is on purpose and not > > a mistake. > > > > I'll add comments in the next version. > The patch was applied to machine-next and will be in my next pull request. The comment can be sent as a follow-up patch.
diff --git a/exec.c b/exec.c index 587b489..a2b371a 100644 --- a/exec.c +++ b/exec.c @@ -1224,6 +1224,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, @@ -1235,6 +1244,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, @@ -1297,6 +1307,8 @@ static void *file_ram_alloc(RAMBlock *block, } #endif + file_size = get_file_size(fd); + 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", @@ -1311,8 +1323,16 @@ static void *file_ram_alloc(RAMBlock *block, * hosts, so don't bother bailing out on errors. * If anything goes wrong with it under other filesystems, * mmap will fail. + * + * Do not truncate the non-empty backend file to avoid corrupting + * 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 (ftruncate(fd, memory)) { + if (!file_size && ftruncate(fd, memory)) { perror("ftruncate"); }
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 <haozhong.zhang@intel.com> --- exec.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)