[libvirt] [PATCH 0/2] Fix 2 small LXC network interface bugs

[Laine Stump]

(Patch 1 fixes a recently filed BZ that it just happened was easy for
me to test, because I had encountered the same problem awhile back)

Laine Stump (2):
lxc: stop incorrectly validating interface type
lxc: check actual type of interface not config type

src/lxc/lxc_controller.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

[Laine Stump]

Commit 017dfa27d changed a few switch statements in the LXC code to
have all possible enum values, and in the process changed the switch
statement in virLXCControllerGetNICIndexes() such that it returned
error status for any interfaces that weren't implemented with a veth
pair when it should have just been ignoring those interfaces.

Since the interface type will have already been validated before
reaching this function, we shouldn't be doing any validation at all -
just add the ifindex of the parent veth if its a veth pair, and ignore
it otherwise.

Resolves: https://bugzilla.redhat.com/1656463
Signed-off-by: Laine Stump <***@laine.org>
---
src/lxc/lxc_controller.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index e853d02d65..1c20f451af 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -364,6 +364,16 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
size_t i;
int ret = -1;

+ /* Gather the ifindexes of the "parent" veths for all interfaces
+ * implemented with a veth pair. These will be used when calling
+ * virCgroupNewMachine (and eventually the dbus method
+ * CreateMachineWithNetwork). ifindexes for the child veths, and
+ * for macvlan interfaces, *should not* be in this list, as they
+ * will be moved into the container. Only the interfaces that will
+ * remain outside the container, but are used for communication
+ * with the container, should be added to the list.
+ */
+
VIR_DEBUG("Getting nic indexes");
for (i = 0; i < ctrl->def->nnets; i++) {
int nicindex = -1;
@@ -394,14 +404,9 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
case VIR_DOMAIN_NET_TYPE_INTERNAL:
case VIR_DOMAIN_NET_TYPE_DIRECT:
case VIR_DOMAIN_NET_TYPE_HOSTDEV:
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Unsupported net type %s"),
- virDomainNetTypeToString(ctrl->def->nets[i]->type));
- goto cleanup;
case VIR_DOMAIN_NET_TYPE_LAST:
default:
- virReportEnumRangeError(virDomainNetType, ctrl->def->nets[i]->type);
- goto cleanup;
+ break;
}
}

[Daniel P. Berrangé]

Post by Laine Stump
Commit 017dfa27d changed a few switch statements in the LXC code to
have all possible enum values, and in the process changed the switch
statement in virLXCControllerGetNICIndexes() such that it returned
error status for any interfaces that weren't implemented with a veth
pair when it should have just been ignoring those interfaces.
Since the interface type will have already been validated before
reaching this function, we shouldn't be doing any validation at all -
just add the ifindex of the parent veth if its a veth pair, and ignore
it otherwise.
Resolves: https://bugzilla.redhat.com/1656463
---
src/lxc/lxc_controller.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index e853d02d65..1c20f451af 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -364,6 +364,16 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
size_t i;
int ret = -1;
+ /* Gather the ifindexes of the "parent" veths for all interfaces
+ * implemented with a veth pair. These will be used when calling
+ * virCgroupNewMachine (and eventually the dbus method
+ * CreateMachineWithNetwork). ifindexes for the child veths, and
+ * for macvlan interfaces, *should not* be in this list, as they
+ * will be moved into the container. Only the interfaces that will
+ * remain outside the container, but are used for communication
+ * with the container, should be added to the list.
+ */
+
VIR_DEBUG("Getting nic indexes");
for (i = 0; i < ctrl->def->nnets; i++) {
int nicindex = -1;
@@ -394,14 +404,9 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Unsupported net type %s"),
- virDomainNetTypeToString(ctrl->def->nets[i]->type));
- goto cleanup;
- virReportEnumRangeError(virDomainNetType, ctrl->def->nets[i]->type);
- goto cleanup;
+ break;
}

This is removing too much. Only ethernet, bridge, network & direct are going
to work with LXC. All others should always result in an error message here,
and absolutely the LAST/default case must always report enum error.


Regards,
Daniel

[Laine Stump]

Post by Daniel P. Berrangé

Post by Laine Stump
Commit 017dfa27d changed a few switch statements in the LXC code to
have all possible enum values, and in the process changed the switch
statement in virLXCControllerGetNICIndexes() such that it returned
error status for any interfaces that weren't implemented with a veth
pair when it should have just been ignoring those interfaces.
Since the interface type will have already been validated before
reaching this function, we shouldn't be doing any validation at all -
just add the ifindex of the parent veth if its a veth pair, and ignore
it otherwise.
Resolves: https://bugzilla.redhat.com/1656463
---
src/lxc/lxc_controller.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index e853d02d65..1c20f451af 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -364,6 +364,16 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
size_t i;
int ret = -1;
+ /* Gather the ifindexes of the "parent" veths for all interfaces
+ * implemented with a veth pair. These will be used when calling
+ * virCgroupNewMachine (and eventually the dbus method
+ * CreateMachineWithNetwork). ifindexes for the child veths, and
+ * for macvlan interfaces, *should not* be in this list, as they
+ * will be moved into the container. Only the interfaces that will
+ * remain outside the container, but are used for communication
+ * with the container, should be added to the list.
+ */
+
VIR_DEBUG("Getting nic indexes");
for (i = 0; i < ctrl->def->nnets; i++) {
int nicindex = -1;
@@ -394,14 +404,9 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Unsupported net type %s"),
- virDomainNetTypeToString(ctrl->def->nets[i]->type));
- goto cleanup;
- virReportEnumRangeError(virDomainNetType, ctrl->def->nets[i]->type);
- goto cleanup;
+ break;
}

