[libvirt] [PATCH] LXC: fix order in virProcessGetNamespaces
virProcessGetNamespaces() opens files in /proc/XXX/ns/ which will later be passed to setns(). We have to make sure that the file descriptors in the array are in the correct order. Otherwise setns() may fail. The order has been taken from util-linux's sys-utils/nsenter.c Signed-off-by: Richard Weinberger <richard@nod.at> --- src/util/virprocess.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index bc028d7..fce0d46 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -513,11 +513,11 @@ int virProcessGetNamespaces(pid_t pid, int **fdlist) { int ret = -1; - DIR *dh = NULL; struct dirent *de; char *nsdir = NULL; char *nsfile = NULL; - size_t i; + char *ns_files[] = { "user", "ipc", "uts", "net", "pid", "mnt", NULL }; + size_t i = 0; *nfdlist = 0; *fdlist = NULL; @@ -528,45 +528,32 @@ int virProcessGetNamespaces(pid_t pid, goto cleanup; } - if (!(dh = opendir(nsdir))) { - virReportSystemError(errno, - _("Cannot read directory %s"), - nsdir); - goto cleanup; - } - - while ((de = readdir(dh))) { + while (ns_files[i]) { int fd; - if (de->d_name[0] == '.') - continue; - - if (VIR_EXPAND_N(*fdlist, *nfdlist, 1) < 0) { + if (virAsprintf(&nsfile, "%s/%s", nsdir, ns_files[i]) < 0) { virReportOOMError(); goto cleanup; } - if (virAsprintf(&nsfile, "%s/%s", nsdir, de->d_name) < 0) { - virReportOOMError(); - goto cleanup; + if ((fd = open(nsfile, O_RDWR)) < 0) { + goto next; } - if ((fd = open(nsfile, O_RDWR)) < 0) { - virReportSystemError(errno, - _("Unable to open %s"), - nsfile); + if (VIR_EXPAND_N(*fdlist, *nfdlist, 1) < 0) { + virReportOOMError(); goto cleanup; } (*fdlist)[(*nfdlist)-1] = fd; +next: VIR_FREE(nsfile); + i++; } ret = 0; cleanup: - if (dh) - closedir(dh); VIR_FREE(nsdir); VIR_FREE(nsfile); if (ret < 0) { -- 1.8.3
On Wed, Jun 05, 2013 at 11:23:07PM +0200, Richard Weinberger wrote:
virProcessGetNamespaces() opens files in /proc/XXX/ns/ which will later be passed to setns(). We have to make sure that the file descriptors in the array are in the correct order. Otherwise setns() may fail.
What is the scenario / cause of the failure ?
The order has been taken from util-linux's sys-utils/nsenter.c
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Am 06.06.2013 09:53, schrieb Daniel P. Berrange:
On Wed, Jun 05, 2013 at 11:23:07PM +0200, Richard Weinberger wrote:
virProcessGetNamespaces() opens files in /proc/XXX/ns/ which will later be passed to setns(). We have to make sure that the file descriptors in the array are in the correct order. Otherwise setns() may fail.
What is the scenario / cause of the failure ?
You cannot attach to namespaces in random order. For example with user namespaces an unprivileged can enter other namespaces. But to do so you have to enter the user namespace first and then the other ones. Same for mnt and pid, if you enter the mnt namespace before pid your procfs will go nuts. Thanks, //richard
On Thu, Jun 06, 2013 at 09:58:28AM +0200, Richard Weinberger wrote:
Am 06.06.2013 09:53, schrieb Daniel P. Berrange:
On Wed, Jun 05, 2013 at 11:23:07PM +0200, Richard Weinberger wrote:
virProcessGetNamespaces() opens files in /proc/XXX/ns/ which will later be passed to setns(). We have to make sure that the file descriptors in the array are in the correct order. Otherwise setns() may fail.
What is the scenario / cause of the failure ?
You cannot attach to namespaces in random order. For example with user namespaces an unprivileged can enter other namespaces. But to do so you have to enter the user namespace first and then the other ones.
Ok, that kind of makes sense, ACK to the patch. I'll update the commit message with this information.
Same for mnt and pid, if you enter the mnt namespace before pid your procfs will go nuts.
That shouldn't affect us since we don't need to access procfs at all during the loop where we call setns(). Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Am 06.06.2013 10:08, schrieb Daniel P. Berrange:
On Thu, Jun 06, 2013 at 09:58:28AM +0200, Richard Weinberger wrote:
Am 06.06.2013 09:53, schrieb Daniel P. Berrange:
On Wed, Jun 05, 2013 at 11:23:07PM +0200, Richard Weinberger wrote:
virProcessGetNamespaces() opens files in /proc/XXX/ns/ which will later be passed to setns(). We have to make sure that the file descriptors in the array are in the correct order. Otherwise setns() may fail.
What is the scenario / cause of the failure ?
You cannot attach to namespaces in random order. For example with user namespaces an unprivileged can enter other namespaces. But to do so you have to enter the user namespace first and then the other ones.
Ok, that kind of makes sense, ACK to the patch. I'll update the commit message with this information.
Thanks. :) FYI: util-linux's nsenter.c says: /* Careful the order is significant in this array. * * The user namespace comes first, so that it is entered * first. This gives an unprivileged user the potential to * enter the other namespaces. */ { .nstype = CLONE_NEWUSER, .name = "ns/user", .fd = -1 }, { .nstype = CLONE_NEWIPC, .name = "ns/ipc", .fd = -1 }, { .nstype = CLONE_NEWUTS, .name = "ns/uts", .fd = -1 }, { .nstype = CLONE_NEWNET, .name = "ns/net", .fd = -1 }, { .nstype = CLONE_NEWPID, .name = "ns/pid", .fd = -1 }, { .nstype = CLONE_NEWNS, .name = "ns/mnt", .fd = -1 }, { .nstype = 0, .name = NULL, .fd = -1 }
Same for mnt and pid, if you enter the mnt namespace before pid your procfs will go nuts.
That shouldn't affect us since we don't need to access procfs at all during the loop where we call setns().
Yeah, but there are some setns() combination which may still fail. Some weeks ago I wrote my own ns attach tool that worked also with user namespaces. After debugging a few very strange issues there setns() failed all of the sudden I realized that the setns() order matters. Thanks, //richard
participants (2)
-
Daniel P. Berrange -
Richard Weinberger