Message-ID: <5288BD3D.1050302@nod.at>
Date:	Sun, 17 Nov 2013 13:57:33 +0100
From:	Richard Weinberger <richard@....at>
To:	Tristan Schmelcher <tschmelcher@...gle.com>
CC:	user-mode-linux-devel@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org, Thomas Meyer <thomas@...3r.de>
Subject: Re: [PATCH] uml: Simplify tempdir logic.

Am 11.11.2013 19:03, schrieb Tristan Schmelcher:
> From: Tristan Schmelcher <tschmelcher@...gle.com>
> 
> Inferring the mount hierarchy correctly from /proc/mounts is hard when MS_MOVE
> may have been used, and the previous code did it wrongly. This change simplifies
> the logic to only require that /dev/shm be _on_ tmpfs (which can be checked
> trivially with statfs) rather than that it be a _mountpoint_ of tmpfs, since
> there isn't a compelling reason to be that strict. We also now check for tmpfs
> on whatever directory we ultimately use so that the user is better informed.
> 
> This change also moves the more standard TMPDIR environment variable check ahead
> of the others.
> 
> Applies to 3.12.
> 
> Signed-off-by: Tristan Schmelcher <tschmelcher@...gle.com>

I like what I see but so far I didn't had to time to review your changes carefully.
Stay tuned. :)

Thanks,
//richard

