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

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