[libvirt] [PATCH 0/5] Getting virt-sandbox-service to work
Hi all, this patch series fixes a few problems I encountered when getting virt-sandbox-service containers with images to work. The very first patch fixes a crasher in virt-aa-helper tests... but we were pretty lucky it didn't explode before: virErrors weren't initialized. Cédric Bosdonnat (5): virt-aa-helper wasn't running virErrorInitialize virt-aa-helper: /etc/libvirt-sandbox/services isn't restricted ip link needs 'name' in 3.16 to create the veth pair lxc: be more patient while resolving symlinks lxc: don't unmount subtree if it contains the source of the mount src/conf/domain_conf.h | 1 + src/lxc/lxc_container.c | 85 ++++++++++++++++++++++++++++++------------- src/security/virt-aa-helper.c | 14 ++++++- src/util/virnetdevveth.c | 4 +- 4 files changed, 74 insertions(+), 30 deletions(-) -- 2.1.2
This turns out to be working by magic but needs to be fixed. --- src/security/virt-aa-helper.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index e4d9a76..81f9f40 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1251,6 +1251,12 @@ main(int argc, char **argv) exit(EXIT_FAILURE); } + if (virThreadInitialize() < 0 || + virErrorInitialize() < 0) { + fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } + /* clear the environment */ environ = NULL; if (setenv("PATH", "/sbin:/usr/sbin", 1) != 0) -- 2.1.2
On Mon, Nov 24, 2014 at 09:54:42PM +0100, Cédric Bosdonnat wrote:
This turns out to be working by magic but needs to be fixed. --- src/security/virt-aa-helper.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index e4d9a76..81f9f40 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1251,6 +1251,12 @@ main(int argc, char **argv) exit(EXIT_FAILURE); }
+ if (virThreadInitialize() < 0 || + virErrorInitialize() < 0) { + fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } +
Calling virInitialize() would also set the logging output and filters from the environment. And if some parts of it are not needed, it can be controlled by compilation flags, but that's just a hint. As a general fix, this is enough. ACK.
/* clear the environment */ environ = NULL; if (setenv("PATH", "/sbin:/usr/sbin", 1) != 0) -- 2.1.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
To get virt-sandbox-service working with AppArmor, virt-aa-helper needs not to choke on path in /etc/libvirt-sandbox/services. --- src/security/virt-aa-helper.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 81f9f40..9c9e570 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -608,8 +608,12 @@ valid_path(const char *path, const bool readonly) npaths = sizeof(restricted)/sizeof(*(restricted)); if (array_starts_with(path, restricted, npaths) == 0 && - array_starts_with(path, override, opaths) != 0) - return 1; + array_starts_with(path, override, opaths) != 0) { + + /* We want to have virt-sandbox services config allowed */ + if (!STRPREFIX(path, "/etc/libvirt-sandbox/services/")) + return 1; + } npaths = sizeof(restricted_rw)/sizeof(*(restricted_rw)); if (!readonly) { -- 2.1.2
On Mon, Nov 24, 2014 at 09:54:43PM +0100, Cédric Bosdonnat wrote:
To get virt-sandbox-service working with AppArmor, virt-aa-helper needs not to choke on path in /etc/libvirt-sandbox/services. --- src/security/virt-aa-helper.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 81f9f40..9c9e570 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -608,8 +608,12 @@ valid_path(const char *path, const bool readonly)
npaths = sizeof(restricted)/sizeof(*(restricted)); if (array_starts_with(path, restricted, npaths) == 0 && - array_starts_with(path, override, opaths) != 0) - return 1; + array_starts_with(path, override, opaths) != 0) { + + /* We want to have virt-sandbox services config allowed */ + if (!STRPREFIX(path, "/etc/libvirt-sandbox/services/")) + return 1; + }
Isn't this what override[] is there for? It'd be way easier to read if it's just added there.
npaths = sizeof(restricted_rw)/sizeof(*(restricted_rw)); if (!readonly) { -- 2.1.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2014-11-25 at 08:05 +0100, Martin Kletzander wrote:
On Mon, Nov 24, 2014 at 09:54:43PM +0100, Cédric Bosdonnat wrote:
To get virt-sandbox-service working with AppArmor, virt-aa-helper needs not to choke on path in /etc/libvirt-sandbox/services. --- src/security/virt-aa-helper.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 81f9f40..9c9e570 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -608,8 +608,12 @@ valid_path(const char *path, const bool readonly)
npaths = sizeof(restricted)/sizeof(*(restricted)); if (array_starts_with(path, restricted, npaths) == 0 && - array_starts_with(path, override, opaths) != 0) - return 1; + array_starts_with(path, override, opaths) != 0) { + + /* We want to have virt-sandbox services config allowed */ + if (!STRPREFIX(path, "/etc/libvirt-sandbox/services/")) + return 1; + }
Isn't this what override[] is there for? It'd be way easier to read if it's just added there.
Oh indeed! I overlooked that one. I'll change that. -- Cedric
Due to a change (or bug?) in ip link implementation, the command 'ip link add vnet0...' is forced into 'ip link add name vnet0...' The changed command also works on older versions of iproute2, just the 'name' parameter has been made mandatory. --- src/util/virnetdevveth.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index e9d6f9c..ad30e1d 100644 --- a/src/util/virnetdevveth.c +++ b/src/util/virnetdevveth.c @@ -89,7 +89,7 @@ static int virNetDevVethGetFreeNum(int startDev) * @veth2: pointer to return name for container end of veth pair * * Creates a veth device pair using the ip command: - * ip link add veth1 type veth peer name veth2 + * ip link add name veth1 type veth peer name veth2 * If veth1 points to NULL on entry, it will be a valid interface on * return. veth2 should point to NULL on entry. * @@ -146,7 +146,7 @@ int virNetDevVethCreate(char** veth1, char** veth2) } cmd = virCommandNew("ip"); - virCommandAddArgList(cmd, "link", "add", + virCommandAddArgList(cmd, "link", "add", "name", *veth1 ? *veth1 : veth1auto, "type", "veth", "peer", "name", *veth2 ? *veth2 : veth2auto, -- 2.1.2
On Mon, Nov 24, 2014 at 09:54:44PM +0100, Cédric Bosdonnat wrote:
Due to a change (or bug?) in ip link implementation, the command 'ip link add vnet0...' is forced into 'ip link add name vnet0...' The changed command also works on older versions of iproute2, just the 'name' parameter has been made mandatory. --- src/util/virnetdevveth.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index e9d6f9c..ad30e1d 100644 --- a/src/util/virnetdevveth.c +++ b/src/util/virnetdevveth.c @@ -89,7 +89,7 @@ static int virNetDevVethGetFreeNum(int startDev) * @veth2: pointer to return name for container end of veth pair * * Creates a veth device pair using the ip command: - * ip link add veth1 type veth peer name veth2 + * ip link add name veth1 type veth peer name veth2 * If veth1 points to NULL on entry, it will be a valid interface on * return. veth2 should point to NULL on entry. * @@ -146,7 +146,7 @@ int virNetDevVethCreate(char** veth1, char** veth2) }
cmd = virCommandNew("ip"); - virCommandAddArgList(cmd, "link", "add", + virCommandAddArgList(cmd, "link", "add", "name", *veth1 ? *veth1 : veth1auto, "type", "veth", "peer", "name", *veth2 ? *veth2 : veth2auto, -- 2.1.2
I agree, the 'name' was always there, just optional. But what version of iproute2 do you have that requires it? I checked the current HEAD and it's still optional. This must be a bug in that particular implementation. ACK if you can argue with the version or platform this is required on. Martin
On Tue, 2014-11-25 at 08:42 +0100, Martin Kletzander wrote:
On Mon, Nov 24, 2014 at 09:54:44PM +0100, Cédric Bosdonnat wrote:
Due to a change (or bug?) in ip link implementation, the command 'ip link add vnet0...' is forced into 'ip link add name vnet0...' The changed command also works on older versions of iproute2, just the 'name' parameter has been made mandatory. --- src/util/virnetdevveth.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index e9d6f9c..ad30e1d 100644 --- a/src/util/virnetdevveth.c +++ b/src/util/virnetdevveth.c @@ -89,7 +89,7 @@ static int virNetDevVethGetFreeNum(int startDev) * @veth2: pointer to return name for container end of veth pair * * Creates a veth device pair using the ip command: - * ip link add veth1 type veth peer name veth2 + * ip link add name veth1 type veth peer name veth2 * If veth1 points to NULL on entry, it will be a valid interface on * return. veth2 should point to NULL on entry. * @@ -146,7 +146,7 @@ int virNetDevVethCreate(char** veth1, char** veth2) }
cmd = virCommandNew("ip"); - virCommandAddArgList(cmd, "link", "add", + virCommandAddArgList(cmd, "link", "add", "name", *veth1 ? *veth1 : veth1auto, "type", "veth", "peer", "name", *veth2 ? *veth2 : veth2auto, -- 2.1.2
I agree, the 'name' was always there, just optional. But what version of iproute2 do you have that requires it? I checked the current HEAD and it's still optional. This must be a bug in that particular implementation.
ACK if you can argue with the version or platform this is required on.
At least the 3.16 shipped on openSUSE 13.2 has that problem... though I think it's just a side effect of another change in iproute2. It worked fine with version 3.12. -- Cedric
On Tue, Nov 25, 2014 at 09:21:10AM +0100, Cedric Bosdonnat wrote:
On Tue, 2014-11-25 at 08:42 +0100, Martin Kletzander wrote:
On Mon, Nov 24, 2014 at 09:54:44PM +0100, Cédric Bosdonnat wrote:
Due to a change (or bug?) in ip link implementation, the command 'ip link add vnet0...' is forced into 'ip link add name vnet0...' The changed command also works on older versions of iproute2, just the 'name' parameter has been made mandatory. --- src/util/virnetdevveth.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index e9d6f9c..ad30e1d 100644 --- a/src/util/virnetdevveth.c +++ b/src/util/virnetdevveth.c @@ -89,7 +89,7 @@ static int virNetDevVethGetFreeNum(int startDev) * @veth2: pointer to return name for container end of veth pair * * Creates a veth device pair using the ip command: - * ip link add veth1 type veth peer name veth2 + * ip link add name veth1 type veth peer name veth2 * If veth1 points to NULL on entry, it will be a valid interface on * return. veth2 should point to NULL on entry. * @@ -146,7 +146,7 @@ int virNetDevVethCreate(char** veth1, char** veth2) }
cmd = virCommandNew("ip"); - virCommandAddArgList(cmd, "link", "add", + virCommandAddArgList(cmd, "link", "add", "name", *veth1 ? *veth1 : veth1auto, "type", "veth", "peer", "name", *veth2 ? *veth2 : veth2auto, -- 2.1.2
I agree, the 'name' was always there, just optional. But what version of iproute2 do you have that requires it? I checked the current HEAD and it's still optional. This must be a bug in that particular implementation.
ACK if you can argue with the version or platform this is required on.
At least the 3.16 shipped on openSUSE 13.2 has that problem... though I think it's just a side effect of another change in iproute2. It worked fine with version 3.12.
Must be OpenSUSE specific. Anyway, we need to work with that as well. ACK then ;) Martin
On Tue, Nov 25, 2014 at 9:21 AM, Cedric Bosdonnat <cbosdonnat@suse.com> wrote:
On Tue, 2014-11-25 at 08:42 +0100, Martin Kletzander wrote:
On Mon, Nov 24, 2014 at 09:54:44PM +0100, Cédric Bosdonnat wrote:
Due to a change (or bug?) in ip link implementation, the command 'ip link add vnet0...' is forced into 'ip link add name vnet0...' The changed command also works on older versions of iproute2, just the 'name' parameter has been made mandatory. --- src/util/virnetdevveth.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index e9d6f9c..ad30e1d 100644 --- a/src/util/virnetdevveth.c +++ b/src/util/virnetdevveth.c @@ -89,7 +89,7 @@ static int virNetDevVethGetFreeNum(int startDev) * @veth2: pointer to return name for container end of veth pair * * Creates a veth device pair using the ip command: - * ip link add veth1 type veth peer name veth2 + * ip link add name veth1 type veth peer name veth2 * If veth1 points to NULL on entry, it will be a valid interface on * return. veth2 should point to NULL on entry. * @@ -146,7 +146,7 @@ int virNetDevVethCreate(char** veth1, char** veth2) }
cmd = virCommandNew("ip"); - virCommandAddArgList(cmd, "link", "add", + virCommandAddArgList(cmd, "link", "add", "name", *veth1 ? *veth1 : veth1auto, "type", "veth", "peer", "name", *veth2 ? *veth2 : veth2auto, -- 2.1.2
I agree, the 'name' was always there, just optional. But what version of iproute2 do you have that requires it? I checked the current HEAD and it's still optional. This must be a bug in that particular implementation.
ACK if you can argue with the version or platform this is required on.
At least the 3.16 shipped on openSUSE 13.2 has that problem... though I think it's just a side effect of another change in iproute2. It worked fine with version 3.12.
Instead of papering over the issue in libvirt better ship a non-broken iproute2 in openSUSE 13.2. real fix: https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/... -- Thanks, //richard
On Tue, Nov 25, 2014 at 04:19:48PM +0100, Richard Weinberger wrote:
On Tue, Nov 25, 2014 at 9:21 AM, Cedric Bosdonnat <cbosdonnat@suse.com> wrote:
On Tue, 2014-11-25 at 08:42 +0100, Martin Kletzander wrote:
On Mon, Nov 24, 2014 at 09:54:44PM +0100, Cédric Bosdonnat wrote:
Due to a change (or bug?) in ip link implementation, the command 'ip link add vnet0...' is forced into 'ip link add name vnet0...' The changed command also works on older versions of iproute2, just the 'name' parameter has been made mandatory. --- src/util/virnetdevveth.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index e9d6f9c..ad30e1d 100644 --- a/src/util/virnetdevveth.c +++ b/src/util/virnetdevveth.c @@ -89,7 +89,7 @@ static int virNetDevVethGetFreeNum(int startDev) * @veth2: pointer to return name for container end of veth pair * * Creates a veth device pair using the ip command: - * ip link add veth1 type veth peer name veth2 + * ip link add name veth1 type veth peer name veth2 * If veth1 points to NULL on entry, it will be a valid interface on * return. veth2 should point to NULL on entry. * @@ -146,7 +146,7 @@ int virNetDevVethCreate(char** veth1, char** veth2) }
cmd = virCommandNew("ip"); - virCommandAddArgList(cmd, "link", "add", + virCommandAddArgList(cmd, "link", "add", "name", *veth1 ? *veth1 : veth1auto, "type", "veth", "peer", "name", *veth2 ? *veth2 : veth2auto, -- 2.1.2
I agree, the 'name' was always there, just optional. But what version of iproute2 do you have that requires it? I checked the current HEAD and it's still optional. This must be a bug in that particular implementation.
ACK if you can argue with the version or platform this is required on.
At least the 3.16 shipped on openSUSE 13.2 has that problem... though I think it's just a side effect of another change in iproute2. It worked fine with version 3.12.
Instead of papering over the issue in libvirt better ship a non-broken iproute2 in openSUSE 13.2. real fix: https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/...
Oh, thank you for finding that, I should've done my homework! Since it really is just a bug on iproute2 side in openSUSE, I'd rather keep it in its original state. And since the patch is already pushed, I'm inclining to reverting it. Other opinions? Martin
Am 26.11.2014 um 05:51 schrieb Martin Kletzander:
On Tue, Nov 25, 2014 at 04:19:48PM +0100, Richard Weinberger wrote:
On Tue, Nov 25, 2014 at 9:21 AM, Cedric Bosdonnat <cbosdonnat@suse.com> wrote:
On Tue, 2014-11-25 at 08:42 +0100, Martin Kletzander wrote:
On Mon, Nov 24, 2014 at 09:54:44PM +0100, Cédric Bosdonnat wrote:
Due to a change (or bug?) in ip link implementation, the command 'ip link add vnet0...' is forced into 'ip link add name vnet0...' The changed command also works on older versions of iproute2, just the 'name' parameter has been made mandatory. --- src/util/virnetdevveth.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index e9d6f9c..ad30e1d 100644 --- a/src/util/virnetdevveth.c +++ b/src/util/virnetdevveth.c @@ -89,7 +89,7 @@ static int virNetDevVethGetFreeNum(int startDev) * @veth2: pointer to return name for container end of veth pair * * Creates a veth device pair using the ip command: - * ip link add veth1 type veth peer name veth2 + * ip link add name veth1 type veth peer name veth2 * If veth1 points to NULL on entry, it will be a valid interface on * return. veth2 should point to NULL on entry. * @@ -146,7 +146,7 @@ int virNetDevVethCreate(char** veth1, char** veth2) }
cmd = virCommandNew("ip"); - virCommandAddArgList(cmd, "link", "add", + virCommandAddArgList(cmd, "link", "add", "name", *veth1 ? *veth1 : veth1auto, "type", "veth", "peer", "name", *veth2 ? *veth2 : veth2auto, -- 2.1.2
I agree, the 'name' was always there, just optional. But what version of iproute2 do you have that requires it? I checked the current HEAD and it's still optional. This must be a bug in that particular implementation.
ACK if you can argue with the version or platform this is required on.
At least the 3.16 shipped on openSUSE 13.2 has that problem... though I think it's just a side effect of another change in iproute2. It worked fine with version 3.12.
Instead of papering over the issue in libvirt better ship a non-broken iproute2 in openSUSE 13.2. real fix: https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/...
Oh, thank you for finding that, I should've done my homework! Since it really is just a bug on iproute2 side in openSUSE, I'd rather keep it in its original state. And since the patch is already pushed, I'm inclining to reverting it.
Yes, please revert the libvirt change. openSUSE is working on the issue: https://bugzilla.novell.com/show_bug.cgi?id=907093 Thanks, //richard
Hi Martin, On Wed, 2014-11-26 at 05:51 +0100, Martin Kletzander wrote:
Instead of papering over the issue in libvirt better ship a non-broken iproute2 in openSUSE 13.2. real fix: https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/...
Oh, thank you for finding that, I should've done my homework! Since it really is just a bug on iproute2 side in openSUSE, I'd rather keep it in its original state. And since the patch is already pushed, I'm inclining to reverting it.
Other opinions?
Quoting a colleague of mine working on the network stack: [this is] a regression in (upstream) iproute2 3.16, fixed in 3.17. bnc#907093 has been created for it. (Factory is also affected but a submit request is already on its way.) So I think we should keep that for those running the buggy 3.16. -- Cedric
Am 26.11.2014 um 09:25 schrieb Cedric Bosdonnat:
Hi Martin,
On Wed, 2014-11-26 at 05:51 +0100, Martin Kletzander wrote:
Instead of papering over the issue in libvirt better ship a non-broken iproute2 in openSUSE 13.2. real fix: https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/...
Oh, thank you for finding that, I should've done my homework! Since it really is just a bug on iproute2 side in openSUSE, I'd rather keep it in its original state. And since the patch is already pushed, I'm inclining to reverting it.
Other opinions?
Quoting a colleague of mine working on the network stack:
[this is] a regression in (upstream) iproute2 3.16, fixed in 3.17. bnc#907093 has been created for it. (Factory is also affected but a submit request is already on its way.)
So I think we should keep that for those running the buggy 3.16.
openSUSE has to fix their package and to serve a bugfix update, full stop. Thanks, //richard
On Wed, 2014-11-26 at 09:34 +0100, Richard Weinberger wrote:
Am 26.11.2014 um 09:25 schrieb Cedric Bosdonnat:
Hi Martin,
On Wed, 2014-11-26 at 05:51 +0100, Martin Kletzander wrote:
Instead of papering over the issue in libvirt better ship a non-broken iproute2 in openSUSE 13.2. real fix: https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/...
Oh, thank you for finding that, I should've done my homework! Since it really is just a bug on iproute2 side in openSUSE, I'd rather keep it in its original state. And since the patch is already pushed, I'm inclining to reverting it.
Other opinions?
Quoting a colleague of mine working on the network stack:
[this is] a regression in (upstream) iproute2 3.16, fixed in 3.17. bnc#907093 has been created for it. (Factory is also affected but a submit request is already on its way.)
So I think we should keep that for those running the buggy 3.16.
openSUSE has to fix their package and to serve a bugfix update, full stop.
Thought that may not happen only to openSUSE... and that fix didn't harm at all. -- Cedric
Am 26.11.2014 um 10:16 schrieb Cedric Bosdonnat:
On Wed, 2014-11-26 at 09:34 +0100, Richard Weinberger wrote:
Am 26.11.2014 um 09:25 schrieb Cedric Bosdonnat:
Hi Martin,
On Wed, 2014-11-26 at 05:51 +0100, Martin Kletzander wrote:
Instead of papering over the issue in libvirt better ship a non-broken iproute2 in openSUSE 13.2. real fix: https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/...
Oh, thank you for finding that, I should've done my homework! Since it really is just a bug on iproute2 side in openSUSE, I'd rather keep it in its original state. And since the patch is already pushed, I'm inclining to reverting it.
Other opinions?
Quoting a colleague of mine working on the network stack:
[this is] a regression in (upstream) iproute2 3.16, fixed in 3.17. bnc#907093 has been created for it. (Factory is also affected but a submit request is already on its way.)
So I think we should keep that for those running the buggy 3.16.
openSUSE has to fix their package and to serve a bugfix update, full stop.
Thought that may not happen only to openSUSE... and that fix didn't harm at all.
Yes, but "fixing" this issue in libvirt is not correct. It needs to be fixed in the right place and iproute2 this is. libvirt is not the only user of this iproute2 feature. Thanks, //richard
On 11/26/2014 02:25 AM, Richard Weinberger wrote:
So I think we should keep that for those running the buggy 3.16.
openSUSE has to fix their package and to serve a bugfix update, full stop.
Thought that may not happen only to openSUSE... and that fix didn't harm at all.
Yes, but "fixing" this issue in libvirt is not correct. It needs to be fixed in the right place and iproute2 this is.
libvirt is not the only user of this iproute2 feature.
I agree that making all clients work around a bug is annoying; on the other hand, once a client has worked around it, I see no reason to force the client to have to revert, since the workaround is arguably more legible. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Am 26.11.2014 um 14:23 schrieb Eric Blake:
On 11/26/2014 02:25 AM, Richard Weinberger wrote:
So I think we should keep that for those running the buggy 3.16.
openSUSE has to fix their package and to serve a bugfix update, full stop.
Thought that may not happen only to openSUSE... and that fix didn't harm at all.
Yes, but "fixing" this issue in libvirt is not correct. It needs to be fixed in the right place and iproute2 this is.
libvirt is not the only user of this iproute2 feature.
I agree that making all clients work around a bug is annoying; on the other hand, once a client has worked around it, I see no reason to force the client to have to revert, since the workaround is arguably more legible.
I agree that we don't have to force the revert. But we have to be very sure that *every* iproute2 version understands the new command line. I'm not an iproute2 expert, so I can't tell for sure. I doubt I'd revert it. Distros which ship the broken iproute2 package have to fix it anyways as a lot of other tools/scripts break too. Thanks, //richard
Resolving symlinks can fail before mounting any file system if one file system depends on another being mounted. Symlinks are now resolved in two passes: * Before any file system is mounted, but then we are more gentle if the source path can't be accessed * Right before mounting a file system, so that we are sure that we have the resolved path... but then if it can't be accessed we raise an error. --- src/conf/domain_conf.h | 1 + src/lxc/lxc_container.c | 77 ++++++++++++++++++++++++++++++++++--------------- 2 files changed, 54 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0a609df..f56b8b1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -821,6 +821,7 @@ struct _virDomainFSDef { virDomainDeviceInfo info; unsigned long long space_hard_limit; /* in bytes */ unsigned long long space_soft_limit; /* in bytes */ + bool symlinksResolved; }; diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index db823d6..12f3a41 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -608,6 +608,48 @@ static int lxcContainerUnmountSubtree(const char *prefix, return ret; } +static int lxcContainerResolveSymlinks(virDomainFSDefPtr fs, bool gentle) +{ + char *newroot; + + if (!fs->src || fs->symlinksResolved) + return 0; + + if (access(fs->src, F_OK)) { + if (gentle) { + /* Just ignore the error for the while, we'll try again later */ + VIR_DEBUG("Skipped unaccessible '%s'", fs->src); + return 0; + } else { + virReportSystemError(errno, + _("Failed to access '%s'"), fs->src); + return -1; + } + } + + VIR_DEBUG("Resolving '%s'", fs->src); + if (virFileResolveAllLinks(fs->src, &newroot) < 0) { + if (gentle) { + VIR_DEBUG("Skipped non-resolvable '%s'", fs->src); + return 0; + } else { + virReportSystemError(errno, + _("Failed to resolve symlink at %s"), + fs->src); + } + return -1; + } + + /* Mark it resolved to skip it the next time */ + fs->symlinksResolved = true; + + VIR_DEBUG("Resolved '%s' to %s", fs->src, newroot); + + VIR_FREE(fs->src); + fs->src = newroot; + + return 0; +} static int lxcContainerPrepareRoot(virDomainDefPtr def, virDomainFSDefPtr root, @@ -634,6 +676,9 @@ static int lxcContainerPrepareRoot(virDomainDefPtr def, return -1; } + if (lxcContainerResolveSymlinks(root, false) < 0) + return -1; + if (virAsprintf(&dst, "%s/%s.root", LXC_STATE_DIR, def->name) < 0) return -1; @@ -1552,6 +1597,9 @@ static int lxcContainerMountAllFS(virDomainDefPtr vmDef, if (STREQ(vmDef->fss[i]->dst, "/")) continue; + if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0) + return -1; + if (lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0) return -1; @@ -1735,37 +1783,18 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, return ret; } - -static int lxcContainerResolveSymlinks(virDomainDefPtr vmDef) +static int lxcContainerResolveAllSymlinks(virDomainDefPtr vmDef) { - char *newroot; size_t i; VIR_DEBUG("Resolving symlinks"); for (i = 0; i < vmDef->nfss; i++) { virDomainFSDefPtr fs = vmDef->fss[i]; - if (!fs->src) - continue; - - if (access(fs->src, F_OK)) { - virReportSystemError(errno, - _("Failed to access '%s'"), fs->src); + /* In the first pass, be gentle as some files may + depend on other filesystems to be mounted */ + if (lxcContainerResolveSymlinks(fs, true) < 0) return -1; - } - - VIR_DEBUG("Resolving '%s'", fs->src); - if (virFileResolveAllLinks(fs->src, &newroot) < 0) { - virReportSystemError(errno, - _("Failed to resolve symlink at %s"), - fs->src); - return -1; - } - - VIR_DEBUG("Resolved '%s' to %s", fs->src, newroot); - - VIR_FREE(fs->src); - fs->src = newroot; } VIR_DEBUG("Resolved all filesystem symlinks"); @@ -2106,7 +2135,7 @@ static int lxcContainerChild(void *data) goto cleanup; } - if (lxcContainerResolveSymlinks(vmDef) < 0) + if (lxcContainerResolveAllSymlinks(vmDef) < 0) goto cleanup; VIR_DEBUG("Setting up pivot"); -- 2.1.2
On Mon, Nov 24, 2014 at 09:54:45PM +0100, Cédric Bosdonnat wrote:
Resolving symlinks can fail before mounting any file system if one file system depends on another being mounted. Symlinks are now resolved in two passes:
* Before any file system is mounted, but then we are more gentle if the source path can't be accessed * Right before mounting a file system, so that we are sure that we have the resolved path... but then if it can't be accessed we raise an error. --- src/conf/domain_conf.h | 1 + src/lxc/lxc_container.c | 77 ++++++++++++++++++++++++++++++++++--------------- 2 files changed, 54 insertions(+), 24 deletions(-)
ACK, Martin
The typical case where we had a problem is with such a filesystem definition as created by virt-sandbox-service: <filesystem type='bind' accessmode='passthrough'> <source dir='/var/lib/libvirt/filesystems/mysshd/var'/> <target dir='/var'/> </filesystem> In this case, we don't want to unmount the /var subtree or we may loose the access to the source folder. --- src/lxc/lxc_container.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 12f3a41..334a1df 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1597,11 +1597,15 @@ static int lxcContainerMountAllFS(virDomainDefPtr vmDef, if (STREQ(vmDef->fss[i]->dst, "/")) continue; + VIR_DEBUG("Mounting '%s' -> '%s'", vmDef->fss[i]->src, vmDef->fss[i]->dst); + if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0) return -1; - if (lxcContainerUnmountSubtree(vmDef->fss[i]->dst, - false) < 0) + + if (!(vmDef->fss[i]->src && + STRPREFIX(vmDef->fss[i]->src, vmDef->fss[i]->dst)) && + lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0) return -1; if (lxcContainerMountFS(vmDef->fss[i], sec_mount_options) < 0) -- 2.1.2
On Mon, Nov 24, 2014 at 09:54:46PM +0100, Cédric Bosdonnat wrote:
The typical case where we had a problem is with such a filesystem definition as created by virt-sandbox-service:
<filesystem type='bind' accessmode='passthrough'> <source dir='/var/lib/libvirt/filesystems/mysshd/var'/> <target dir='/var'/> </filesystem>
In this case, we don't want to unmount the /var subtree or we may loose the access to the source folder.
I probably didn't quite get this. This is only true when host root is the root of the container, isn't it? And in that case it doesn't make much sense to do this.
--- src/lxc/lxc_container.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 12f3a41..334a1df 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1597,11 +1597,15 @@ static int lxcContainerMountAllFS(virDomainDefPtr vmDef, if (STREQ(vmDef->fss[i]->dst, "/")) continue;
+ VIR_DEBUG("Mounting '%s' -> '%s'", vmDef->fss[i]->src, vmDef->fss[i]->dst); + if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0) return -1;
- if (lxcContainerUnmountSubtree(vmDef->fss[i]->dst, - false) < 0) + + if (!(vmDef->fss[i]->src && + STRPREFIX(vmDef->fss[i]->src, vmDef->fss[i]->dst)) && + lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0) return -1;
if (lxcContainerMountFS(vmDef->fss[i], sec_mount_options) < 0) -- 2.1.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2014-11-25 at 08:48 +0100, Martin Kletzander wrote:
On Mon, Nov 24, 2014 at 09:54:46PM +0100, Cédric Bosdonnat wrote:
The typical case where we had a problem is with such a filesystem definition as created by virt-sandbox-service:
<filesystem type='bind' accessmode='passthrough'> <source dir='/var/lib/libvirt/filesystems/mysshd/var'/> <target dir='/var'/> </filesystem>
In this case, we don't want to unmount the /var subtree or we may loose the access to the source folder.
I probably didn't quite get this. This is only true when host root is the root of the container, isn't it? And in that case it doesn't make much sense to do this.
Indeed that happens when the host root is mounted as the container root... and that's what virt-sandbox-service does. We have this situation when the libvirt-sandbox service has a disk image: * The disk image is mounted to /var/lib/libvirt/filesystems/<name> * Quite a few items from /var/lib/libvirt/filesystems/<name> are bind mounted to their equivalent in the container root, and /var is one of them. -- Cedric
--- src/lxc/lxc_container.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 12f3a41..334a1df 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1597,11 +1597,15 @@ static int lxcContainerMountAllFS(virDomainDefPtr vmDef, if (STREQ(vmDef->fss[i]->dst, "/")) continue;
+ VIR_DEBUG("Mounting '%s' -> '%s'", vmDef->fss[i]->src, vmDef->fss[i]->dst); + if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0) return -1;
- if (lxcContainerUnmountSubtree(vmDef->fss[i]->dst, - false) < 0) + + if (!(vmDef->fss[i]->src && + STRPREFIX(vmDef->fss[i]->src, vmDef->fss[i]->dst)) && + lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0) return -1;
if (lxcContainerMountFS(vmDef->fss[i], sec_mount_options) < 0) -- 2.1.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Nov 25, 2014 at 09:27:28AM +0100, Cedric Bosdonnat wrote:
On Tue, 2014-11-25 at 08:48 +0100, Martin Kletzander wrote:
On Mon, Nov 24, 2014 at 09:54:46PM +0100, Cédric Bosdonnat wrote:
The typical case where we had a problem is with such a filesystem definition as created by virt-sandbox-service:
<filesystem type='bind' accessmode='passthrough'> <source dir='/var/lib/libvirt/filesystems/mysshd/var'/> <target dir='/var'/> </filesystem>
In this case, we don't want to unmount the /var subtree or we may loose the access to the source folder.
I probably didn't quite get this. This is only true when host root is the root of the container, isn't it? And in that case it doesn't make much sense to do this.
Indeed that happens when the host root is mounted as the container root... and that's what virt-sandbox-service does. We have this situation when the libvirt-sandbox service has a disk image: * The disk image is mounted to /var/lib/libvirt/filesystems/<name> * Quite a few items from /var/lib/libvirt/filesystems/<name> are bind mounted to their equivalent in the container root, and /var is one of them.
OK, and the mount below is done in the namespace of the container and since we drop CAP_SYS_ADMIN, we're safe from the guest unmounting the filesystem. We can't handle this in case of circular dependencies and I guess it doesn't make sense to even check for it. It will just fail anyway. So ACK, thanks for the explanation. Martin
Pushed them all with changes to patch #2 as requested by Martin. Thanks Martin for your review. -- Cedric On Mon, 2014-11-24 at 21:54 +0100, Cédric Bosdonnat wrote:
Hi all,
this patch series fixes a few problems I encountered when getting virt-sandbox-service containers with images to work. The very first patch fixes a crasher in virt-aa-helper tests... but we were pretty lucky it didn't explode before: virErrors weren't initialized.
Cédric Bosdonnat (5): virt-aa-helper wasn't running virErrorInitialize virt-aa-helper: /etc/libvirt-sandbox/services isn't restricted ip link needs 'name' in 3.16 to create the veth pair lxc: be more patient while resolving symlinks lxc: don't unmount subtree if it contains the source of the mount
src/conf/domain_conf.h | 1 + src/lxc/lxc_container.c | 85 ++++++++++++++++++++++++++++++------------- src/security/virt-aa-helper.c | 14 ++++++- src/util/virnetdevveth.c | 4 +- 4 files changed, 74 insertions(+), 30 deletions(-)
participants (6)
-
Cedric Bosdonnat -
Cédric Bosdonnat
-
Eric Blake -
Martin Kletzander -
Richard Weinberger -
Richard Weinberger