[lxc-devel] Patch: pivot_root, new version

Daniel Lezcano daniel.lezcano at free.fr
Wed Jan 6 22:05:18 UTC 2010


Michael Holzt wrote:
> Hi Cedric!
>
> On Mi, 06 Jan 2010, Cedric Le Goater wrote:
>
>   
>> 2 minor comments on allocations.
>>     
>
> Thanks for pointing this out. I have done some minor work on the patch
> anyway, so a new version is attached.
>   

Thanks Michael,

here some questions / comments :

> diff -Naur lxc-0.6.4/src/lxc/conf.c lxc-0.6.4-pivot/src/lxc/conf.c
> --- lxc-0.6.4/src/lxc/conf.c	2009-11-24 09:47:26.000000000 +0100
> +++ lxc-0.6.4-pivot/src/lxc/conf.c	2010-01-06 19:35:48.000000000 +0100
> @@ -65,6 +65,8 @@
>  #define MS_REC 16384
>  #endif
>  
> +extern int pivot_root(const char * new_root, const char * put_old);
> +
>  typedef int (*instanciate_cb)(struct lxc_netdev *);
>  
>  struct mount_opt {
> @@ -334,7 +336,168 @@
>  	return 0;
>  }
>  
> -static int setup_rootfs(const char *rootfs)
> +static int setup_rootfs_pivot_root_cb(void *buffer, void *data)
> +{
> +	struct lxc_list	*mountlist, *listentry, *iterator;
> +	char *pivotdir, *mountpoint, *mountentry;
> +	int found;
> +	void **cbparm;
> +	
> +	mountentry = buffer;
> +	cbparm = (void **)data;
> +	
> +	mountlist = cbparm[0];
> +	pivotdir  = cbparm[1];
> +
> +	/* parse entry, first field is mountname, ignore */	
> +	mountpoint = strtok(mountentry, " ");
> +	if (!mountpoint)
> +		return -1;
> +	
> +	/* second field is mountpoint */
> +	mountpoint = strtok(NULL, " ");
> +	if (!mountpoint)
> +		return -1;
> +
> +	/* only consider mountpoints below old root fs */
> +	if (strncmp(mountpoint, pivotdir, strlen(pivotdir)))
> +		return 0;
> +		
> +	/* filter duplicate mountpoints */
> +	found = 0;
> +	lxc_list_for_each(iterator, mountlist) {
> +		if (!strcmp(iterator->elem, mountpoint)) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +	if (found)
> +		return 0;
> +		
> +	/* add entry to list */
> +	listentry = malloc(sizeof(*listentry));
> +	if (!listentry) {
> +		SYSERROR("malloc for mountpoint listentry failed");
> +		return -1;
> +	}
> +	
> +	listentry->elem = strdup(mountpoint);
> +	if (!listentry->elem) {
> +		SYSERROR("strdup failed");
> +		return -1
>   
";" is missing
> +	}
> +	lxc_list_add_tail(mountlist, listentry);
> +	
> +	return 0;
> +}
> +
> +
> +static int setup_rootfs_pivot_root(const char *rootfs, const char *pivotdir)
> +{
> +	char path[MAXPATHLEN], buffer[MAXPATHLEN];
> +	void *cbparm[2];
> +	struct lxc_list mountlist, *iterator;
> +	int ok, still_mounted, last_still_mounted;
> +
> +	/* pivot_root into our new root fs */
> +	snprintf(path, sizeof(path), ".%s", pivotdir);
> +
> +	if (chdir(rootfs)) {
> +		SYSERROR("can't chroot to new rootfs '%s'", rootfs);
> +		return -1;
> +	}
> +	
> +	if (pivot_root(".", path)) {
> +		SYSERROR("pivot_root syscall failed");
> +		return -1;
> +	}
> +	
> +	if (chdir("/")) {
> +		SYSERROR("can't chroot to / after pivot_root");
> +		return -1;
> +	}
> +	
> +	DEBUG("pivot_root syscall successful");
> +	
> +	/* read and parse /proc/mounts in old root fs */
> +	lxc_list_init(&mountlist);
> +
> +	snprintf(path, sizeof(path), "%s/", pivotdir);
> +	cbparm[0] = &mountlist;
> +	cbparm[1] = strdup(path);
> +
> +	if (!cbparm[1]) {
> +		SYSERROR("strdup failed");
> +		return -1;
> +	}
> +
> +	snprintf(path, sizeof(path), "/%s/proc/mounts", pivotdir);
> +	ok = lxc_file_for_each_line(path,
> +			       setup_rootfs_pivot_root_cb,
> +			       buffer, sizeof(buffer), &cbparm);
> +	if (ok < 0) {
> +		SYSERROR("failed to read or parse mount list '%s'", path);
> +		return -1;
> +	}
>   
Why the setup_rootfs_pivot_root_cb does not directly umount instead of 
creating a list and then browse the list to umount ?

> +
> +	/* umount filesystems until none left or list no longer shrinks */
> +	still_mounted = 0;
> +	do {
> +		last_still_mounted = still_mounted;
> +		still_mounted = 0;
> +		
> +		lxc_list_for_each(iterator, &mountlist) {
> +
> +			if (!umount(iterator->elem)) {
> +				DEBUG("umounted '%s'", (char *)iterator->elem);
> +				lxc_list_del(iterator);
> +				continue;
> +			}
> +			
> +			if (errno != EBUSY) {
> +				SYSERROR("failed to umount '%s'", (char *)iterator->elem);
> +				return -1;
> +			}
> +
> +			still_mounted++;
> +		}
> +	} while (still_mounted > 0 && still_mounted != last_still_mounted);
> +	
> +	if (still_mounted) {
> +		ERROR("could not umount %d mounts", still_mounted);
> +		return -1;
> +	}
> +
> +	/* umount old root fs */
> +	if (umount(pivotdir)) {
> +		SYSERROR("could not unmount old rootfs");
> +		return -1;
> +	}
> +	DEBUG("umounted '%s'", pivotdir);
> +	
> +
> +	INFO("pivoted to '%s'", rootfs);
> +	return 0;
> +}	
> +
> +static int setup_rootfs_chroot(const char *rootfs)
> +{
> +	if (chroot(rootfs)) {
> +		SYSERROR("failed to set chroot %s", rootfs);
> +		return -1;
> +	}
> +
> +	if (chdir(getenv("HOME")) && chdir("/")) {
> +		SYSERROR("failed to change to home directory");
> +		return -1;
> +	}
> +
> +	INFO("chrooted to '%s'", rootfs);
> +	return 0;
> +}
> +
> +
> +static int setup_rootfs(const char *rootfs, const char *pivotdir)
>  {
>  	char *tmpname;
>  	int ret = -1;
> @@ -358,18 +521,16 @@
>  		goto out;
>  	}
>  
> -	if (chroot(tmpname)) {
> -		SYSERROR("failed to set chroot %s", tmpname);
> -		goto out;
> +        if (pivotdir && setup_rootfs_pivot_root(tmpname, pivotdir)) {
> +       		ERROR("failed to pivot_root to '%s' (put_old: '%s')", rootfs, pivotdir);
> +       		goto out;
>  	}
>  
> -	if (chdir(getenv("HOME")) && chdir("/")) {
> -		SYSERROR("failed to change to home directory");
> +	if (!pivotdir && setup_rootfs_chroot(tmpname)) {
> +		ERROR("failed to chroot to '%s'", rootfs);
>  		goto out;
>  	}
> -
> -	INFO("chrooted to '%s'", rootfs);
> -
> +	
>  	ret = 0;
>  out:
>  	rmdir(tmpname);
> @@ -816,6 +977,7 @@
>  int lxc_conf_init(struct lxc_conf *conf)
>  {
>  	conf->rootfs = NULL;
> +	conf->pivotdir = NULL;
>   
I think you convinced with your demonstration the chroot is insane for a 
container, so IMO we can consider using always the pivot_root and 
definitively get rid of the chroot only. Is it possible to not add the 
pivotdir configuration variable and keep this internally by creating in 
the /tmp/lxc-rootfs the pivotdir to be used ?
That will simplify the code a lot and will avoid the user/admin to be 
puzzled with an extra configuration variable path, no ?

Thanks !




More information about the lxc-devel mailing list