[libvirt] [PATCH] qemuProcessHandleMonitorEOF: Disable namespace for domain

Michal Privoznik posted 1 patch 7 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/76da32831e6ca87787f09b11f17433a56a975e92.1489149683.git.mprivozn@redhat.com
src/qemu/qemu_domain.c  | 25 +++++++++++++++++++++++++
src/qemu/qemu_domain.h  |  3 +++
src/qemu/qemu_process.c |  4 ++++
3 files changed, 32 insertions(+)
[libvirt] [PATCH] qemuProcessHandleMonitorEOF: Disable namespace for domain
Posted by Michal Privoznik 7 years ago
https://bugzilla.redhat.com/show_bug.cgi?id=1430634

If a qemu process has died, we get EOF on its monitor. At this
point, since qemu process was the only one running in the
namespace kernel has already cleaned the namespace up. Any
attempt of ours to enter it has to fail.

This really happened in the bug linked above. We've tried to
attach a disk to qemu and while we were in the monitor talking to
qemu it just died. Therefore our code tried to do some roll back
(e.g. deny the device in cgroups again, restore labels, etc.).
However, during the roll back (esp. when restoring labels) we
still thought that domain has a namespace. So we used secdriver's
transactions. This failed as there is no namespace to enter.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain.c  | 25 +++++++++++++++++++++++++
 src/qemu/qemu_domain.h  |  3 +++
 src/qemu/qemu_process.c |  4 ++++
 3 files changed, 32 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1a42fcf1b..d5833b026 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -201,6 +201,22 @@ qemuDomainEnableNamespace(virDomainObjPtr vm,
 }
 
 
+static void
+qemuDomainDisableNamespace(virDomainObjPtr vm,
+                           qemuDomainNamespace ns)
+{
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+
+    if (priv->namespaces) {
+        ignore_value(virBitmapClearBit(priv->namespaces, ns));
+        if (virBitmapIsAllClear(priv->namespaces)) {
+            virBitmapFree(priv->namespaces);
+            priv->namespaces = NULL;
+        }
+    }
+}
+
+
 void qemuDomainEventQueue(virQEMUDriverPtr driver,
                           virObjectEventPtr event)
 {
@@ -7805,6 +7821,15 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver,
 }
 
 
+void
+qemuDomainDestroyNamespace(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+                           virDomainObjPtr vm)
+{
+    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
+        qemuDomainDisableNamespace(vm, QEMU_DOMAIN_NS_MOUNT);
+}
+
+
 bool
 qemuDomainNamespaceAvailable(qemuDomainNamespace ns ATTRIBUTE_UNUSED)
 {
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 7fa717390..c646828e6 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -816,6 +816,9 @@ int qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
 int qemuDomainCreateNamespace(virQEMUDriverPtr driver,
                               virDomainObjPtr vm);
 
+void qemuDomainDestroyNamespace(virQEMUDriverPtr driver,
+                                virDomainObjPtr vm);
+
 bool qemuDomainNamespaceAvailable(qemuDomainNamespace ns);
 
 int qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fcba7d309..b9c1847bb 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -314,6 +314,10 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
      */
     qemuMonitorUnregister(mon);
 
+    /* We don't want any cleanup from EOF handler (or any other
+     * thread) to enter qemu namespace. */
+    qemuDomainDestroyNamespace(driver, vm);
+
  cleanup:
     virObjectUnlock(vm);
 }
-- 
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuProcessHandleMonitorEOF: Disable namespace for domain
Posted by Martin Kletzander 7 years ago
On Fri, Mar 10, 2017 at 01:41:23PM +0100, Michal Privoznik wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1430634
>
>If a qemu process has died, we get EOF on its monitor. At this
>point, since qemu process was the only one running in the
>namespace kernel has already cleaned the namespace up. Any
>attempt of ours to enter it has to fail.
>
>This really happened in the bug linked above. We've tried to
>attach a disk to qemu and while we were in the monitor talking to
>qemu it just died. Therefore our code tried to do some roll back
>(e.g. deny the device in cgroups again, restore labels, etc.).
>However, during the roll back (esp. when restoring labels) we
>still thought that domain has a namespace. So we used secdriver's
>transactions. This failed as there is no namespace to enter.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_domain.c  | 25 +++++++++++++++++++++++++
> src/qemu/qemu_domain.h  |  3 +++
> src/qemu/qemu_process.c |  4 ++++
> 3 files changed, 32 insertions(+)
>
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index 1a42fcf1b..d5833b026 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -201,6 +201,22 @@ qemuDomainEnableNamespace(virDomainObjPtr vm,
> }
>
>
>+static void
>+qemuDomainDisableNamespace(virDomainObjPtr vm,
>+                           qemuDomainNamespace ns)
>+{
>+    qemuDomainObjPrivatePtr priv = vm->privateData;
>+
>+    if (priv->namespaces) {
>+        ignore_value(virBitmapClearBit(priv->namespaces, ns));
>+        if (virBitmapIsAllClear(priv->namespaces)) {
>+            virBitmapFree(priv->namespaces);
>+            priv->namespaces = NULL;
>+        }
>+    }
>+}
>+

