[lxc-devel] [PATCH 1/1] api locking: take process lock around open/close

Stéphane Graber stgraber at ubuntu.com
Fri May 31 15:39:17 UTC 2013


On 05/29/2013 11:21 PM, Serge Hallyn wrote:
> And update the comment explaining the locking.
> 
> Also take memlock in want_daemonize.
> 
> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>

Is that patch still relevant?

It doesn't appear to have been reviewed/applied to staging and touches
bits that have been changed with more recent patches, so just want to
confirm that I can ignore it.

> ---
>  src/lxc/lxccontainer.c | 153 +++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 115 insertions(+), 38 deletions(-)
> 
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index 9bc1caf..145ed7c 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -131,7 +131,12 @@ void remove_partial(struct lxc_container *c, int fd)
>  	char *path = alloca(len);
>  	int ret;
>  
> +	if (process_lock()) {
> +		ERROR("Failed getting process lock");
> +		return;
> +	}
>  	close(fd);
> +	process_unlock();
>  	ret = snprintf(path, len, "%s/%s/partial", c->config_path, c->name);
>  	if (ret < 0 || ret >= len) {
>  		ERROR("Error writing partial pathname");
> @@ -145,11 +150,11 @@ void remove_partial(struct lxc_container *c, int fd)
>  }
>  
>  /* LOCKING
> - * 1. c->privlock protects the struct lxc_container from multiple threads.
> - * 2. c->slock protects the on-disk container data
> + * 1. container_mem_lock(c) protects the struct lxc_container from multiple threads.
> + * 2. container_disk_lock(c) protects the on-disk container data - in particular the
> + *    container configuration file.
> + *    The container_disk_lock also takes the container_mem_lock.
>   * 3. thread_mutex protects process data (ex: fd table) from multiple threads
> - * slock is an flock, which does not exclude threads.  Therefore slock should
> - * always be wrapped inside privlock.
>   * NOTHING mutexes two independent programs with their own struct
>   * lxc_container for the same c->name, between API calls.  For instance,
>   * c->config_read(); c->start();  Between those calls, data on disk
> @@ -160,7 +165,7 @@ void remove_partial(struct lxc_container *c, int fd)
>   * due to hung callers.  So I prefer to keep the locks only within our own
>   * functions, not across functions.
>   *
> - * If you're going to fork while holding a lxccontainer, increment
> + * If you're going to clone while holding a lxccontainer, increment
>   * c->numthreads (under privlock) before forking.  When deleting,
>   * decrement numthreads under privlock, then if it hits 0 you can delete.
>   * Do not ever use a lxccontainer whose numthreads you did not bump.
> @@ -358,11 +363,14 @@ static pid_t lxcapi_init_pid(struct lxc_container *c)
>  
>  static bool load_config_locked(struct lxc_container *c, const char *fname)
>  {
> +	bool ret = false;
>  	if (!c->lxc_conf)
>  		c->lxc_conf = lxc_conf_init();
> +	process_lock();
>  	if (c->lxc_conf && !lxc_config_read(fname, c->lxc_conf))
> -		return true;
> -	return false;
> +		ret = true;
> +	process_unlock();
> +	return ret;
>  }
>  
>  static bool lxcapi_load_config(struct lxc_container *c, const char *alt_file)
> @@ -406,7 +414,12 @@ static void lxcapi_want_daemonize(struct lxc_container *c)
>  {
>  	if (!c)
>  		return;
> +	if (!container_mem_lock(c)) {
> +		ERROR("Error getting mem lock");
> +		return;
> +	}
>  	c->daemonize = 1;
> +	container_mem_unlock(c);
>  }
>  
>  static bool lxcapi_wait(struct lxc_container *c, const char *state, int timeout)
> @@ -512,6 +525,7 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
>  			process_unlock();
>  			return ret;
>  		}
> +		// In a forked task, no longer threaded
>  		process_unlock();
>  		/* second fork to be reparented by init */
>  		pid = fork();
> @@ -1023,7 +1037,9 @@ char** lxcapi_get_ips(struct lxc_container *c, char* interface, char* family, in
>  		goto out;
>  
>  	/* Save reference to old netns */
> +	process_lock();
>  	old_netns = open("/proc/self/ns/net", O_RDONLY);
> +	process_unlock();
>  	if (old_netns < 0) {
>  		SYSERROR("failed to open /proc/self/ns/net");
>  		goto out;
> @@ -1034,7 +1050,9 @@ char** lxcapi_get_ips(struct lxc_container *c, char* interface, char* family, in
>  	if (ret < 0 || ret >= MAXPATHLEN)
>  		goto out;
>  
> +	process_lock();
>  	new_netns = open(new_netns_path, O_RDONLY);
> +	process_unlock();
>  	if (new_netns < 0) {
>  		SYSERROR("failed to open %s", new_netns_path);
>  		goto out;
> @@ -1097,10 +1115,12 @@ out:
>  	/* Switch back to original netns */
>  	if (old_netns >= 0 && setns(old_netns, CLONE_NEWNET))
>  		SYSERROR("failed to setns");
> +	process_lock();
>  	if (new_netns >= 0)
>  		close(new_netns);
>  	if (old_netns >= 0)
>  		close(old_netns);
> +	process_unlock();
>  
>  	/* Append NULL to the array */
>  	if (count) {
> @@ -1194,11 +1214,15 @@ static bool lxcapi_save_config(struct lxc_container *c, const char *alt_file)
>  	if (lret)
>  		return false;
>  
> +	process_lock();
>  	fout = fopen(alt_file, "w");
> +	process_unlock();
>  	if (!fout)
>  		goto out;
>  	write_config(fout, c->lxc_conf);
> +	process_lock();
>  	fclose(fout);
> +	process_unlock();
>  	ret = true;
>  
>  out:
> @@ -1214,16 +1238,13 @@ static bool lxcapi_destroy(struct lxc_container *c)
>  {
>  	struct bdev *r = NULL;
>  	bool ret = false;
> +	int v;
>  
>  	if (!c || !lxcapi_is_defined(c))
>  		return false;
>  
> -	if (lxclock(c->privlock, 0))
> +	if (container_disk_lock(c))
>  		return false;
> -	if (lxclock(c->slock, 0)) {
> -		lxcunlock(c->privlock);
> -		return false;
> -	}
>  
>  	if (!is_stopped(c)) {
>  		// we should queue some sort of error - in c->error_string?
> @@ -1231,10 +1252,16 @@ static bool lxcapi_destroy(struct lxc_container *c)
>  		goto out;
>  	}
>  
> -	if (c->lxc_conf->rootfs.path && c->lxc_conf->rootfs.mount)
> +	if (c->lxc_conf->rootfs.path && c->lxc_conf->rootfs.mount) {
> +		process_lock();
>  		r = bdev_init(c->lxc_conf->rootfs.path, c->lxc_conf->rootfs.mount, NULL);
> +		process_unlock();
> +	}
>  	if (r) {
> -		if (r->ops->destroy(r) < 0) {
> +		process_lock();
> +		v = r->ops->destroy(r);
> +		process_unlock();
> +		if (v < 0) {
>  			ERROR("Error destroying rootfs for %s", c->name);
>  			goto out;
>  		}
> @@ -1243,15 +1270,17 @@ static bool lxcapi_destroy(struct lxc_container *c)
>  	const char *p1 = lxcapi_get_config_path(c);
>  	char *path = alloca(strlen(p1) + strlen(c->name) + 2);
>  	sprintf(path, "%s/%s", p1, c->name);
> -	if (lxc_rmdir_onedev(path) < 0) {
> +	process_lock();
> +	v = lxc_rmdir_onedev(path);
> +	process_unlock();
> +	if (v < 0) {
>  		ERROR("Error destroying container directory for %s", c->name);
>  		goto out;
>  	}
>  	ret = true;
>  
>  out:
> -	lxcunlock(c->privlock);
> -	lxcunlock(c->slock);
> +	container_disk_unlock(c);
>  	return ret;
>  }
>  
> @@ -1374,23 +1403,17 @@ err:
>  static bool lxcapi_set_cgroup_item(struct lxc_container *c, const char *subsys, const char *value)
>  {
>  	int ret;
> -	bool b = false;
>  
>  	if (!c)
>  		return false;
>  
> -	if (container_mem_lock(c))
> -		return false;
> -
>  	if (is_stopped(c))
> -		goto err;
> +		return false;
>  
> +	process_lock();
>  	ret = lxc_cgroup_set(c->name, subsys, value, c->config_path);
> -	if (!ret)
> -		b = true;
> -err:
> -	container_mem_unlock(c);
> -	return b;
> +	process_unlock();
> +	return ret == 0;
>  }
>  
>  static int lxcapi_get_cgroup_item(struct lxc_container *c, const char *subsys, char *retv, int inlen)
> @@ -1400,32 +1423,41 @@ static int lxcapi_get_cgroup_item(struct lxc_container *c, const char *subsys, c
>  	if (!c || !c->lxc_conf)
>  		return -1;
>  
> -	if (container_mem_lock(c))
> -		return -1;
> -
>  	if (is_stopped(c))
> -		goto out;
> +		return -1;
>  
> +	process_lock();
>  	ret = lxc_cgroup_get(c->name, subsys, retv, inlen, c->config_path);
> +	process_unlock();
>  
> -out:
> -	container_mem_unlock(c);
>  	return ret;
>  }
>  
>  const char *lxc_get_default_config_path(void)
>  {
> -	return default_lxc_path();
> +	const char *ret;
> +	process_lock();
> +	ret = default_lxc_path();
> +	process_unlock();
> +	return ret;
>  }
>  
>  const char *lxc_get_default_lvm_vg(void)
>  {
> -	return default_lvm_vg();
> +	const char *ret;
> +	process_lock();
> +	ret = default_lvm_vg();
> +	process_unlock();
> +	return ret;
>  }
>  
>  const char *lxc_get_default_zfs_root(void)
>  {
> -	return default_zfs_root();
> +	const char *ret;
> +	process_lock();
> +	ret = default_zfs_root();
> +	process_unlock();
> +	return ret;
>  }
>  
>  const char *lxc_get_version(void)
> @@ -1450,12 +1482,16 @@ static int copy_file(char *old, char *new)
>  		return -1;
>  	}
>  
> +	process_lock();
>  	in = open(old, O_RDONLY);
> +	process_unlock();
>  	if (in < 0) {
>  		SYSERROR("opening original file %s", old);
>  		return -1;
>  	}
> +	process_lock();
>  	out = open(new, O_CREAT | O_EXCL | O_WRONLY, 0644);
> +	process_unlock();
>  	if (out < 0) {
>  		SYSERROR("opening new file %s", new);
>  		close(in);
> @@ -1476,8 +1512,10 @@ static int copy_file(char *old, char *new)
>  			goto err;
>  		}
>  	}
> +	process_lock();
>  	close(in);
>  	close(out);
> +	process_unlock();
>  
>  	// we set mode, but not owner/group
>  	ret = chmod(new, sbuf.st_mode);
> @@ -1489,8 +1527,10 @@ static int copy_file(char *old, char *new)
>  	return 0;
>  
>  err:
> +	process_lock();
>  	close(in);
>  	close(out);
> +	process_unlock();
>  	return -1;
>  }
>  
> @@ -1547,48 +1587,67 @@ static int update_name_and_paths(const char *path, struct lxc_container *oldc,
>  	const char *p0, *p1, *p2, *end;
>  	const char *oldpath = oldc->get_config_path(oldc);
>  	const char *oldname = oldc->name;
> +	int ret;
>  
> +	process_lock();
>  	f = fopen(path, "r");
> +	process_unlock();
>  	if (!f) {
>  		SYSERROR("opening old config");
>  		return -1;
>  	}
>  	if (fseek(f, 0, SEEK_END) < 0) {
>  		SYSERROR("seeking to end of old config");
> +		process_lock();
>  		fclose(f);
> +		process_unlock();
>  		return -1;
>  	}
>  	flen = ftell(f);
>  	if (flen < 0) {
> +		process_lock();
>  		fclose(f);
> +		process_unlock();
>  		SYSERROR("telling size of old config");
>  		return -1;
>  	}
>  	if (fseek(f, 0, SEEK_SET) < 0) {
> +		process_lock();
>  		fclose(f);
> +		process_unlock();
>  		SYSERROR("rewinding old config");
>  		return -1;
>  	}
>  	contents = malloc(flen+1);
>  	if (!contents) {
>  		SYSERROR("out of memory");
> +		process_lock();
>  		fclose(f);
> +		process_unlock();
>  		return -1;
>  	}
>  	if (fread(contents, 1, flen, f) != flen) {
>  		free(contents);
> +		process_lock();
>  		fclose(f);
> +		process_unlock();
>  		SYSERROR("reading old config");
>  		return -1;
>  	}
>  	contents[flen] = '\0';
> -	if (fclose(f) < 0) {
> +
> +	process_lock();
> +	ret = fclose(f);
> +	process_unlock();
> +	if (ret < 0) {
>  		free(contents);
>  		SYSERROR("closing old config");
>  		return -1;
>  	}
>  
> +	process_lock();
>  	f = fopen(path, "w");
> +	process_unlock();
>  	if (!f) {
>  		SYSERROR("reopening config");
>  		free(contents);
> @@ -1605,11 +1664,15 @@ static int update_name_and_paths(const char *path, struct lxc_container *oldc,
>  			if (fwrite(p0, 1, (end-p0), f) != (end-p0)) {
>  				SYSERROR("writing new config");
>  				free(contents);
> +				process_lock();
>  				fclose(f);
> +				process_unlock();
>  				return -1;
>  			}
>  			free(contents);
> +			process_lock();
>  			fclose(f);
> +			process_unlock();
>  			// success
>  			return 0;
>  		} else {
> @@ -1618,7 +1681,9 @@ static int update_name_and_paths(const char *path, struct lxc_container *oldc,
>  			if (fwrite(p0, 1, (p-p0), f) != (p-p0)) {
>  				SYSERROR("writing new config");
>  				free(contents);
> +				process_lock();
>  				fclose(f);
> +				process_unlock();
>  				return -1;
>  			}
>  			p0 = p;
> @@ -1626,7 +1691,9 @@ static int update_name_and_paths(const char *path, struct lxc_container *oldc,
>  			if (fwrite(new, 1, strlen(new), f) != strlen(new)) {
>  				SYSERROR("writing new name or path in new config");
>  				free(contents);
> +				process_lock();
>  				fclose(f);
> +				process_unlock();
>  				return -1;
>  			}
>  			p0 += (p == p2) ? strlen(oldname) : strlen(oldpath);
> @@ -1671,13 +1738,19 @@ static int copyhooks(struct lxc_container *oldc, struct lxc_container *c)
>  
>  static void new_hwaddr(char *hwaddr)
>  {
> -	FILE *f = fopen("/dev/urandom", "r");
> +	FILE *f;
> +	
> +	process_lock();
> +	f = fopen("/dev/urandom", "r");
> +	process_unlock();
>  	if (f) {
>  		unsigned int seed;
>  		int ret = fread(&seed, sizeof(seed), 1, f);
>  		if (ret != 1)
>  			seed = time(NULL);
> +		process_lock();
>  		fclose(f);
> +		process_unlock();
>  		srand(seed);
>  	} else
>  		srand(time(NULL));
> @@ -1891,13 +1964,17 @@ struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *newname,
>  	}
>  
>  	// copy the configuration, tweak it as needed,
> +	process_lock();
>  	fout = fopen(newpath, "w");
> +	process_unlock();
>  	if (!fout) {
>  		SYSERROR("open %s", newpath);
>  		goto out;
>  	}
>  	write_config(fout, c->lxc_conf);
> +	process_lock();
>  	fclose(fout);
> +	process_unlock();
>  
>  	if (update_name_and_paths(newpath, c, n, l) < 0) {
>  		ERROR("Error updating name in cloned config");
> 


-- 
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20130531/fcafae80/attachment.pgp>


More information about the lxc-devel mailing list