On Tue, Mar 20, 2018 at 9:50 PM, Jamie Strandboge <ja...@canonical.com> wrote:
> On Tue, 2018-03-20 at 13:08 +0100, Christian Ehrhardt wrote: > > Recent changes have made implementing this mandatory to hot add any > > memory. > > Implementing this in apparmor fixes this as well as allows hot-add of > > nvdimm > > tpye memory with an nvdimmPath set generating a AppArmor rule for > > that > > path. > > > > Example hot adding: > > <memory model='nvdimm'> > > <source> > > <path>/tmp/nvdimm-test</path> > > </source> > > <target> > > <size unit='KiB'>524288</size> > > <node>0</node> > > </target> > > </memory> > > Creates now: > > "/tmp/nvdimm-test" rwk, > > > > Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com> > > --- > > src/security/security_apparmor.c | 50 > > ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 50 insertions(+) > > > > diff --git a/src/security/security_apparmor.c > > b/src/security/security_apparmor.c > > index a989992..4ae1e3d 100644 > > --- a/src/security/security_apparmor.c > > +++ b/src/security/security_apparmor.c > > @@ -718,6 +718,53 @@ > > AppArmorRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, > > > > /* Called when hotplugging */ > > static int > > +AppArmorSetMemoryLabel(virSecurityManagerPtr mgr, > > + virDomainDefPtr def, > > + virDomainMemoryDefPtr mem) > > +{ > > + virSecurityLabelDefPtr secdef; > > + > > + switch ((virDomainMemoryModel) mem->model) { > > + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: > > + secdef = virDomainDefGetSecurityLabelDef(def, > > SECURITY_APPARMOR_NAME); > > + if (!secdef || !secdef->relabel) > > + return 0; > > + > > + if (!virFileExists(mem->nvdimmPath)) { > > + virReportError(VIR_ERR_INTERNAL_ERROR, > > + _("%s: \'%s\' does not exist"), > > + __func__, mem->nvdimmPath); > > + return -1; > > + } > > + > > + return reload_profile(mgr, def, mem->nvdimmPath, true); > > + break; > > + case VIR_DOMAIN_MEMORY_MODEL_NONE: > > + case VIR_DOMAIN_MEMORY_MODEL_DIMM: > > + case VIR_DOMAIN_MEMORY_MODEL_LAST: > > + break; > > + } > > + > > + return 0; > > +} > > + > > + > > +static int > > +AppArmorRestoreMemoryLabel(virSecurityManagerPtr mgr, > > + virDomainDefPtr def, > > + virDomainMemoryDefPtr mem > > ATTRIBUTE_UNUSED) > > +{ > > + virSecurityLabelDefPtr secdef = > > + virDomainDefGetSecurityLabelDef(def, > > SECURITY_APPARMOR_NAME); > > + > > + if (!secdef || !secdef->relabel) > > + return 0; > > + > > This secdef check is done already in reload_profile. > I was following other examples, but you are right - I'll drop it in a V2 > > + return reload_profile(mgr, def, NULL, false); > > Calling this with NULL means that virt-aa-helper will be called and > remove the mem file, but will it also remove anything else that was > hotplugged and not present in 'xml = virDomainDefFormat(def, ...)'? > I've not looked at this in a while, so maybe it's ok (it seems like > this is what the various other AppArmorRestore* functions do after > all). Yeah I was following other examples that were present in the file already. It is a complex case to follow "what will happen" > A test case for hotplugging all these different devices and > hotunplugging each would be good to show if hotunplugging one > inadvertently removes rules for other devices that are still > hotplugged. > I agree, first I ensured I actually test the requested code. Disks should be going through AppArmorRestoreSecurityImageLabel I ensured with GDB that they do on detach. reload_profile (mgr=0x7f96741060e0, def=0x7f967418b480, fn=0x0, append=false) at ../../../src/security/security_apparmor.c:287 That way we can be sure that attach/detach disks is a valid test for the reload_profile call with NULL. Test with two disks $ qemu-img create /var/lib/libvirt/images/hp-test.qcow 10M $ qemu-img create /var/lib/libvirt/images/hp-test2.qcow 10M root@b:~# cat hotplug-disk cat: hotplug-disk: No such file or directory root@b:~# cat hotplug-disk.xml <!-- qemu-img create /var/lib/libvirt/images/hp-test.qcow 10M --> <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/hp-test.qcow'/> <target dev='vdy' bus='virtio'/> </disk> root@b:~# cat hotplug-disk2.xml <!-- qemu-img create /var/lib/libvirt/images/hp-test.qcow 10M --> <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/hp-test2.qcow'/> <target dev='vdz' bus='virtio'/> </disk> root@b:~# cat /etc/apparmor.d/libvirt/libvirt-$(virsh dominfo b-test | awk '/^UUID:/ {print $2}').files | grep hp root@b:~# virsh attach-device b-test hotplug-disk.xml --live Device attached successfully root@b:~# cat /etc/apparmor.d/libvirt/libvirt-$(virsh dominfo b-test | awk '/^UUID:/ {print $2}').files | grep hp "/var/lib/libvirt/images/hp-test.qcow" rwk, root@b:~# virsh attach-device b-test hotplug-disk2.xml --live Device attached successfully root@b:~# cat /etc/apparmor.d/libvirt/libvirt-$(virsh dominfo b-test | awk '/^UUID:/ {print $2}').files | grep hp "/var/lib/libvirt/images/hp-test.qcow" rwk, "/var/lib/libvirt/images/hp-test2.qcow" rwk, root@b:~# virsh detach-device b-test hotplug-disk.xml --live Device detached successfully root@b:~# cat /etc/apparmor.d/libvirt/libvirt-$(virsh dominfo b-test | awk '/^UUID:/ {print $2}').files | grep hp "/var/lib/libvirt/images/hp-test2.qcow" rwk, The one not detached remains in the profile as it should. So it seems safe against the mentioned "accidental" removal of the other hotplugged devices. I can't see how to do so in a unit test atm, maybe down the road in a dep8 or qa test. But the test above should reassure you that it is not a generic problem currently. > > +} > > + > > +/* Called when hotplugging */ > > +static int > > AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, > > virDomainDefPtr def, > > virStorageSourcePtr src) > > @@ -1115,6 +1162,9 @@ virSecurityDriver virAppArmorSecurityDriver = { > > .domainSetSecurityImageLabel = > > AppArmorSetSecurityImageLabel, > > .domainRestoreSecurityImageLabel = > > AppArmorRestoreSecurityImageLabel, > > > > + .domainSetSecurityMemoryLabel = AppArmorSetMemoryLabel, > > + .domainRestoreSecurityMemoryLabel = > > AppArmorRestoreMemoryLabel, > > + > > .domainSetSecurityDaemonSocketLabel = > > AppArmorSetSecurityDaemonSocketLabel, > > .domainSetSecuritySocketLabel = > > AppArmorSetSecuritySocketLabel, > > .domainClearSecuritySocketLabel = > > AppArmorClearSecuritySocketLabel, > -- > Jamie Strandboge | http://www.canonical.com -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list