Re: [Qemu-devel] [PATCH 01/38] exec: Fix memory allocation when memory path names new file

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> On 29/02/2016 19:40, Markus Armbruster wrote:
>> -    if (!stat(path, &st) && S_ISDIR(st.st_mode)) {
>> +    ret = stat(path, &st);
>> +    if (!ret && S_ISDIR(st.st_mode)) {
>> +        /* path names a directory -> create a temporary file there */
>>          /* Make name safe to use with mkstemp by replacing '/' with '_'. */
>>          sanitized_name = g_strdup(memory_region_name(block->mr));
>>          for (c = sanitized_name; *c != '\0'; c++) {
>> @@ -1282,13 +1271,32 @@ static void *file_ram_alloc(RAMBlock *block,
>>              unlink(filename);
>>          }
>>          g_free(filename);
>> +    } else if (!ret) {
>> +        /* path names an existing file -> use it */
>> +        fd = open(path, O_RDWR);
>>      } else {
>> +        /* create a new file */
>>          fd = open(path, O_RDWR | O_CREAT, 0644);
>> +        unlink_on_error = true;
>>      }
>
> While at it, let's avoid TOCTTOU conditions:
>
>     for (;;) {
>         fd = open(path, O_RDWR);
>         if (fd != -1) {
>             break;
>         }
>         if (errno == ENOENT) {
>             fd = open(path, O_RDWR | O_CREAT | O_EXCL, 0644);
>             if (fd != -1) {
>                 unlink_on_error = true;
>                 break;
>             }
>         } else if (errno == EISDIR) {
>             ... mkstemp ...
>             if (fd != -1) {
>                 unlink_on_error = true;
>                 break;
>             }
>         }
>         if (errno != EEXIST && errno != EINTR) {
>             goto error;
>         }
>     }
>
> and use fstatfs in gethugepagesize.

A question on gethugepagesize().  We have a couple of copies.

Here's target-ppc/kvm.c's:

    static long gethugepagesize(const char *mem_path)
    {
        struct statfs fs;
        int ret;

        do {
            ret = statfs(mem_path, &fs);
        } while (ret != 0 && errno == EINTR);

        if (ret != 0) {
            fprintf(stderr, "Couldn't statfs() memory path: %s\n",
                    strerror(errno));
            exit(1);
        }

    #define HUGETLBFS_MAGIC       0x958458f6

        if (fs.f_type != HUGETLBFS_MAGIC) {
            /* Explicit mempath, but it's ordinary pages */
            return getpagesize();
        }

        /* It's hugepage, return the huge page size */
        return fs.f_bsize;
    }

I guess the use of HUGETLBFS_MAGIC is fine since kvm.c is Linux-specific.

There's another one in ivshmem_server.c, functionally identical and
wrapped in CONFIG_LINUX.

Here's exec.c's:

    #define HUGETLBFS_MAGIC       0x958458f6

    static long gethugepagesize(const char *path, Error **errp)
    {
        struct statfs fs;
        int ret;

        do {
            ret = statfs(path, &fs);
        } while (ret != 0 && errno == EINTR);

        if (ret != 0) {
            error_setg_errno(errp, errno, "failed to get page size of file %s",
                             path);
            return 0;
        }

        return fs.f_bsize;
    }

Before commit bfc2a1a, it additionally had

    if (fs.f_type != HUGETLBFS_MAGIC)
        fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path);

Note the lack of "if not hugetlbfs, use getpagesize()" logic.

Here's util/mmap-alloc.c's:

    #define HUGETLBFS_MAGIC       0x958458f6

    #ifdef CONFIG_LINUX
    #include <sys/vfs.h>
    #endif

    size_t qemu_fd_getpagesize(int fd)
    {
    #ifdef CONFIG_LINUX
        struct statfs fs;
        int ret;

        if (fd != -1) {
            do {
                ret = fstatfs(fd, &fs);
            } while (ret != 0 && errno == EINTR);

            if (ret == 0 && fs.f_type == HUGETLBFS_MAGIC) {
                return fs.f_bsize;
            }
        }
    #endif

        return getpagesize();
    }

Would you like me to convert the others users to this one and drop the
dupes?