This function is written in a way that...

>@@ -7805,6 +7821,15 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver,
> }
>
>
>+void
>+qemuDomainDestroyNamespace(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>+                           virDomainObjPtr vm)
>+{
>+    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>+        qemuDomainDisableNamespace(vm, QEMU_DOMAIN_NS_MOUNT);
>+}
>+

...this wrapper over it is kinda useless.  But your call on whether to
keep it (I get there are some "consistency" and "naming" reasons).

ACK to this, although I feel like commit 3e6839d4e801 should be
reverted, but that's up for a discussion.  It does not make *any*
difference now, but I just feel like it's cleaner that way.

Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuProcessHandleMonitorEOF: Disable namespace for domain
Posted by Michal Privoznik 7 years ago
On 03/10/2017 03:50 PM, Martin Kletzander wrote:
> On Fri, Mar 10, 2017 at 01:41:23PM +0100, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1430634
>>
>> If a qemu process has died, we get EOF on its monitor. At this
>> point, since qemu process was the only one running in the
>> namespace kernel has already cleaned the namespace up. Any
>> attempt of ours to enter it has to fail.
>>
>> This really happened in the bug linked above. We've tried to
>> attach a disk to qemu and while we were in the monitor talking to
>> qemu it just died. Therefore our code tried to do some roll back
>> (e.g. deny the device in cgroups again, restore labels, etc.).
>> However, during the roll back (esp. when restoring labels) we
>> still thought that domain has a namespace. So we used secdriver's
>> transactions. This failed as there is no namespace to enter.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/qemu/qemu_domain.c  | 25 +++++++++++++++++++++++++
>> src/qemu/qemu_domain.h  |  3 +++
>> src/qemu/qemu_process.c |  4 ++++
>> 3 files changed, 32 insertions(+)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 1a42fcf1b..d5833b026 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -201,6 +201,22 @@ qemuDomainEnableNamespace(virDomainObjPtr vm,
>> }
>>
>>
>> +static void
>> +qemuDomainDisableNamespace(virDomainObjPtr vm,
>> +                           qemuDomainNamespace ns)
>> +{
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +
>> +    if (priv->namespaces) {
>> +        ignore_value(virBitmapClearBit(priv->namespaces, ns));
>> +        if (virBitmapIsAllClear(priv->namespaces)) {
>> +            virBitmapFree(priv->namespaces);
>> +            priv->namespaces = NULL;
>> +        }
>> +    }
>> +}
>> +
> 
> This function is written in a way that...
> 
>> @@ -7805,6 +7821,15 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver,
>> }
>>
>>
>> +void
>> +qemuDomainDestroyNamespace(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>> +                           virDomainObjPtr vm)
>> +{
>> +    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>> +        qemuDomainDisableNamespace(vm, QEMU_DOMAIN_NS_MOUNT);
>> +}
>> +
> 
> ...this wrapper over it is kinda useless.  But your call on whether to
> keep it (I get there are some "consistency" and "naming" reasons).

Yeah, I'd like to keep it for consistency reasons. I'll push this as is
and if somebody disagree I'm happy to review their patch.

> 
> ACK to this, although I feel like commit 3e6839d4e801 should be
> reverted, but that's up for a discussion.  It does not make *any*
> difference now, but I just feel like it's cleaner that way.

I was thinking about this too and I'm torn. I mean, on one side it would
be great from consistency POV. On the other we would be effectively
adding dead code. So I guess for now lets stick with status quo and not
revert it.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list