[PATCH 0/4] virt-aa-helper: Add new option to remove corrupted
This patch-series aims to address the bug reported in [1] and [2]. Bug description : Some times libvirt fails to start a vm with the following error : libvirt: error : unable to set AppArmor profile 'libvirt-b05b297f-952f-42d6-b04e-f9a13767db54' for '/usr/bin/kvm-spice': No such file or directory This happens because file /etc/apparmor.d/libvirt/libvirt-<vm-uuid> has 0 size. During the vm start-up virt-aa-helper tries to load the profile and because it is 0 it fails. When file /etc/apparmor.d/libvirt/libvirt-<vm-uuid> is removed the vm can start without problems. To address this issue this patch-series suggests the following. On the vm start-up check if the profile has 0 size and if this is the case remove it and create it again. To do so a new option (-P) is introduced and also create and remove profile fuctionalities are placed into separate functions. The first commit moves create and remove functionlites into functinos for later reuse from different places. The second commit adds a new option (-P) to remove the profile file. The thid commit implements the actual fix (check if the profile has 0 size and if so remove it and create it again). The fourth patch adds a test for the above fix. [1] https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1927519 [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084 Ioanna Alifieraki (4): virt-aa-helper: Move create and remove profile into separate functions virt-aa-helper: Add new purge (-P) option virt-aa-helper: Purge profile if corrupted virt-aa-helper: test: add test for new option -P src/security/virt-aa-helper.c | 87 ++++++++++++++++++++++++++--------- tests/meson.build | 1 + tests/virt-aa-helper-test | 29 ++++++++++++ 3 files changed, 96 insertions(+), 21 deletions(-) -- 2.17.1
Reorganise create and remove functionality into functions for later reuse in calls from multiple places. Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> --- src/security/virt-aa-helper.c | 55 ++++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 77b2307594..269c372704 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -262,7 +262,7 @@ update_include_file(const char *include_file, const char *included_files, * Create a profile based on a template */ static int -create_profile(const char *profile, const char *profile_name, +_create_profile(const char *profile, const char *profile_name, const char *profile_files, int virtType) { g_autofree char *template = NULL; @@ -1437,6 +1437,36 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) return 0; } +static int create_profile(vahControl *ctl, char *profile, char *include_file) +{ + + int rc = 0; + char *tmp = g_strdup_printf(" #include <libvirt/%s.files>\n", ctl->uuid); + if (ctl->dryrun) { + vah_info(profile); + vah_info(ctl->uuid); + vah_info(tmp); + } else if ((rc = _create_profile(profile, ctl->uuid, tmp, + ctl->def->virtType)) != 0) { + unlink(include_file); + rc = -1; + } + VIR_FREE(tmp); + + return rc; +} + +static int remove_profile(vahControl *ctl, char *include_file) +{ + int rc = 0; + + if ((rc = parserRemove(ctl->uuid)) != 0) + return rc; + if (ctl->cmd == 'D') + unlink(include_file); + + return rc; +} /* * virt-aa-helper -c -u UUID < file.xml @@ -1489,9 +1519,7 @@ main(int argc, char **argv) if (ctl->cmd == 'a') { rc = parserLoad(ctl->uuid); } else if (ctl->cmd == 'R' || ctl->cmd == 'D') { - rc = parserRemove(ctl->uuid); - if (ctl->cmd == 'D') - unlink(include_file); + rc = remove_profile(ctl, include_file); } else if (ctl->cmd == 'c' || ctl->cmd == 'r') { char *included_files = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; @@ -1538,22 +1566,9 @@ main(int argc, char **argv) /* create the profile from TEMPLATE */ - if (ctl->cmd == 'c') { - char *tmp = NULL; - tmp = g_strdup_printf(" #include <libvirt/%s.files>\n", ctl->uuid); - - if (ctl->dryrun) { - vah_info(profile); - vah_info(ctl->uuid); - vah_info(tmp); - rc = 0; - } else if ((rc = create_profile(profile, ctl->uuid, tmp, - ctl->def->virtType)) != 0) { - vah_error(ctl, 0, _("could not create profile")); - unlink(include_file); - } - VIR_FREE(tmp); - } + if (ctl->cmd == 'c') + if ((rc = create_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 0, _("could not create profile")); if (rc == 0 && !ctl->dryrun) { if (ctl->cmd == 'c') -- 2.17.1
Currently there is no way to remove the profile file. This commit provides this functionality (required for next commit). Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> --- src/security/virt-aa-helper.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 269c372704..5ec0fb8807 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -101,6 +101,7 @@ vah_usage(void) " -a | --add load profile\n" " -c | --create create profile from template\n" " -D | --delete unload profile and delete generated rules\n" + " -P | --purge purge profile\n" " -r | --replace reload profile\n" " -R | --remove unload profile\n" " Options:\n" @@ -1361,13 +1362,14 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) {"add-file", 0, 0, 'f'}, {"append-file", 0, 0, 'F'}, {"help", 0, 0, 'h'}, + {"purge", 0, 0, 'P'}, {"replace", 0, 0, 'r'}, {"remove", 0, 0, 'R'}, {"uuid", 1, 0, 'u'}, {0, 0, 0, 0} }; - while ((arg = getopt_long(argc, argv, "acdDhrRH:b:u:p:f:F:", opt, + while ((arg = getopt_long(argc, argv, "acdDhPrRH:b:u:p:f:F:", opt, &idx)) != -1) { switch (arg) { case 'a': @@ -1391,6 +1393,9 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) vah_usage(); exit(EXIT_SUCCESS); break; + case 'P': + ctl->cmd = 'P'; + break; case 'r': ctl->cmd = 'r'; break; @@ -1456,7 +1461,7 @@ static int create_profile(vahControl *ctl, char *profile, char *include_file) return rc; } -static int remove_profile(vahControl *ctl, char *include_file) +static int remove_profile(vahControl *ctl, char *profile, char *include_file) { int rc = 0; @@ -1464,6 +1469,8 @@ static int remove_profile(vahControl *ctl, char *include_file) return rc; if (ctl->cmd == 'D') unlink(include_file); + if (ctl->cmd == 'P') + unlink(profile); return rc; } @@ -1519,7 +1526,7 @@ main(int argc, char **argv) if (ctl->cmd == 'a') { rc = parserLoad(ctl->uuid); } else if (ctl->cmd == 'R' || ctl->cmd == 'D') { - rc = remove_profile(ctl, include_file); + rc = remove_profile(ctl, profile, include_file); } else if (ctl->cmd == 'c' || ctl->cmd == 'r') { char *included_files = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; -- 2.17.1
This commit aims to address the bug reported in [1] and [2]. If the profile is corrupted (0-size) the VM cannot be launched. To overcome this check if the profile exists and if it has 0 size remove it and create it again. [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084 [2] https://bugs.launchpad.net/bugs/1927519 Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> --- src/security/virt-aa-helper.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5ec0fb8807..5e13b29053 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1489,6 +1489,7 @@ main(int argc, char **argv) int rc = -1; char *profile = NULL; char *include_file = NULL; + off_t size; if (virGettextInitialize() < 0 || virErrorInitialize() < 0) { @@ -1534,6 +1535,28 @@ main(int argc, char **argv) if (ctl->cmd == 'c' && virFileExists(profile)) vah_error(ctl, 1, _("profile exists")); + /* + * Rare cases can leave corrupted empty files behind breaking + * the guest. An empty file is never correct as virt-aa-helper + * would at least add the basic rules, therefore clean this up + * for a proper refresh. + */ + + if (virFileExists(profile)) { + size = virFileLength(profile, -1); + if (size == 0) { + char temp; + vah_warning(_("Profile of 0 size detected, will attempt to remove and re-create it")); + temp = ctl->cmd; + ctl->cmd = 'P'; + if ((rc = remove_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 1, _("could not remove corrupted profile")); + ctl->cmd = temp; + if ((rc = create_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 1, _("could not re-create profile")); + } + } + if (ctl->append && ctl->newfile) { if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) goto cleanup; -- 2.17.1
On a Thursday in 2021, Ioanna Alifieraki wrote:
This commit aims to address the bug reported in [1] and [2]. If the profile is corrupted (0-size) the VM cannot be launched. To overcome this check if the profile exists and if it has 0 size remove it and create it again.
[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084 [2] https://bugs.launchpad.net/bugs/1927519
Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> --- src/security/virt-aa-helper.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5ec0fb8807..5e13b29053 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1489,6 +1489,7 @@ main(int argc, char **argv) int rc = -1; char *profile = NULL; char *include_file = NULL; + off_t size;
if (virGettextInitialize() < 0 || virErrorInitialize() < 0) { @@ -1534,6 +1535,28 @@ main(int argc, char **argv) if (ctl->cmd == 'c' && virFileExists(profile)) vah_error(ctl, 1, _("profile exists"));
+ /* + * Rare cases can leave corrupted empty files behind breaking + * the guest. An empty file is never correct as virt-aa-helper + * would at least add the basic rules, therefore clean this up + * for a proper refresh. + */ + + if (virFileExists(profile)) { + size = virFileLength(profile, -1); + if (size == 0) { + char temp; + vah_warning(_("Profile of 0 size detected, will attempt to remove and re-create it")); + temp = ctl->cmd;
Please do not pass 'ctl' to the helper functions. It makes it more diffuclt to see what's going on.
+ ctl->cmd = 'P'; + if ((rc = remove_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 1, _("could not remove corrupted profile"));
Easier to read as: if (parserRemove(ctl->uuid) < 0) goto cleanup; unlink(profile);
+ ctl->cmd = temp; + if ((rc = create_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 1, _("could not re-create profile")); + }
Since we did not honour ctl->dryrun in the previous step, it should be safe to call create_profile directly, without the helper introduced in 1/4. Jano
+ } + if (ctl->append && ctl->newfile) { if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) goto cleanup; -- 2.17.1
On Thu, Oct 7, 2021 at 10:41 PM Ján Tomko <jtomko@redhat.com> wrote:
On a Thursday in 2021, Ioanna Alifieraki wrote:
This commit aims to address the bug reported in [1] and [2]. If the profile is corrupted (0-size) the VM cannot be launched. To overcome this check if the profile exists and if it has 0 size remove it and create it again.
[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084 [2] https://bugs.launchpad.net/bugs/1927519
Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> --- src/security/virt-aa-helper.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5ec0fb8807..5e13b29053 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1489,6 +1489,7 @@ main(int argc, char **argv) int rc = -1; char *profile = NULL; char *include_file = NULL; + off_t size;
if (virGettextInitialize() < 0 || virErrorInitialize() < 0) { @@ -1534,6 +1535,28 @@ main(int argc, char **argv) if (ctl->cmd == 'c' && virFileExists(profile)) vah_error(ctl, 1, _("profile exists"));
+ /* + * Rare cases can leave corrupted empty files behind breaking + * the guest. An empty file is never correct as virt-aa-helper + * would at least add the basic rules, therefore clean this up + * for a proper refresh. + */ + + if (virFileExists(profile)) { + size = virFileLength(profile, -1); + if (size == 0) { + char temp; + vah_warning(_("Profile of 0 size detected, will attempt to remove and re-create it")); + temp = ctl->cmd;
Thank you very much for the feedback provided. I'd like some clarifications.
Please do not pass 'ctl' to the helper functions. It makes it more diffuclt to see what's going on.
Do you suggest this for both remove_profile and create_profile helper functions ? Not passing 'ctl' to the helper functions would make it difficult to tell between the different options. For example, not passing ctl to remove_profile we cannot tell if it's called for D,R or P option. I guess we could overcome this by passing an extra 'cmd' argument and not the 'ctl'. Regarding create_profile 'ctl' is used to check the 'ctl->dryrun' case. Again, an alternative could be an extra argument for the dryrun. What do you think?
+ ctl->cmd = 'P'; + if ((rc = remove_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 1, _("could not remove corrupted profile"));
Easier to read as: if (parserRemove(ctl->uuid) < 0) goto cleanup; unlink(profile);
In that case, I question whether patch 2/4 is needed after all. Adding the '-P' option targets (at least for the time being) that specific case.
+ ctl->cmd = temp; + if ((rc = create_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 1, _("could not re-create profile")); + }
Since we did not honour ctl->dryrun in the previous step, it should be safe to call create_profile directly, without the helper introduced in 1/4.
I'm a bit concerned here, as all tests in virt-aa-helper-test are run with the dryrun option set. Thanks, Jo
Jano
+ } + if (ctl->append && ctl->newfile) { if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) goto cleanup; -- 2.17.1
On a Wednesday in 2021, Ioanna Alifieraki wrote:
On Thu, Oct 7, 2021 at 10:41 PM Ján Tomko <jtomko@redhat.com> wrote:
On a Thursday in 2021, Ioanna Alifieraki wrote:
This commit aims to address the bug reported in [1] and [2]. If the profile is corrupted (0-size) the VM cannot be launched. To overcome this check if the profile exists and if it has 0 size remove it and create it again.
[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084 [2] https://bugs.launchpad.net/bugs/1927519
Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> --- src/security/virt-aa-helper.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5ec0fb8807..5e13b29053 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1489,6 +1489,7 @@ main(int argc, char **argv) int rc = -1; char *profile = NULL; char *include_file = NULL; + off_t size;
if (virGettextInitialize() < 0 || virErrorInitialize() < 0) { @@ -1534,6 +1535,28 @@ main(int argc, char **argv) if (ctl->cmd == 'c' && virFileExists(profile)) vah_error(ctl, 1, _("profile exists"));
+ /* + * Rare cases can leave corrupted empty files behind breaking + * the guest. An empty file is never correct as virt-aa-helper + * would at least add the basic rules, therefore clean this up + * for a proper refresh. + */ + + if (virFileExists(profile)) { + size = virFileLength(profile, -1); + if (size == 0) { + char temp; + vah_warning(_("Profile of 0 size detected, will attempt to remove and re-create it")); + temp = ctl->cmd;
Thank you very much for the feedback provided. I'd like some clarifications.
Please do not pass 'ctl' to the helper functions. It makes it more diffuclt to see what's going on.
Do you suggest this for both remove_profile and create_profile helper functions ? Not passing 'ctl' to the helper functions would make it difficult to tell between the different options. For example, not passing ctl to remove_profile we cannot tell if it's called for D,R or P option. I guess we could overcome this by passing an extra 'cmd' argument and not the 'ctl'.
I think the remove_profile function is not necessary - you can just call parserRemove directly in main() and unlink the file there. But passing the uuid and cmd as arguments instead of ctl would at least remove the need to replace ctl->cmd temporarily.
Regarding create_profile 'ctl' is used to check the 'ctl->dryrun' case. Again, an alternative could be an extra argument for the dryrun.
Passing dryrun in ctl is ok.
What do you think?
+ ctl->cmd = 'P'; + if ((rc = remove_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 1, _("could not remove corrupted profile"));
Easier to read as: if (parserRemove(ctl->uuid) < 0) goto cleanup; unlink(profile);
In that case, I question whether patch 2/4 is needed after all. Adding the '-P' option targets (at least for the time being) that specific case.
That depends on whether this should be available to the users of virt-aa-helper, or it's just for internal use when creating the profile.
+ ctl->cmd = temp; + if ((rc = create_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 1, _("could not re-create profile")); + }
Since we did not honour ctl->dryrun in the previous step, it should be safe to call create_profile directly, without the helper introduced in 1/4.
I'm a bit concerned here, as all tests in virt-aa-helper-test are run with the dryrun option set.
Above, I suggested that creating the profile here does not need to check for ctl->dryrun, because the remove_profile('P') call right above does not check for it. That sounds wrong to me now - nothing should be purged if the dryrun option was specified. Also, is there a need to call create_profile here? For 'c' it would get called twice. Adding a 'bool purged' variable and only creating the profile if (ctl->cmd == 'c' || purged) would get rid of the extra invocation. Jano
Thanks, Jo
Jano
+ } + if (ctl->append && ctl->newfile) { if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) goto cleanup; -- 2.17.1
Create a corrupt profile and expect to be removed after the test is run. Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> --- tests/meson.build | 1 + tests/virt-aa-helper-test | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/tests/meson.build b/tests/meson.build index dfbc2c01e2..991cfc402d 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -40,6 +40,7 @@ tests_env = [ 'LC_ALL=C', 'LIBVIRT_AUTOSTART=0', 'G_DEBUG=fatal-warnings', + 'sysconfdir=@0@'.format(get_option('prefix') / get_option('sysconfdir')), ] if use_expensive_tests diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 83f53acef6..135c4968b5 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -16,6 +16,7 @@ fi output="/dev/null" use_valgrind="" ld_library_path="$abs_top_builddir/src/" +profile_path="$sysconfdir/apparmor.d/libvirt/" if [ ! -z "$1" ] && [ "$1" = "-d" ]; then output="/dev/stdout" shift @@ -399,6 +400,34 @@ testme "0" "shmem doorbell" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='shmem_server'><model type='ivshmem-doorbell'/><server path='/var/lib/libvirt/ivshmem_socket'/></shmem></devices>,g" "$template_xml" > "$test_xml" testme "0" "shmem doorbell serverpath" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ivshmem_socket\"\s*rw,$" +# For the next test to run apparmor needs to be installed and enabled. +# In some environments (e.g. containers) even though apparmor is +# installed, it is not enabled because securityfs is not mounted. +# In those environments this test cannot run so skip it. +# This test also needs to be run as root. +if [ `eval id -u` = 0 ] && [ -x "$(command -v aa-enabled)" ] && [ `eval aa-enabled` = "Yes" ]; then + sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk2,g" "$template_xml" > "$test_xml" + # Running the tests does not require libvirt to be installed. As a + # result the appropriate directories have not been created. Create them + # now to run the test. + mkdir -p "$profile_path" + # create a corrupted profile + touch "$profile_path/$valid_uuid" + testme "0" "purge" "-r -u $valid_uuid" "$test_xml" + # All the tests are run with the --dry-run option this test is + # never going to fail because the profile is not going to be loaded. + # However, since we touch the profile if it's still here after the test + # it means that something went wrong, so make the test fail. + if [ -f "$profile_path/$valid_uuid" ]; then + echo "FAIL: failed to purge corrupted profile" >$output + echo " '$extra_args $args': " + errors=$(($errors + 1)) + # remove corrupted profile anyways not to interfere with + # subsequent runs of the tests. + rm "$profile_path/$valid_uuid" + fi +fi + testme "0" "help" "-h" echo "" >$output -- 2.17.1
On a Thursday in 2021, Ioanna Alifieraki wrote:
Create a corrupt profile and expect to be removed after the test is run.
Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> --- tests/meson.build | 1 + tests/virt-aa-helper-test | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Thu, Oct 7, 2021 at 7:25 PM Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> wrote:
Create a corrupt profile and expect to be removed after the test is run.
Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> --- tests/meson.build | 1 + tests/virt-aa-helper-test | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+)
diff --git a/tests/meson.build b/tests/meson.build index dfbc2c01e2..991cfc402d 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -40,6 +40,7 @@ tests_env = [ 'LC_ALL=C', 'LIBVIRT_AUTOSTART=0', 'G_DEBUG=fatal-warnings', + 'sysconfdir=@0@'.format(get_option('prefix') / get_option('sysconfdir')), ]
if use_expensive_tests diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 83f53acef6..135c4968b5 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -16,6 +16,7 @@ fi output="/dev/null" use_valgrind="" ld_library_path="$abs_top_builddir/src/" +profile_path="$sysconfdir/apparmor.d/libvirt/" if [ ! -z "$1" ] && [ "$1" = "-d" ]; then output="/dev/stdout" shift @@ -399,6 +400,34 @@ testme "0" "shmem doorbell" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='shmem_server'><model type='ivshmem-doorbell'/><server path='/var/lib/libvirt/ivshmem_socket'/></shmem></devices>,g" "$template_xml" > "$test_xml" testme "0" "shmem doorbell serverpath" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ivshmem_socket\"\s*rw,$"
+# For the next test to run apparmor needs to be installed and enabled. +# In some environments (e.g. containers) even though apparmor is +# installed, it is not enabled because securityfs is not mounted. +# In those environments this test cannot run so skip it. +# This test also needs to be run as root. +if [ `eval id -u` = 0 ] && [ -x "$(command -v aa-enabled)" ] && [ `eval aa-enabled` = "Yes" ]; then
This is great to be checked before causing a failure, but a question to the libvirt-CI experts, how doable (or not) would it be to get apparmor installed on those distro testbeds that support it? Are there any good pointers one would start to look at adapting those testbeds?
+ sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk2,g" "$template_xml" > "$test_xml" + # Running the tests does not require libvirt to be installed. As a + # result the appropriate directories have not been created. Create them + # now to run the test. + mkdir -p "$profile_path" + # create a corrupted profile + touch "$profile_path/$valid_uuid" + testme "0" "purge" "-r -u $valid_uuid" "$test_xml" + # All the tests are run with the --dry-run option this test is + # never going to fail because the profile is not going to be loaded. + # However, since we touch the profile if it's still here after the test + # it means that something went wrong, so make the test fail. + if [ -f "$profile_path/$valid_uuid" ]; then + echo "FAIL: failed to purge corrupted profile" >$output + echo " '$extra_args $args': " + errors=$(($errors + 1)) + # remove corrupted profile anyways not to interfere with + # subsequent runs of the tests. + rm "$profile_path/$valid_uuid" + fi +fi + testme "0" "help" "-h"
echo "" >$output -- 2.17.1
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
On Mon, Oct 11, 2021 at 07:59:47AM +0200, Christian Ehrhardt wrote:
On Thu, Oct 7, 2021 at 7:25 PM Ioanna Alifieraki
+# For the next test to run apparmor needs to be installed and enabled. +# In some environments (e.g. containers) even though apparmor is +# installed, it is not enabled because securityfs is not mounted. +# In those environments this test cannot run so skip it. +# This test also needs to be run as root. +if [ `eval id -u` = 0 ] && [ -x "$(command -v aa-enabled)" ] && [ `eval aa-enabled` = "Yes" ]; then
This is great to be checked before causing a failure, but a question to the libvirt-CI experts, how doable (or not) would it be to get apparmor installed on those distro testbeds that support it?
Assuming the necessary packages are included in the container image, what else is needed to have apparmor running? Does apparmor need to be running in the host OS as well for it to work in containers? Does the "securityfs" thing mentioned in the comment above need to be passed through from the host OS? Our CI pipeline uses containers running on the GitLab infrastructure. I'm not sure what they're using as host OS, but if it's something like Fedora for example I would expect that running apparmor would be a problem. If special filesystems need to be passed to the container, that's probably going to pose a challenge too.
Are there any good pointers one would start to look at adapting those testbeds?
The container images are generated from the Dockerfiles in ci/containers, which in turn are generated using the lcitool utility that's being developed as part of https://gitlab.com/libvirt/libvirt-ci/ If you want to include more packages, you should start by defining a mapping for it in guests/lcitool/lcitool/ansible/vars/mappings.yml and then adding it to guests/lcitool/lcitool/ansible/vars/projects/libvirt.yml That's the short version. If you're looking for more information, just let me know and I'll be happy to help :) -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Oct 7, 2021 at 7:25 PM Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> wrote:
This patch-series aims to address the bug reported in [1] and [2].
Bug description : Some times libvirt fails to start a vm with the following error : libvirt: error : unable to set AppArmor profile 'libvirt-b05b297f-952f-42d6-b04e-f9a13767db54' for '/usr/bin/kvm-spice': No such file or directory This happens because file /etc/apparmor.d/libvirt/libvirt-<vm-uuid> has 0 size. During the vm start-up virt-aa-helper tries to load the profile and because it is 0 it fails. When file /etc/apparmor.d/libvirt/libvirt-<vm-uuid> is removed the vm can start without problems.
To address this issue this patch-series suggests the following. On the vm start-up check if the profile has 0 size and if this is the case remove it and create it again. To do so a new option (-P) is introduced and also create and remove profile fuctionalities are placed into separate functions.
The first commit moves create and remove functionlites into functinos for later reuse from different places. The second commit adds a new option (-P) to remove the profile file. The thid commit implements the actual fix (check if the profile has 0 size and if so remove it and create it again). The fourth patch adds a test for the above fix.
I'm generally +1 on the overall approach and wanted to thank you for working on this. It will fix a rare but real issue. Jan had a few requests on 3/4 that all seemed reasonable suggestions, will you submit a v2 addressing those?
[1] https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1927519 [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084
Ioanna Alifieraki (4): virt-aa-helper: Move create and remove profile into separate functions virt-aa-helper: Add new purge (-P) option virt-aa-helper: Purge profile if corrupted virt-aa-helper: test: add test for new option -P
src/security/virt-aa-helper.c | 87 ++++++++++++++++++++++++++--------- tests/meson.build | 1 + tests/virt-aa-helper-test | 29 ++++++++++++ 3 files changed, 96 insertions(+), 21 deletions(-)
-- 2.17.1
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
participants (4)
-
Andrea Bolognani -
Christian Ehrhardt -
Ioanna Alifieraki -
Ján Tomko