[PATCH v2 0/1] virt-aa-helper: Remove corrupted profile
This is a v2 of the patches sent previously and hopefully makes things simpler. (previous patches subject: [PATCH 0/4] virt-aa-helper: Add new option to remove corrupted). This patch 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 checks if the profile has 0 size and if this is the case it removes it. Changes with v1: I incorporated the feedback provided on v1 so the patches change as follows : Patches 1, 2 and 4 from v1 are dropped. The first patch is dropped because according to feedback provided remove_profile is not necessary and in the new version we unlink the profile directly in main(). In addition we skip calling create_profile twice by adding a boolean variable 'purged' if the profile was purged and creation occurs later on in main(). The second patch, which was adding a the option (-P) to remove the profile is dropped because currently this action happens only internally and there is no use case needed to make it available to the users of virt-aa-helper. The third patch which is the actual fix stays but modified. The forth patch which was adding a test to virt-aa-helper-test was the hardest to drop. Although, I'd like to have a test for this case, there is no apparent to make a test for this without having any side effects. The tests in virt-aa-helper-test are run with the --dryrun option and therefore no action should really happen. To test this fix, we need to create a corrupted profile and then remove it violating the dryrun. [1] https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1927519 [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084 Ioanna Alifieraki (1): virt-aa-helper: Purge profile if corrupted src/security/virt-aa-helper.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) -- 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. [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 | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7c21ab9515..218e07bfb0 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1437,6 +1437,8 @@ main(int argc, char **argv) int rc = -1; char *profile = NULL; char *include_file = NULL; + off_t size; + bool purged = 0; if (virGettextInitialize() < 0 || virErrorInitialize() < 0) { @@ -1484,6 +1486,22 @@ 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) { + vah_warning(_("Profile of 0 size detected, will attempt to remove it")); + if ((rc = parserRemove(ctl->uuid) != 0)) + vah_error(ctl, 1, _("could not remove profile")); + unlink(profile); + purged = true; + } + } if (ctl->append && ctl->newfile) { if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) goto cleanup; @@ -1523,7 +1541,7 @@ main(int argc, char **argv) /* create the profile from TEMPLATE */ - if (ctl->cmd == 'c') { + if (ctl->cmd == 'c' || purged) { char *tmp = NULL; tmp = g_strdup_printf(" #include <libvirt/%s.files>\n", ctl->uuid); -- 2.17.1
On a Tuesday 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.
[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 | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Tue, Nov 2, 2021 at 3:04 PM Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> wrote:
This is a v2 of the patches sent previously and hopefully makes things simpler. (previous patches subject: [PATCH 0/4] virt-aa-helper: Add new option to remove corrupted).
This patch 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 checks if the profile has 0 size and if this is the case it removes it.
Changes with v1: I incorporated the feedback provided on v1 so the patches change as follows :
Patches 1, 2 and 4 from v1 are dropped. The first patch is dropped because according to feedback provided remove_profile is not necessary and in the new version we unlink the profile directly in main(). In addition we skip calling create_profile twice by adding a boolean variable 'purged' if the profile was purged and creation occurs later on in main().
The second patch, which was adding a the option (-P) to remove the profile is dropped because currently this action happens only internally and there is no use case needed to make it available to the users of virt-aa-helper.
The third patch which is the actual fix stays but modified.
The forth patch which was adding a test to virt-aa-helper-test was the hardest to drop. Although, I'd like to have a test for this case, there is no apparent to make a test for this without having any side effects. The tests in virt-aa-helper-test are run with the --dryrun option and therefore no action should really happen. To test this fix, we need to create a corrupted profile and then remove it violating the dryrun.
[1] https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1927519 [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084
Ioanna Alifieraki (1): virt-aa-helper: Purge profile if corrupted
Thanks to Jan for making this improved V2 happen. Slightly sad about not having a test, but since (as you explained) it almost never would have ran there isn't much reason for it as-is. I was unsure at first if this now would have an issue when called with -F triggering ctl->append extending the include_files and then (due to empty profile setting purged) going into create_profile. But since you only detect/refresh the profile, but not the include_file that seems to work well even in that case. I appreciate your efforts on this! Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> With v7.9.0 tagged on Monday we are out of the freeze and I think we can apply this unless there is any new negative feedback.
src/security/virt-aa-helper.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
-- 2.17.1
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
On Thu, Nov 4, 2021 at 1:07 PM Christian Ehrhardt <christian.ehrhardt@canonical.com> wrote:
On Tue, Nov 2, 2021 at 3:04 PM Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> wrote:
This is a v2 of the patches sent previously and hopefully makes things simpler. (previous patches subject: [PATCH 0/4] virt-aa-helper: Add new option to remove corrupted).
This patch 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 checks if the profile has 0 size and if this is the case it removes it.
Changes with v1: I incorporated the feedback provided on v1 so the patches change as follows :
Patches 1, 2 and 4 from v1 are dropped. The first patch is dropped because according to feedback provided remove_profile is not necessary and in the new version we unlink the profile directly in main(). In addition we skip calling create_profile twice by adding a boolean variable 'purged' if the profile was purged and creation occurs later on in main().
The second patch, which was adding a the option (-P) to remove the profile is dropped because currently this action happens only internally and there is no use case needed to make it available to the users of virt-aa-helper.
The third patch which is the actual fix stays but modified.
The forth patch which was adding a test to virt-aa-helper-test was the hardest to drop. Although, I'd like to have a test for this case, there is no apparent to make a test for this without having any side effects. The tests in virt-aa-helper-test are run with the --dryrun option and therefore no action should really happen. To test this fix, we need to create a corrupted profile and then remove it violating the dryrun.
[1] https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1927519 [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084
Ioanna Alifieraki (1): virt-aa-helper: Purge profile if corrupted
Thanks to Jan for making this improved V2 happen.
Slightly sad about not having a test, but since (as you explained) it almost never would have ran there isn't much reason for it as-is.
I was unsure at first if this now would have an issue when called with -F triggering ctl->append extending the include_files and then (due to empty profile setting purged) going into create_profile. But since you only detect/refresh the profile, but not the include_file that seems to work well even in that case.
I appreciate your efforts on this! Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
With v7.9.0 tagged on Monday we are out of the freeze and I think we can apply this unless there is any new negative feedback.
FYI - I've ran another set of tests on it and since all were fine I applied it.
src/security/virt-aa-helper.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
-- 2.17.1
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
participants (3)
-
Christian Ehrhardt -
Ioanna Alifieraki -
Ján Tomko