Re: [libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
On Mon, Aug 20, 2018 at 07:13:46PM +0200, Michal Prívozník wrote: > On 08/20/2018 05:07 PM, Daniel P. Berrangé wrote: > > On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote: > >> In order for our drivers to lock resources for metadata change we > >> need set of new APIs. Fortunately, we don't have to care about > >> every possible device a domain can have. We care only about those > >> which can live on a network filesystem and hence can be accessed > >> by multiple daemons at the same time. These devices are covered > >> in virDomainLockMetadataLock() and only a small fraction of > >> those can be hotplugged (covered in the rest of the introduced > >> APIs). > > > > I'm not sure I understand the rationale behind saying we only care > > about resources on network filesystems. > > > > If I have 2 locally running guests, and both have a serial port > > backed by a physical serial port, eg > > > > > > > > > > > > > > we *do* care about locking /dev/ttyS0, as libvirtd isn't doing > > mutual exclusion checks anywhere else for the /dev/ttyS0 device > > node. > > Ah you mean that the system wide daemon and session daemon could clash > when relabeling /dev/ttyS0? Well, we don't do relabeling for session > daemons and running two system daemons is not supported (you're gonna > hit more serious problems when trying that anyway). We certainly *do* have relabelling for session daemons, for SELinux: $ ls -alZ ~/VirtualMachines/demo.img -rw-r--r--. 1 berrange berrange unconfined_u:object_r:virt_home_t:s0 1073741824 Aug 21 09:25 /home/berrange/VirtualMachines/demo.img $ virsh uri qemu:///session $ virsh start QEMUGuest1 Domain QEMUGuest1 started $ ls -alZ ~/VirtualMachines/demo.img -rw-r--r--. 1 berrange berrange unconfined_u:object_r:svirt_image_t:s0:c310,c831 1073741824 Aug 21 09:25 /home/berrange/VirtualMachines/demo.img but I was more thinking about the future where we have the libvirt shim concept that allows starting & managing of QEMU processes independantly, and libvirtd is merely there for aggregated data reporting. In that case you'd have many instances of the security driver operating in parallel. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
On 08/20/2018 05:07 PM, Daniel P. Berrangé wrote: > On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote: >> In order for our drivers to lock resources for metadata change we >> need set of new APIs. Fortunately, we don't have to care about >> every possible device a domain can have. We care only about those >> which can live on a network filesystem and hence can be accessed >> by multiple daemons at the same time. These devices are covered >> in virDomainLockMetadataLock() and only a small fraction of >> those can be hotplugged (covered in the rest of the introduced >> APIs). > > I'm not sure I understand the rationale behind saying we only care > about resources on network filesystems. > > If I have 2 locally running guests, and both have a serial port > backed by a physical serial port, eg > > > > > > > we *do* care about locking /dev/ttyS0, as libvirtd isn't doing > mutual exclusion checks anywhere else for the /dev/ttyS0 device > node. Ah you mean that the system wide daemon and session daemon could clash when relabeling /dev/ttyS0? Well, we don't do relabeling for session daemons and running two system daemons is not supported (you're gonna hit more serious problems when trying that anyway). > > In general I think we need to lock every single file resource > that is labelled for a guest, regardless of whether its local > or remote. Well this might be feasible iff locking would be done from security driver. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
On Mon, Aug 20, 2018 at 04:07:28PM +0100, Daniel P. Berrangé wrote: > On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote: > > In order for our drivers to lock resources for metadata change we > > need set of new APIs. Fortunately, we don't have to care about > > every possible device a domain can have. We care only about those > > which can live on a network filesystem and hence can be accessed > > by multiple daemons at the same time. These devices are covered > > in virDomainLockMetadataLock() and only a small fraction of > > those can be hotplugged (covered in the rest of the introduced > > APIs). > > I'm not sure I understand the rationale behind saying we only care > about resources on network filesystems. > > If I have 2 locally running guests, and both have a serial port > backed by a physical serial port, eg > > > > > > > we *do* care about locking /dev/ttyS0, as libvirtd isn't doing > mutual exclusion checks anywhere else for the /dev/ttyS0 device > node. > > In general I think we need to lock every single file resource > that is labelled for a guest, regardless of whether its local > or remote. In the next patch I propose integration into the security manager that would avoid the need to touch this domain lock abstraction at all. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote: > In order for our drivers to lock resources for metadata change we > need set of new APIs. Fortunately, we don't have to care about > every possible device a domain can have. We care only about those > which can live on a network filesystem and hence can be accessed > by multiple daemons at the same time. These devices are covered > in virDomainLockMetadataLock() and only a small fraction of > those can be hotplugged (covered in the rest of the introduced > APIs). I'm not sure I understand the rationale behind saying we only care about resources on network filesystems. If I have 2 locally running guests, and both have a serial port backed by a physical serial port, eg we *do* care about locking /dev/ttyS0, as libvirtd isn't doing mutual exclusion checks anywhere else for the /dev/ttyS0 device node. In general I think we need to lock every single file resource that is labelled for a guest, regardless of whether its local or remote. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
On 08/17/2018 07:58 PM, John Ferlan wrote:
>
>
> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
>> In order for our drivers to lock resources for metadata change we
>> need set of new APIs. Fortunately, we don't have to care about
>> every possible device a domain can have. We care only about those
>> which can live on a network filesystem and hence can be accessed
>> by multiple daemons at the same time. These devices are covered
>> in virDomainLockMetadataLock() and only a small fraction of
>> those can be hotplugged (covered in the rest of the introduced
>> APIs).
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/libvirt_private.syms | 8 ++
>> src/locking/domain_lock.c | 304
>> ++
>> src/locking/domain_lock.h | 28 +
>> 3 files changed, 317 insertions(+), 23 deletions(-)
>>
>
> There seems to be more than just what's described going on in this patch
> which could be split out
>
> 1. Use @def in virDomainLockManagerNew
>
> 2. Add @metadataonly to virDomainLockManagerNew and
> virDomainLockManagerAddImage
>
> 3. Introduce virDomainLockManagerAddMemory and
> virDomainLockManagerAddFile along with changes to
> virDomainLockManagerNew along with perhaps a few words why those files
> were chosen and what decisions led to their selection. If someone adds
> something in the future how would they know to add them to the list?
>
> 4. Various virDomainLockMetadata* API's.
>
D'oh! Okay, I'll split this.
>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index ca4a192a4a..720ae12301 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1284,6 +1284,14 @@ virDomainLockImageAttach;
>> virDomainLockImageDetach;
>> virDomainLockLeaseAttach;
>> virDomainLockLeaseDetach;
>> +virDomainLockMetadataDiskLock;
>> +virDomainLockMetadataDiskUnlock;
>> +virDomainLockMetadataImageLock;
>> +virDomainLockMetadataImageUnlock;
>> +virDomainLockMetadataLock;
>> +virDomainLockMetadataMemLock;
>> +virDomainLockMetadataMemUnlock;
>> +virDomainLockMetadataUnlock;
>> virDomainLockProcessInquire;
>> virDomainLockProcessPause;
>> virDomainLockProcessResume;
>> diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
>> index 705b132457..19a097fb25 100644
>> --- a/src/locking/domain_lock.c
>> +++ b/src/locking/domain_lock.c
>> @@ -69,7 +69,8 @@ static int virDomainLockManagerAddLease(virLockManagerPtr
>> lock,
>>
>>
>> static int virDomainLockManagerAddImage(virLockManagerPtr lock,
>> -virStorageSourcePtr src)
>> +virStorageSourcePtr src,
>> +bool metadataOnly)
>> {
>> unsigned int diskFlags = 0;
>> int type = virStorageSourceGetActualType(src);
>> @@ -82,10 +83,14 @@ static int
>> virDomainLockManagerAddImage(virLockManagerPtr lock,
>>type == VIR_STORAGE_TYPE_DIR))
>> return 0;
>>
>> -if (src->readonly)
>> -diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY;
>> -if (src->shared)
>> -diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED;
>> +if (metadataOnly) {
>> +diskFlags = VIR_LOCK_MANAGER_RESOURCE_METADATA;
>> +} else {
>> +if (src->readonly)
>> +diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY;
>> +if (src->shared)
>> +diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED;
>> +}
>>
>> VIR_DEBUG("Add disk %s", src->path);
>> if (virLockManagerAddResource(lock,
>> @@ -101,13 +106,68 @@ static int
>> virDomainLockManagerAddImage(virLockManagerPtr lock,
>> }
>>
>>
>> +static int
>> +virDomainLockManagerAddMemory(virLockManagerPtr lock,
>> + const virDomainMemoryDef *mem)
>> +{
>> +const char *path = NULL;
>> +
>> +switch ((virDomainMemoryModel) mem->model) {
>> +case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
>> +path = mem->nvdimmPath;
>
> Why not flip flop the order of new helpers and just call/return
> virDomainLockManagerAddFile directly with mem->nvdimmPath
>
>> +break;
>> +
>> +case VIR_DOMAIN_MEMORY_MODEL_DIMM:
>> +case VIR_DOMAIN_MEMORY_MODEL_LAST:
>> +case VIR_DOMAIN_MEMORY_MODEL_NONE:
>> +break;
>> +}
>> +
>> +if (!path)
>> +return 0;
>
> And then just return 0 here. or just call it here since it returns 0 if
> @!file anyway.
Well, I think it's just a matter of preference though. My preference was
to have this future extensible. I mean, if we ever introduce new model
that we need to lock, this switch() would just get its path and the rest
is already handled. Whereas if I'd move AddResource() call right into
the switch() the call would be duplicated then.
>
>> +
>> +VIR_DEBUG("Adding memory %s", path);
>> +if (virLockManagerAddResource(lock,
>> + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
>> + path,
Re: [libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> In order for our drivers to lock resources for metadata change we
> need set of new APIs. Fortunately, we don't have to care about
> every possible device a domain can have. We care only about those
> which can live on a network filesystem and hence can be accessed
> by multiple daemons at the same time. These devices are covered
> in virDomainLockMetadataLock() and only a small fraction of
> those can be hotplugged (covered in the rest of the introduced
> APIs).
>
> Signed-off-by: Michal Privoznik
> ---
> src/libvirt_private.syms | 8 ++
> src/locking/domain_lock.c | 304
> ++
> src/locking/domain_lock.h | 28 +
> 3 files changed, 317 insertions(+), 23 deletions(-)
>
There seems to be more than just what's described going on in this patch
which could be split out
1. Use @def in virDomainLockManagerNew
2. Add @metadataonly to virDomainLockManagerNew and
virDomainLockManagerAddImage
3. Introduce virDomainLockManagerAddMemory and
virDomainLockManagerAddFile along with changes to
virDomainLockManagerNew along with perhaps a few words why those files
were chosen and what decisions led to their selection. If someone adds
something in the future how would they know to add them to the list?
4. Various virDomainLockMetadata* API's.
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ca4a192a4a..720ae12301 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1284,6 +1284,14 @@ virDomainLockImageAttach;
> virDomainLockImageDetach;
> virDomainLockLeaseAttach;
> virDomainLockLeaseDetach;
> +virDomainLockMetadataDiskLock;
> +virDomainLockMetadataDiskUnlock;
> +virDomainLockMetadataImageLock;
> +virDomainLockMetadataImageUnlock;
> +virDomainLockMetadataLock;
> +virDomainLockMetadataMemLock;
> +virDomainLockMetadataMemUnlock;
> +virDomainLockMetadataUnlock;
> virDomainLockProcessInquire;
> virDomainLockProcessPause;
> virDomainLockProcessResume;
> diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
> index 705b132457..19a097fb25 100644
> --- a/src/locking/domain_lock.c
> +++ b/src/locking/domain_lock.c
> @@ -69,7 +69,8 @@ static int virDomainLockManagerAddLease(virLockManagerPtr
> lock,
>
>
> static int virDomainLockManagerAddImage(virLockManagerPtr lock,
> -virStorageSourcePtr src)
> +virStorageSourcePtr src,
> +bool metadataOnly)
> {
> unsigned int diskFlags = 0;
> int type = virStorageSourceGetActualType(src);
> @@ -82,10 +83,14 @@ static int virDomainLockManagerAddImage(virLockManagerPtr
> lock,
>type == VIR_STORAGE_TYPE_DIR))
> return 0;
>
> -if (src->readonly)
> -diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY;
> -if (src->shared)
> -diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED;
> +if (metadataOnly) {
> +diskFlags = VIR_LOCK_MANAGER_RESOURCE_METADATA;
> +} else {
> +if (src->readonly)
> +diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY;
> +if (src->shared)
> +diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED;
> +}
>
> VIR_DEBUG("Add disk %s", src->path);
> if (virLockManagerAddResource(lock,
> @@ -101,13 +106,68 @@ static int
> virDomainLockManagerAddImage(virLockManagerPtr lock,
> }
>
>
> +static int
> +virDomainLockManagerAddMemory(virLockManagerPtr lock,
> + const virDomainMemoryDef *mem)
> +{
> +const char *path = NULL;
> +
> +switch ((virDomainMemoryModel) mem->model) {
> +case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +path = mem->nvdimmPath;
Why not flip flop the order of new helpers and just call/return
virDomainLockManagerAddFile directly with mem->nvdimmPath
> +break;
> +
> +case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> +case VIR_DOMAIN_MEMORY_MODEL_LAST:
> +case VIR_DOMAIN_MEMORY_MODEL_NONE:
> +break;
> +}
> +
> +if (!path)
> +return 0;
And then just return 0 here. or just call it here since it returns 0 if
@!file anyway.
> +
> +VIR_DEBUG("Adding memory %s", path);
> +if (virLockManagerAddResource(lock,
> + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
> + path,
> + 0,
> + NULL,
> + VIR_LOCK_MANAGER_RESOURCE_METADATA) < 0)
> +return -1;
> +
> +return 0;
> +}
> +
> +
> +static int
> +virDomainLockManagerAddFile(virLockManagerPtr lock,
> +const char *file)
> +{
> +if (!file)
> +return 0;
> +
> +VIR_DEBUG("Adding file %s", file);
> +if (virLockManagerAddResource(lock,
> + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
> +
[libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
In order for our drivers to lock resources for metadata change we
need set of new APIs. Fortunately, we don't have to care about
every possible device a domain can have. We care only about those
which can live on a network filesystem and hence can be accessed
by multiple daemons at the same time. These devices are covered
in virDomainLockMetadataLock() and only a small fraction of
those can be hotplugged (covered in the rest of the introduced
APIs).
Signed-off-by: Michal Privoznik
---
src/libvirt_private.syms | 8 ++
src/locking/domain_lock.c | 304 ++
src/locking/domain_lock.h | 28 +
3 files changed, 317 insertions(+), 23 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ca4a192a4a..720ae12301 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1284,6 +1284,14 @@ virDomainLockImageAttach;
virDomainLockImageDetach;
virDomainLockLeaseAttach;
virDomainLockLeaseDetach;
+virDomainLockMetadataDiskLock;
+virDomainLockMetadataDiskUnlock;
+virDomainLockMetadataImageLock;
+virDomainLockMetadataImageUnlock;
+virDomainLockMetadataLock;
+virDomainLockMetadataMemLock;
+virDomainLockMetadataMemUnlock;
+virDomainLockMetadataUnlock;
virDomainLockProcessInquire;
virDomainLockProcessPause;
virDomainLockProcessResume;
diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
index 705b132457..19a097fb25 100644
--- a/src/locking/domain_lock.c
+++ b/src/locking/domain_lock.c
@@ -69,7 +69,8 @@ static int virDomainLockManagerAddLease(virLockManagerPtr
lock,
static int virDomainLockManagerAddImage(virLockManagerPtr lock,
-virStorageSourcePtr src)
+virStorageSourcePtr src,
+bool metadataOnly)
{
unsigned int diskFlags = 0;
int type = virStorageSourceGetActualType(src);
@@ -82,10 +83,14 @@ static int virDomainLockManagerAddImage(virLockManagerPtr
lock,
type == VIR_STORAGE_TYPE_DIR))
return 0;
-if (src->readonly)
-diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY;
-if (src->shared)
-diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED;
+if (metadataOnly) {
+diskFlags = VIR_LOCK_MANAGER_RESOURCE_METADATA;
+} else {
+if (src->readonly)
+diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY;
+if (src->shared)
+diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED;
+}
VIR_DEBUG("Add disk %s", src->path);
if (virLockManagerAddResource(lock,
@@ -101,13 +106,68 @@ static int virDomainLockManagerAddImage(virLockManagerPtr
lock,
}
+static int
+virDomainLockManagerAddMemory(virLockManagerPtr lock,
+ const virDomainMemoryDef *mem)
+{
+const char *path = NULL;
+
+switch ((virDomainMemoryModel) mem->model) {
+case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
+path = mem->nvdimmPath;
+break;
+
+case VIR_DOMAIN_MEMORY_MODEL_DIMM:
+case VIR_DOMAIN_MEMORY_MODEL_LAST:
+case VIR_DOMAIN_MEMORY_MODEL_NONE:
+break;
+}
+
+if (!path)
+return 0;
+
+VIR_DEBUG("Adding memory %s", path);
+if (virLockManagerAddResource(lock,
+ VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
+ path,
+ 0,
+ NULL,
+ VIR_LOCK_MANAGER_RESOURCE_METADATA) < 0)
+return -1;
+
+return 0;
+}
+
+
+static int
+virDomainLockManagerAddFile(virLockManagerPtr lock,
+const char *file)
+{
+if (!file)
+return 0;
+
+VIR_DEBUG("Adding file %s", file);
+if (virLockManagerAddResource(lock,
+ VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
+ file,
+ 0,
+ NULL,
+ VIR_LOCK_MANAGER_RESOURCE_METADATA) < 0)
+return -1;
+
+return 0;
+}
+
+
static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr
plugin,
const char *uri,
virDomainObjPtr dom,
bool withResources,
+ bool metadataOnly,
unsigned int flags)
{
virLockManagerPtr lock;
+const virDomainDef *def = dom->def;
size_t i;
virLockManagerParam params[] = {
{ .type = VIR_LOCK_MANAGER_PARAM_TYPE_UUID,
@@ -115,11 +175,11 @@ static virLockManagerPtr
virDomainLockManagerNew(virLockManagerPluginPtr plugin,
},
{ .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING,
.key = "name",
- .value = { .str = dom->def->name },
+