This is removing too much. Only ethernet, bridge, network & direct are going
to work with LXC. All others should always result in an error message here,
and absolutely the LAST/default case must always report enum error.

I had originally done what you suggest, then went back and changed it in
order to not conflict with my commit comment saying that validation
shouldn't even be done in this function, since the value had already
been validated and doing it again would just be adding bulk to the code
for no reason.


However, now I see that my belief that the interface configs had been
validated is a bit "off". The config might have been validated by
libvirt, but lxc_controller.c code runs in a different process, so
"outside forces" could have changed things during the transition. And my
assumption of prior validation had been reinforced by seeing that the
function called directly before virLXCControllerGetNICIndexes() was
literally called virLXCControllerValidateNICs(), but when I actually
look at that function I see that the only thing it validates is that the
number of interfaces on the commandline matches the number of interfaces
in the config, which doesn't do us much good.


So okay, I'll change the patch to just make TYPE_DIRECT a NOP and leave
in the validation. (somebody with more drive an ambition might instead
expand *ValidateNICs to do a more complete job.)

[Daniel P. Berrangé]

Post by Laine Stump

Post by Daniel P. Berrangé

Post by Laine Stump
Commit 017dfa27d changed a few switch statements in the LXC code to
have all possible enum values, and in the process changed the switch
statement in virLXCControllerGetNICIndexes() such that it returned
error status for any interfaces that weren't implemented with a veth
pair when it should have just been ignoring those interfaces.
Since the interface type will have already been validated before
reaching this function, we shouldn't be doing any validation at all -
just add the ifindex of the parent veth if its a veth pair, and ignore
it otherwise.
Resolves: https://bugzilla.redhat.com/1656463
---
src/lxc/lxc_controller.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index e853d02d65..1c20f451af 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -364,6 +364,16 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
size_t i;
int ret = -1;
+ /* Gather the ifindexes of the "parent" veths for all interfaces
+ * implemented with a veth pair. These will be used when calling
+ * virCgroupNewMachine (and eventually the dbus method
+ * CreateMachineWithNetwork). ifindexes for the child veths, and
+ * for macvlan interfaces, *should not* be in this list, as they
+ * will be moved into the container. Only the interfaces that will
+ * remain outside the container, but are used for communication
+ * with the container, should be added to the list.
+ */
+
VIR_DEBUG("Getting nic indexes");
for (i = 0; i < ctrl->def->nnets; i++) {
int nicindex = -1;
@@ -394,14 +404,9 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Unsupported net type %s"),
- virDomainNetTypeToString(ctrl->def->nets[i]->type));
- goto cleanup;
- virReportEnumRangeError(virDomainNetType, ctrl->def->nets[i]->type);
- goto cleanup;
+ break;
}

This is removing too much. Only ethernet, bridge, network & direct are going
to work with LXC. All others should always result in an error message here,
and absolutely the LAST/default case must always report enum error.

I had originally done what you suggest, then went back and changed it in
order to not conflict with my commit comment saying that validation
shouldn't even be done in this function, since the value had already
been validated and doing it again would just be adding bulk to the code
for no reason.

My preference is always to be explicit with the validation in a switch()
unless something earlier *in the same function scope* has already done
validation. IOW don't rely on the caller having previously called something
else to do validation, as that's fragile when code is refactored.

Regards,
Daniel

[Laine Stump]

virLXCControllerGetNICIndexes() was deciding whether or not to add the
ifindex for an interface's ifname to the list of ifindexes sent to
CreateMachineWithNetwork based on the interface type stored in the
config. This would be incorrect in the case of <interface
type='network'> where the network was giving out macvlan interfaces
tied to a physical device (i.e. when the actual interface type was
"direct").

Instead of checking the setting of "net->type", we should be checking
the setting of virDomainNetGetActualType(net).

I don't think this caused any actual misbehavior, it was just
technically wrong.

Signed-off-by: Laine Stump <***@laine.org>
---
src/lxc/lxc_controller.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 1c20f451af..dad6abed3b 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -377,7 +377,8 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
VIR_DEBUG("Getting nic indexes");
for (i = 0; i < ctrl->def->nnets; i++) {
int nicindex = -1;
- switch (ctrl->def->nets[i]->type) {
+
+ switch (virDomainNetGetActualType(ctrl->def->nets[i])) {
case VIR_DOMAIN_NET_TYPE_BRIDGE:
case VIR_DOMAIN_NET_TYPE_NETWORK:
case VIR_DOMAIN_NET_TYPE_ETHERNET:

[Daniel P. Berrangé]

Post by Laine Stump
virLXCControllerGetNICIndexes() was deciding whether or not to add the
ifindex for an interface's ifname to the list of ifindexes sent to
CreateMachineWithNetwork based on the interface type stored in the
config. This would be incorrect in the case of <interface
type='network'> where the network was giving out macvlan interfaces
tied to a physical device (i.e. when the actual interface type was
"direct").
Instead of checking the setting of "net->type", we should be checking
the setting of virDomainNetGetActualType(net).
I don't think this caused any actual misbehavior, it was just
technically wrong.
---
src/lxc/lxc_controller.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <***@redhat.com>


Regards,
Daniel