[libvirt] [PATCH v2 0/2] nwfilter: Fix a couple of session mode issues
v1: https://www.redhat.com/archives/libvir-list/2018-August/msg01464.html Changes in v2 - different approach as review pointed out we should never open the nwfilter driver in session mode (although driver initialization does set up some barebones list infrastructure). First, let's make sure we don't allow creation of the nwfilter filter binding similar to how nwfiler filter creation is not allowed. Second, rather than blindly open the nwfilter during the teardown processing, let's first ensure a filter exists for the network. It's not possible to call instantiation when net->filter == NULL. Rather than alter all the callers, just alter the two teardown API's to check if !net->filter and return prior to opening the nwfilter connection. Since we cannot create a filter nor can we create a binding, this filtering works. Keeps the changes minimal too. John Ferlan (2): nwfilter: Disallow binding creation in session mode nwfilter: Check for filter presence before open connect during teardown src/conf/domain_nwfilter.c | 22 +++++++++++++++------- src/nwfilter/nwfilter_driver.c | 6 ++++++ 2 files changed, 21 insertions(+), 7 deletions(-) -- 2.17.1
Similar to nwfilterDefineXML, let's be sure the a filter binding creation is not attempted in session mode and generate the proper error message. Failure to open nwfilter in session mode (nwfilterConnectOpen) fails already, but that doesn't stop the free thinker from using a different connection in order to attempt to attempt to create the binding. Although even doing that would result in a failure: $ virsh nwfilter-binding-create QEMUGuest1-binding.xml error: Failed to create network filter from QEMUGuest1-binding.xml error: internal error: Could not get access to ACL tech driver 'ebiptables' $ Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index ac3a964388..1ee5162b9a 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -745,6 +745,12 @@ nwfilterBindingCreateXML(virConnectPtr conn, virCheckFlags(0, NULL); + if (!driver->privileged) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't define NWFilter bindings in session mode")); + return NULL; + } + def = virNWFilterBindingDefParseString(xml); if (!def) return NULL; -- 2.17.1
On Thu, Aug 30, 2018 at 11:06:07AM -0400, John Ferlan wrote:
Similar to nwfilterDefineXML, let's be sure the a filter binding creation is not attempted in session mode and generate the proper error message.
Failure to open nwfilter in session mode (nwfilterConnectOpen) fails already, but that doesn't stop the free thinker from using a different connection in order to attempt to attempt to create the binding. Although even doing that would result in a failure:
$ virsh nwfilter-binding-create QEMUGuest1-binding.xml error: Failed to create network filter from QEMUGuest1-binding.xml error: internal error: Could not get access to ACL tech driver 'ebiptables'
$
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index ac3a964388..1ee5162b9a 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -745,6 +745,12 @@ nwfilterBindingCreateXML(virConnectPtr conn,
virCheckFlags(0, NULL);
+ if (!driver->privileged) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't define NWFilter bindings in session mode")); + return NULL; + } + def = virNWFilterBindingDefParseString(xml); if (!def) return NULL;
How do we ever get to this point in a session daemon ? The nwfilterConnectOpen() method should have failed due to 'driver' being NULL, so the virConnectPtr doesn't exist and so no driver callback points to nwfilterBindingCreateXML. 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 :|
On 08/30/2018 12:27 PM, Daniel P. Berrangé wrote:
On Thu, Aug 30, 2018 at 11:06:07AM -0400, John Ferlan wrote:
Similar to nwfilterDefineXML, let's be sure the a filter binding creation is not attempted in session mode and generate the proper error message.
Failure to open nwfilter in session mode (nwfilterConnectOpen) fails already, but that doesn't stop the free thinker from using a different connection in order to attempt to attempt to create the binding. Although even doing that would result in a failure:
$ virsh nwfilter-binding-create QEMUGuest1-binding.xml error: Failed to create network filter from QEMUGuest1-binding.xml error: internal error: Could not get access to ACL tech driver 'ebiptables'
$
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index ac3a964388..1ee5162b9a 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -745,6 +745,12 @@ nwfilterBindingCreateXML(virConnectPtr conn,
virCheckFlags(0, NULL);
+ if (!driver->privileged) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't define NWFilter bindings in session mode")); + return NULL; + } + def = virNWFilterBindingDefParseString(xml); if (!def) return NULL;
How do we ever get to this point in a session daemon ?
Like I noted in the commit message an enterprising user... With no guest running: $ virsh -c qemu:///session nwfilter-binding-create QEMUGuest1-binding.xml Network filter binding on tap0 created from QEMUGuest1-binding.xml $ With a guest running, one would get the error: $ virsh -c qemu:///session nwfilter-binding-create QEMUGuest1-binding.xml error: Failed to create network filter from QEMUGuest1-binding.xml error: internal error: Could not get access to ACL tech driver 'ebiptables' $ It fails now, so I suppose it doesn't matter other than the tap0 which when the enterprising consumer does: $ virsh -c qemu:///session nwfilter-binding-create QEMUGuest1-binding.xml Network filter binding on tap0 created from QEMUGuest1-binding.xml $ virsh start QEMUGuest1 Domain QEMUGuest1 started $ virsh nwfilter-binding-list Port Dev Filter ------------------------------------------------------------------ tap0 clean-traffic $ virsh dumpxml QEMUGuest1 <domain type='qemu' id='3'> <name>QEMUGuest1</name> ... <interface type='bridge'> <mac address='52:54:00:f7:d6:f9'/> <source network='default' bridge='virbr0'/> <target dev='tap0'/> <model type='virtio'/> <alias name='net0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </interface> ... $ $ cat QEMUGuest1-binding.xml <filterbinding> <owner> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> </owner> <portdev name='tap0'/> <mac address='52:54:00:f7:d6:f9'/> <filterref filter='clean-traffic'> <parameter name='CTRL_IP_LEARNING' value='dhcp'/> </filterref> </filterbinding> My AVC'S started firing like crazy... John
The nwfilterConnectOpen() method should have failed due to 'driver' being NULL, so the virConnectPtr doesn't exist and so no driver callback points to nwfilterBindingCreateXML.
Regards, Daniel
On Thu, Aug 30, 2018 at 12:50:09PM -0400, John Ferlan wrote:
On 08/30/2018 12:27 PM, Daniel P. Berrangé wrote:
On Thu, Aug 30, 2018 at 11:06:07AM -0400, John Ferlan wrote:
Similar to nwfilterDefineXML, let's be sure the a filter binding creation is not attempted in session mode and generate the proper error message.
Failure to open nwfilter in session mode (nwfilterConnectOpen) fails already, but that doesn't stop the free thinker from using a different connection in order to attempt to attempt to create the binding. Although even doing that would result in a failure:
$ virsh nwfilter-binding-create QEMUGuest1-binding.xml error: Failed to create network filter from QEMUGuest1-binding.xml error: internal error: Could not get access to ACL tech driver 'ebiptables'
$
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index ac3a964388..1ee5162b9a 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -745,6 +745,12 @@ nwfilterBindingCreateXML(virConnectPtr conn,
virCheckFlags(0, NULL);
+ if (!driver->privileged) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't define NWFilter bindings in session mode")); + return NULL; + } + def = virNWFilterBindingDefParseString(xml); if (!def) return NULL;
How do we ever get to this point in a session daemon ?
Like I noted in the commit message an enterprising user...
With no guest running:
$ virsh -c qemu:///session nwfilter-binding-create QEMUGuest1-binding.xml Network filter binding on tap0 created from QEMUGuest1-binding.xml
Oh, i see it is because when using qemu://session, we never actually call the nwfilterConnectOpen method - it is opened implicitly. So Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
https://bugzilla.redhat.com/show_bug.cgi?id=1608275 Instantiation of an nwfilter binding is only allowed when the net->filter is defined for the network; however, the teardown of the binding does not make this check. This leaves open the possibility that the teardown could be called during guest shutdown/teardown in session mode resulting in the following error being logged: error : nwfilterConnectOpen:383 : internal error: unexpected nwfilter URI path '/session', try nwfilter:///system So before going through the teardown processing, let's be sure the network had a filter and then attempt to get a connection. For session mode it's not even possible create an nwfilter binding. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_nwfilter.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c index f39c8a1f9b..e75fb598e8 100644 --- a/src/conf/domain_nwfilter.c +++ b/src/conf/domain_nwfilter.c @@ -149,9 +149,12 @@ virDomainConfNWFilterTeardownImpl(virConnectPtr conn, void virDomainConfNWFilterTeardown(virDomainNetDefPtr net) { - virConnectPtr conn = virGetConnectNWFilter(); + virConnectPtr conn; - if (!conn) + if (!net->filter) + return; + + if (!(conn = virGetConnectNWFilter())) return; virDomainConfNWFilterTeardownImpl(conn, net); @@ -163,14 +166,19 @@ void virDomainConfVMNWFilterTeardown(virDomainObjPtr vm) { size_t i; - virConnectPtr conn = virGetConnectNWFilter(); + virConnectPtr conn = NULL; - if (!conn) - return; + for (i = 0; i < vm->def->nnets; i++) { + virDomainNetDefPtr net = vm->def->nets[i]; + if (!net->filter) + continue; - for (i = 0; i < vm->def->nnets; i++) - virDomainConfNWFilterTeardownImpl(conn, vm->def->nets[i]); + if (!conn && !(conn = virGetConnectNWFilter())) + return; + + virDomainConfNWFilterTeardownImpl(conn, net); + } virObjectUnref(conn); } -- 2.17.1
On Thu, Aug 30, 2018 at 11:06:08AM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1608275
Instantiation of an nwfilter binding is only allowed when the net->filter is defined for the network; however, the teardown of the binding does not make this check. This leaves open the possibility that the teardown could be called during guest shutdown/teardown in session mode resulting in the following error being logged:
error : nwfilterConnectOpen:383 : internal error: unexpected nwfilter URI path '/session', try nwfilter:///system
So before going through the teardown processing, let's be sure the network had a filter and then attempt to get a connection. For session mode it's not even possible create an nwfilter binding.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_nwfilter.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
participants (2)
-
Daniel P. Berrangé -
John Ferlan