Re: [Qemu-devel] [PATCH v7 07/35] util: introduce qemu_file_get_page_size()

[Eduardo Habkost]

On Mon, Nov 09, 2015 at 12:36:36PM +0800, Xiao Guangrong wrote:
> On 11/06/2015 11:36 PM, Eduardo Habkost wrote:
> >On Mon, Nov 02, 2015 at 05:13:09PM +0800, Xiao Guangrong wrote:
> >>There are three places use the some logic to get the page size on
> >>the file path or file fd
> >>
> >>Windows did not support file hugepage, so it will return normal page
> >>for this case. And this interface has not been used on windows so far
> >>
> >>This patch introduces qemu_file_get_page_size() to unify the code
> >>
> >>Signed-off-by: Xiao Guangrong <address@hidden>
> >[...]
> >>diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> >>index 914cef5..51437ff 100644
> >>--- a/util/oslib-posix.c
> >>+++ b/util/oslib-posix.c
> >>@@ -340,7 +340,7 @@ static void sigbus_handler(int signal)
> >>      siglongjmp(sigjump, 1);
> >>  }
> >>
> >>-static size_t fd_getpagesize(int fd)
> >>+static size_t fd_getpagesize(int fd, Error **errp)
> >>  {
> >>  #ifdef CONFIG_LINUX
> >>      struct statfs fs;
> >>@@ -351,7 +351,12 @@ static size_t fd_getpagesize(int fd)
> >>              ret = fstatfs(fd, &fs);
> >>          } while (ret != 0 && errno == EINTR);
> >>
> >>-        if (ret == 0 && fs.f_type == HUGETLBFS_MAGIC) {
> >>+        if (ret) {
> >>+            error_setg_errno(errp, errno, "fstatfs is failed");
> >>+            return 0;
> >>+        }
> >>+
> >>+        if (fs.f_type == HUGETLBFS_MAGIC) {
> >>              return fs.f_bsize;
> >>          }
> >
> >You are changing os_mem_prealloc() behavior when fstatfs() fails, here.
> >Have you ensured there are no cases where fstatfs() fails but this code
> >is still expected to work?
> 
> stat() is supported for all kinds of files, so failed stat() is caused by
> file is not exist or kernel internal error (e,g memory is not enough) or
> security check is not passed. Whichever we should not do any operation on
> the file if stat() failed. The origin code did not check it but it is worth
> being fixed i think.

Note that this is fstatfs(), not stat(). It's possible go get
ENOSYS as error from statfs() if it is not implemented by the
filesystem, I just don't know if this really can happen in
practice.

(But the answer won't matter, as we already aborted on statfs()
errors on all codepaths that call fd_getpagesize(). See below.)

> 
> >
> >The change looks safe: gethugepagesize() seems to be always called in
> >the path that would make fd_getpagesize() be called from
> >os_mem_prealloc(), so allocation would abort much earlier if statfs()
> >failed. But I haven't confirmed that yet, and I wanted to be sure.
> >
> 
> Yes, I am entirely agree with you. :)
> 

I have just confirmed that this is the case, as:

* fd_getpagesize() is only called from os_mem_prealloc(),
* os_mem_prealloc() is called from:
  * host_memory_backend_set_prealloc()
    * Using memory_region_get_fd() as the fd argument
  * host_memory_backend_memory_complete()
    * Using memory_region_get_fd() as the fd argument
  * file_ram_alloc()
    * After qemu_file_get_page_size()/gethugepagesize() was already called
      in the same fd (with errors checked)
* fd_getpagesize() checks for fd == -1
* The only code that sets the fd for a RAMBlock is file_ram_alloc(),
  which checks for qemu_file_get_page_size()/gethugepagesize()
  errors

So, it was already impossible to get os_mem_prealloc() called
with fd != -1 without having gethugepagesize() called first (and
gethugepagesize() already checked for statfs() errors).

Reviewed-by: Eduardo Habkost <address@hidden>

-- 
Eduardo