> ---
>  arch/um/os-Linux/mem.c | 372 ++++++++++---------------------------------------
>  1 file changed, 75 insertions(+), 297 deletions(-)
> 
> diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c
> index 3c4af77..897e9ad 100644
> --- a/arch/um/os-Linux/mem.c
> +++ b/arch/um/os-Linux/mem.c
> @@ -12,337 +12,117 @@
>  #include <string.h>
>  #include <sys/stat.h>
>  #include <sys/mman.h>
> -#include <sys/param.h>
> +#include <sys/vfs.h>
> +#include <linux/magic.h>
>  #include <init.h>
>  #include <os.h>
>  
> -/* Modified by which_tmpdir, which is called during early boot */
> -static char *default_tmpdir = "/tmp";
> -
> -/*
> - *  Modified when creating the physical memory file and when checking
> - * the tmp filesystem for usability, both happening during early boot.
> - */
> +/* Set by make_tempfile() during early boot. */
>  static char *tempdir = NULL;
>  
> -static void __init find_tempdir(void)
> +/* Check if dir is on tmpfs. Return 0 if yes, -1 if no or error. */
> +static int __init check_tmpfs(const char *dir)
>  {
> -	const char *dirs[] = { "TMP", "TEMP", "TMPDIR", NULL };
> -	int i;
> -	char *dir = NULL;
> -
> -	if (tempdir != NULL)
> -		/* We've already been called */
> -		return;
> -	for (i = 0; dirs[i]; i++) {
> -		dir = getenv(dirs[i]);
> -		if ((dir != NULL) && (*dir != '\0'))
> -			break;
> -	}
> -	if ((dir == NULL) || (*dir == '\0'))
> -		dir = default_tmpdir;
> +	struct statfs st;
>  
> -	tempdir = malloc(strlen(dir) + 2);
> -	if (tempdir == NULL) {
> -		fprintf(stderr, "Failed to malloc tempdir, "
> -			"errno = %d\n", errno);
> -		return;
> -	}
> -	strcpy(tempdir, dir);
> -	strcat(tempdir, "/");
> -}
> -
> -/*
> - * Remove bytes from the front of the buffer and refill it so that if there's a
> - * partial string that we care about, it will be completed, and we can recognize
> - * it.
> - */
> -static int pop(int fd, char *buf, size_t size, size_t npop)
> -{
> -	ssize_t n;
> -	size_t len = strlen(&buf[npop]);
> -
> -	memmove(buf, &buf[npop], len + 1);
> -	n = read(fd, &buf[len], size - len - 1);
> -	if (n < 0)
> -		return -errno;
> -
> -	buf[len + n] = '\0';
> -	return 1;
> -}
> -
> -/*
> - * This will return 1, with the first character in buf being the
> - * character following the next instance of c in the file.  This will
> - * read the file as needed.  If there's an error, -errno is returned;
> - * if the end of the file is reached, 0 is returned.
> - */
> -static int next(int fd, char *buf, size_t size, char c)
> -{
> -	ssize_t n;
> -	char *ptr;
> -
> -	while ((ptr = strchr(buf, c)) == NULL) {
> -		n = read(fd, buf, size - 1);
> -		if (n == 0)
> -			return 0;
> -		else if (n < 0)
> -			return -errno;
> -
> -		buf[n] = '\0';
> +	printf("Checking if %s is on tmpfs...", dir);
> +	if (statfs(dir, &st) < 0) {
> +		printf("%s\n", strerror(errno));
> +	} else if (st.f_type != TMPFS_MAGIC) {
> +		printf("no\n");
> +	} else {
> +		printf("OK\n");
> +		return 0;
>  	}
> -
> -	return pop(fd, buf, size, ptr - buf + 1);
> +	return -1;
>  }
>  
>  /*
> - * Decode an octal-escaped and space-terminated path of the form used by
> - * /proc/mounts. May be used to decode a path in-place. "out" must be at least
> - * as large as the input. The output is always null-terminated. "len" gets the
> - * length of the output, excluding the trailing null. Returns 0 if a full path
> - * was successfully decoded, otherwise an error.
> + * Choose the tempdir to use. We want something on tmpfs so that our memory is
> + * not subject to the host's vm.dirty_ratio. If a tempdir is specified in the
> + * environment, we use that even if it's not on tmpfs, but we warn the user.
> + * Otherwise, we try common tmpfs locations, and if no tmpfs directory is found
> + * then we fall back to /tmp.
>   */
> -static int decode_path(const char *in, char *out, size_t *len)
> +static char * __init choose_tempdir(void)
>  {
> -	char *first = out;
> -	int c;
> +	static const char * const vars[] = {
> +		"TMPDIR",
> +		"TMP",
> +		"TEMP",
> +		NULL
> +	};
> +	static const char fallback_dir[] = "/tmp";
> +	static const char * const tmpfs_dirs[] = {
> +		"/dev/shm",
> +		fallback_dir,
> +		NULL
> +	};
>  	int i;
> -	int ret = -EINVAL;
> -	while (1) {
> -		switch (*in) {
> -		case '\0':
> -			goto out;
> -
> -		case ' ':
> -			ret = 0;
> -			goto out;
> -
> -		case '\\':
> -			in++;
> -			c = 0;
> -			for (i = 0; i < 3; i++) {
> -				if (*in < '0' || *in > '7')
> -					goto out;
> -				c = (c << 3) | (*in++ - '0');
> -			}
> -			*(unsigned char *)out++ = (unsigned char) c;
> -			break;
> -
> -		default:
> -			*out++ = *in++;
> -			break;
> +	const char *dir;
> +
> +	printf("Checking environment variables for a tempdir...");
> +	for (i = 0; vars[i]; i++) {
> +		dir = getenv(vars[i]);
> +		if ((dir != NULL) && (*dir != '\0')) {
> +			printf("%s\n", dir);
> +			if (check_tmpfs(dir) >= 0)
> +				goto done;
> +			else
> +				goto warn;
>  		}
>  	}
> +	printf("none found\n");
>  
> -out:
> -	*out = '\0';
> -	*len = out - first;
> -	return ret;
> -}
> -
> -/*
> - * Computes the length of s when encoded with three-digit octal escape sequences
> - * for the characters in chars.
> - */
> -static size_t octal_encoded_length(const char *s, const char *chars)
> -{
> -	size_t len = strlen(s);
> -	while ((s = strpbrk(s, chars)) != NULL) {
> -		len += 3;
> -		s++;
> -	}
> -
> -	return len;
> -}
> -
> -enum {
> -	OUTCOME_NOTHING_MOUNTED,
> -	OUTCOME_TMPFS_MOUNT,
> -	OUTCOME_NON_TMPFS_MOUNT,
> -};
> -
> -/* Read a line of /proc/mounts data looking for a tmpfs mount at "path". */
> -static int read_mount(int fd, char *buf, size_t bufsize, const char *path,
> -		      int *outcome)
> -{
> -	int found;
> -	int match;
> -	char *space;
> -	size_t len;
> -
> -	enum {
> -		MATCH_NONE,
> -		MATCH_EXACT,
> -		MATCH_PARENT,
> -	};
> -
> -	found = next(fd, buf, bufsize, ' ');
> -	if (found != 1)
> -		return found;
> -
> -	/*
> -	 * If there's no following space in the buffer, then this path is
> -	 * truncated, so it can't be the one we're looking for.
> -	 */
> -	space = strchr(buf, ' ');
> -	if (space) {
> -		match = MATCH_NONE;
> -		if (!decode_path(buf, buf, &len)) {
> -			if (!strcmp(buf, path))
> -				match = MATCH_EXACT;
> -			else if (!strncmp(buf, path, len)
> -				 && (path[len] == '/' || !strcmp(buf, "/")))
> -				match = MATCH_PARENT;
> -		}
> -
> -		found = pop(fd, buf, bufsize, space - buf + 1);
> -		if (found != 1)
> -			return found;
> -
> -		switch (match) {
> -		case MATCH_EXACT:
> -			if (!strncmp(buf, "tmpfs", strlen("tmpfs")))
> -				*outcome = OUTCOME_TMPFS_MOUNT;
> -			else
> -				*outcome = OUTCOME_NON_TMPFS_MOUNT;
> -			break;
> -
> -		case MATCH_PARENT:
> -			/* This mount obscures any previous ones. */
> -			*outcome = OUTCOME_NOTHING_MOUNTED;
> -			break;
> -		}
> +	for (i = 0; tmpfs_dirs[i]; i++) {
> +		dir = tmpfs_dirs[i];
> +		if (check_tmpfs(dir) >= 0)
> +			goto done;
>  	}
>  
> -	return next(fd, buf, bufsize, '\n');
> +	dir = fallback_dir;
> +warn:
> +	printf("Warning: tempdir %s is not on tmpfs\n", dir);
> +done:
> +	/* Make a copy since getenv results may not remain valid forever. */
> +	return strdup(dir);
>  }
>  
> -/* which_tmpdir is called only during early boot */
> -static int checked_tmpdir = 0;
> -
>  /*
> - * Look for a tmpfs mounted at /dev/shm.  I couldn't find a cleaner
> - * way to do this than to parse /proc/mounts.  statfs will return the
> - * same filesystem magic number and fs id for both /dev and /dev/shm
> - * when they are both tmpfs, so you can't tell if they are different
> - * filesystems.  Also, there seems to be no other way of finding the
> - * mount point of a filesystem from within it.
> - *
> - * If a /dev/shm tmpfs entry is found, then we switch to using it.
> - * Otherwise, we stay with the default /tmp.
> + * Create an unlinked tempfile in a suitable tempdir. template must be the
> + * basename part of the template with a leading '/'.
>   */
> -static void which_tmpdir(void)
> +static int __init make_tempfile(const char *template)
>  {
> +	char *tempname;
>  	int fd;
> -	int found;
> -	int outcome;
> -	char *path;
> -	char *buf;
> -	size_t bufsize;
>  
> -	if (checked_tmpdir)
> -		return;
> -
> -	checked_tmpdir = 1;
> -
> -	printf("Checking for tmpfs mount on /dev/shm...");
> -
> -	path = realpath("/dev/shm", NULL);
> -	if (!path) {
> -		printf("failed to check real path, errno = %d\n", errno);
> -		return;
> -	}
> -	printf("%s...", path);
> -
> -	/*
> -	 * The buffer needs to be able to fit the full octal-escaped path, a
> -	 * space, and a trailing null in order to successfully decode it.
> -	 */
> -	bufsize = octal_encoded_length(path, " \t\n\\") + 2;
> -
> -	if (bufsize < 128)
> -		bufsize = 128;
> -
> -	buf = malloc(bufsize);
> -	if (!buf) {
> -		printf("malloc failed, errno = %d\n", errno);
> -		goto out;
> -	}
> -	buf[0] = '\0';
> -
> -	fd = open("/proc/mounts", O_RDONLY);
> -	if (fd < 0) {
> -		printf("failed to open /proc/mounts, errno = %d\n", errno);
> -		goto out1;
> -	}
> -
> -	outcome = OUTCOME_NOTHING_MOUNTED;
> -	while (1) {
> -		found = read_mount(fd, buf, bufsize, path, &outcome);
> -		if (found != 1)
> -			break;
> -	}
> -
> -	if (found < 0) {
> -		printf("read returned errno %d\n", -found);
> -	} else {
> -		switch (outcome) {
> -		case OUTCOME_TMPFS_MOUNT:
> -			printf("OK\n");
> -			default_tmpdir = "/dev/shm";
> -			break;
> -
> -		case OUTCOME_NON_TMPFS_MOUNT:
> -			printf("not tmpfs\n");
> -			break;
> -
> -		default:
> -			printf("nothing mounted on /dev/shm\n");
> -			break;
> +	if (tempdir == NULL) {
> +		tempdir = choose_tempdir();
> +		if (tempdir == NULL) {
> +			fprintf(stderr, "Failed to choose tempdir: %s\n",
> +				strerror(errno));
> +			return -1;
>  		}
>  	}
>  
> -	close(fd);
> -out1:
> -	free(buf);
> -out:
> -	free(path);
> -}
> -
> -static int __init make_tempfile(const char *template, char **out_tempname,
> -				int do_unlink)
> -{
> -	char *tempname;
> -	int fd;
> -
> -	which_tmpdir();
> -	tempname = malloc(MAXPATHLEN);
> +	tempname = malloc(strlen(tempdir) + strlen(template) + 1);
>  	if (tempname == NULL)
>  		return -1;
>  
> -	find_tempdir();
> -	if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN))
> -		goto out;
> -
> -	if (template[0] != '/')
> -		strcpy(tempname, tempdir);
> -	else
> -		tempname[0] = '\0';
> -	strncat(tempname, template, MAXPATHLEN-1-strlen(tempname));
> +	strcpy(tempname, tempdir);
> +	strcat(tempname, template);
>  	fd = mkstemp(tempname);
>  	if (fd < 0) {
>  		fprintf(stderr, "open - cannot create %s: %s\n", tempname,
>  			strerror(errno));
>  		goto out;
>  	}
> -	if (do_unlink && (unlink(tempname) < 0)) {
> +	if (unlink(tempname) < 0) {
>  		perror("unlink");
>  		goto close;
>  	}
> -	if (out_tempname) {
> -		*out_tempname = tempname;
> -	} else
> -		free(tempname);
> +	free(tempname);
>  	return fd;
>  close:
>  	close(fd);
> @@ -351,14 +131,14 @@ out:
>  	return -1;
>  }
>  
> -#define TEMPNAME_TEMPLATE "vm_file-XXXXXX"
> +#define TEMPNAME_TEMPLATE "/vm_file-XXXXXX"
>  
>  static int __init create_tmp_file(unsigned long long len)
>  {
>  	int fd, err;
>  	char zero;
>  
> -	fd = make_tempfile(TEMPNAME_TEMPLATE, NULL, 1);
> +	fd = make_tempfile(TEMPNAME_TEMPLATE);
>  	if (fd < 0)
>  		exit(1);
>  
> @@ -402,7 +182,6 @@ int __init create_mem_file(unsigned long long len)
>  	return fd;
>  }
>  
> -
>  void __init check_tmpexec(void)
>  {
>  	void *addr;
> @@ -410,14 +189,13 @@ void __init check_tmpexec(void)
>  
>  	addr = mmap(NULL, UM_KERN_PAGE_SIZE,
>  		    PROT_READ | PROT_WRITE | PROT_EXEC, MAP_PRIVATE, fd, 0);
> -	printf("Checking PROT_EXEC mmap in %s...",tempdir);
> -	fflush(stdout);
> +	printf("Checking PROT_EXEC mmap in %s...", tempdir);
>  	if (addr == MAP_FAILED) {
>  		err = errno;
> -		perror("failed");
> +		printf("%s\n", strerror(err));
>  		close(fd);
>  		if (err == EPERM)
> -			printf("%s must be not mounted noexec\n",tempdir);
> +			printf("%s must be not mounted noexec\n", tempdir);
>  		exit(1);
>  	}
>  	printf("OK\n");
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists