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 <guangrong.xiao@xxxxxxxxxxxxxxx>
> >[...]
> >>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 <ehabkost@xxxxxxxxxx>
--
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
