From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:34750)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1abuoV-000525-3o
	for qemu-devel@nongnu.org; Fri, 04 Mar 2016 13:51:00 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1abuoR-0004LB-TC
	for qemu-devel@nongnu.org; Fri, 04 Mar 2016 13:50:59 -0500
Received: from mx1.redhat.com ([209.132.183.28]:55707)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1abuoR-0004L7-LK
	for qemu-devel@nongnu.org; Fri, 04 Mar 2016 13:50:55 -0500
From: Markus Armbruster <armbru@redhat.com>
References: <1456771254-17511-1-git-send-email-armbru@redhat.com>
	<1456771254-17511-2-git-send-email-armbru@redhat.com>
	<56D57E76.1080402@redhat.com>
Date: Fri, 04 Mar 2016 19:50:52 +0100
In-Reply-To: <56D57E76.1080402@redhat.com> (Paolo Bonzini's message of "Tue, 1
	Mar 2016 12:35:18 +0100")
Message-ID: <87k2li5b77.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH 01/38] exec: Fix memory allocation when
	memory path names new file
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: claudio.fontana@huawei.com, cam@cs.ualberta.ca, mlureau@redhat.com, qemu-devel@nongnu.org, david.marchand@6wind.com

Paolo Bonzini <pbonzini@redhat.com> 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?