[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 01/38] exec: Fix memory allocation when memory p
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 01/38] exec: Fix memory allocation when memory path names new file |
Date: |
Fri, 04 Mar 2016 19:50:52 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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?