[libvirt] [PATCH 1/6] conf: add trustGuestRxFilters attribute to network and domain interface
Laine Stump
laine at laine.org
Thu Sep 25 18:17:06 UTC 2014
On 09/24/2014 07:17 PM, John Ferlan wrote:
>
> On 09/24/2014 05:50 AM, Laine Stump wrote:
>> This new attribute will control whether or not libvirt will pay
>> attention to guest notifications about changes to network device mac
>> addresses and receive filters. The default for this is 'no' (for
>> security reasons). If it is set to 'yes' *and* the specified device
>> model and connection support it (currently only macvtap+virtio) then
>> libvirt will watch for NIC_RX_FILTER_CHANGED events, and when it
>> receives one, it will issue a query-rx-filter command, retrieve the
>> result, and modify the host-side macvtap interface's mac address and
>> unicast/multicast filters accordingly.
>>
>> The functionality behind this attribute will be in a later patch. This
>> patch merely adds the attribute to the top-level of a domain's
>> <interface> as well as to <network> and <portgroup>, and adds
>> documentation and schema/xml2xml tests. Rather than adding even more
>> test files, I've just added the net attribute in various applicable
>> places of existing test files.
>> ---
>> docs/formatdomain.html.in | 38 ++++++++++++++++----
>> docs/formatnetwork.html.in | 28 +++++++++++++--
>> docs/schemas/domaincommon.rng | 5 +++
>> docs/schemas/network.rng | 10 ++++++
>> src/conf/domain_conf.c | 42 ++++++++++++++++++++++
>> src/conf/domain_conf.h | 3 ++
>> src/conf/network_conf.c | 35 ++++++++++++++++++
>> src/conf/network_conf.h | 2 ++
>> src/libvirt_private.syms | 1 +
>> tests/networkxml2xmlin/vepa-net.xml | 4 +--
>> tests/networkxml2xmlout/vepa-net.xml | 4 +--
>> .../qemuxml2argv-net-virtio-network-portgroup.xml | 4 +--
>> 12 files changed, 160 insertions(+), 16 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index eefdd5e..17e180d 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -3343,10 +3343,9 @@
>> <pre>
>> ...
>> <devices>
>> - <interface type='bridge'>
>> - <source bridge='xenbr0'/>
>> - <mac address='00:16:3e:5d:c7:9e'/>
>> - <script path='vif-bridge'/>
>> + <interface type='direct' trustGuestRxFilters='yes'>
>> + <source dev='eth0'/>
>> + <mac address='52:54:00:5d:c7:9e'/>
>> <boot order='1'/>
>> <rom bar='off'/>
>> </interface>
>> @@ -3356,8 +3355,21 @@
>> <p>
>> There are several possibilities for specifying a network
>> interface visible to the guest. Each subsection below provides
>> - more details about common setup options. Additionally,
>> - each <code><interface></code> element has an
>> + more details about common setup options.
>> + </p>
>> + <p>
>> + libvirt allows specifying when the host should trust reports
>> + from the guest of changes to the interface mac address and
>> + receive filters by setting the <code>trustGuestRxFilters</code>
>> + attribute to <code>yes</code><span class="since">Since
>> + 1.2.9</span>. <code>trustGuestRxFilters</code> defaults
>> + to <code>no</code> for security reasons, and support depends on
>> + the guest network device driver as well as the type of
>> + connection on the host - currently it is only supported for the
>> + virtio driver, and for macvtap connections on the host.
> <span class="since">Since 1.2.9</span>), the <code>interface</code>
> element property <code>trustGuestRxFilters</code> provides the
> capability for the host to detect and trust reports from the guest
> regarding changes to the interface mac address and receive filters by
> setting the attribute to <code>yes</yes>. The default setting for the
> attribute is <code>no</code> for security reasons and support depends on
> the guest network device driver as well as the type of connection on the
> host - currently it is only supported for the virtio driver and for
> macvtap connections on the host.
*applause*
Seriously. Can you write all my documentation from now on?
>
>
> [just looks wierd starting with libvirt allows... I also changed the
> "virtio driver, and for" to not have the "," since it's just joining two
> phrases not a list of things...]
>
>> + </p>
>> + <p>
>> + Each <code><interface></code> element has an
>> optional <code><address></code> sub-element that can tie
>> the interface to a particular pci slot, with
>> attribute <code>type='pci'</code>
>> @@ -3589,6 +3601,18 @@
>> being the default mode. The individual modes cause the delivery of
>> packets to behave as follows:
>> </p>
>> + <p>
>> + If the model type is set to <code>virtio</code> and
>> + interface's <code>trustGuestRxFilters</code> attribute is set
>> + to <code>yes</code>, changes made to the interface mac address,
>> + unicast/multicast receive filters, and vlan settings in the
>> + guest will be monitored and propogated to the associated macvtap
> s/propogated/propagated/
>
>> + device on the host. <span class="since">Since
>> + 1.2.9</span>. If <code>trustGuestRxFilters</code> is not set, or
> Instead of "...on the host. Since 1.2.9. If..." - consider "...on the
> host (since 1.2.9). If..."
>
>> + is not supported for the device model in use, an attempted
>> + change to the mac address originating from the guest side will
>> + result in a non-working network connection.
>> + </p>
>>
>> <dl>
>> <dt><code>vepa</code></dt>
>> @@ -3621,7 +3645,7 @@
>> ...
>> <devices>
>> ...
>> - <interface type='direct'>
>> + <interface type='direct' trustGuestRxFilters='no'>
>> <source dev='eth0' mode='vepa'/>
>> </interface>
>> </devices>
>> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
>> index 1a8ad8e..ff73952 100644
>> --- a/docs/formatnetwork.html.in
>> +++ b/docs/formatnetwork.html.in
>> @@ -35,7 +35,7 @@
>> </p>
>>
>> <pre>
>> - <network ipv6='yes'>
>> + <network ipv6='yes' trustGuestRxFilters='no'>
>> <name>default</name>
>> <uuid>3e3fce45-4f53-4fa7-bb32-11f34168b82b</uuid>
>> ...</pre>
>> @@ -60,6 +60,16 @@
>> to have guest-to-guest communications. For further information,
>> see the example below for the example with no gateway addresses.
>> <span class="since">Since 1.0.1</span></dd>
>> + <dt><code>trustGuestRxFilters='yes'</code></dt>
>> + <dd>The optional parameter <code>trustGuestRxFilters</code> can
>> + be used to set that attribute of the same name for each domain
>> + interface connected to this network <span class="since">Since
>> + 1.2.9</span> See
> I think the "Since" needs some parentheses and period before the "See", eg
> (<span class="since">since 1.2.9</span>). See
>
>> + the <a href="formatdomain.html#elementSNICS">Network
>> + interfaces</a> section of the domain XML documentation for
>> + more details. Note that an explicit setting of this attribute
>> + in a portgroup or the individual domain interface will
>> + override the setting in the network.</dd>
>> </dl>
>>
>> <h3><a name="elementsConnect">Connectivity</a></h3>
>> @@ -606,7 +616,7 @@
>> <outbound average='1000' peak='5000' burst='5120'/>
>> </bandwidth>
>> </portgroup></b>
>> - <b><portgroup name='sales'>
>> + <b><portgroup name='sales' trustGuestRxFilters='no'>
>> <virtualport type='802.1Qbh'>
>> <parameters profileid='salestest'/>
>> </virtualport>
>> @@ -626,7 +636,7 @@
>> network can have multiple portgroup elements (and one of those
>> can optionally be designated as the 'default' portgroup for the
>> network), and each portgroup has a name, as well as various
>> - subelements associated with it. The currently supported
>> + attributes and subelements associated with it. The currently supported
>> subelements are <code><bandwidth></code>
>> (described <a href="formatnetwork.html#elementQoS">here</a>)
>> and <code><virtualport></code>
>> @@ -650,6 +660,18 @@
>> considered an error, and will prevent the interface from
>> starting.
>> </p>
>> + <p>
>> + portgroups also support the optional
>> + parameter <code>trustGuestRxFilters</code> which can be used to
>> + set that attribute of the same name for each domain interface
>> + using this portgroup <span class="since">Since 1.2.9</span> See
> Similar issue here w/r/t to Since & See.
>
>> + the <a href="formatdomain.html#elementSNICS">Network
>> + interfaces</a> section of the domain XML documentation for more
>> + details. Note that an explicit setting of this attribute in the
>> + portgroup overrides the network-wide setting, and an explicit
>> + setting in the individual domain interface will override the
>> + setting in the portgroup.
>> + </p>
>>
>> <h5><a name="elementsStaticroute">Static Routes</a></h5>
>> <p>
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index b6b309d..74a6d8c 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -2007,6 +2007,11 @@
>> -->
>> <define name="interface">
>> <element name="interface">
>> + <optional>
>> + <attribute name="trustGuestRxFilters">
>> + <ref name="virYesNo"/>
>> + </attribute>
>> + </optional>
> Shouldn't this go after the end of the <choice>? Since that's how it's
> written out in the xml.
Well, attributes are position-indifferent, so it doesn't really matter.
I had put it up at the top so that it wasn't lost down at the bottom of
all the <choice> sections. But there is something to be said for
consistent positioning, so I'll change it.
>
>> <choice>
>> <group>
>> <attribute name="type">
>> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
>> index dc732b4..4546f80 100644
>> --- a/docs/schemas/network.rng
>> +++ b/docs/schemas/network.rng
>> @@ -24,6 +24,11 @@
>> <ref name="virYesNo"/>
>> </attribute>
>> </optional>
>> + <optional>
>> + <attribute name="trustGuestRxFilters">
>> + <ref name="virYesNo"/>
>> + </attribute>
>> + </optional>
>> <interleave>
>>
>> <!-- The name of the network, used to refer to it through the API
>> @@ -197,6 +202,11 @@
>> <ref name="virYesNo"/>
>> </attribute>
>> </optional>
>> + <optional>
>> + <attribute name="trustGuestRxFilters">
>> + <ref name="virYesNo"/>
>> + </attribute>
>> + </optional>
>> <interleave>
>> <optional>
>> <ref name="virtualPortProfile"/>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 9cc118c..7e0d147 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6756,6 +6756,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
>> char *type = NULL;
>> char *mode = NULL;
>> char *addrtype = NULL;
>> + char *trustGuestRxFilters = NULL;
>>
>> if (VIR_ALLOC(actual) < 0)
>> return -1;
>> @@ -6783,6 +6784,16 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
>> goto error;
>> }
>>
>> + trustGuestRxFilters = virXMLPropString(node, "trustGuestRxFilters");
>> + if (trustGuestRxFilters &&
>> + ((int)(actual->trustGuestRxFilters
>> + = virTristateBoolTypeFromString(trustGuestRxFilters)) <= 0)) {
> No need for the (int) cast.
Yeah, I think I was following the pattern of the setting of def->type
for virDomainNetDef (which is in the struct as "virDomainNetType type;"
rather than "int type; /* enum virDomainNetType */).
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("unknown trustGuestRxFilters value '%s'"),
>> + trustGuestRxFilters);
>> + goto error;
>> + }
>> +
>> virtPortNode = virXPathNode("./virtualport", ctxt);
>> if (virtPortNode) {
>> if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>> @@ -6869,6 +6880,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
>> VIR_FREE(type);
>> VIR_FREE(mode);
>> VIR_FREE(addrtype);
>> + VIR_FREE(trustGuestRxFilters);
>> virDomainActualNetDefFree(actual);
>>
>> ctxt->node = save_ctxt;
>> @@ -6919,6 +6931,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>> char *vhostuser_mode = NULL;
>> char *vhostuser_path = NULL;
>> char *vhostuser_type = NULL;
>> + char *trustGuestRxFilters = NULL;
>> virNWFilterHashTablePtr filterparams = NULL;
>> virDomainActualNetDefPtr actual = NULL;
>> xmlNodePtr oldnode = ctxt->node;
>> @@ -6940,6 +6953,16 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>> def->type = VIR_DOMAIN_NET_TYPE_USER;
>> }
>>
>> + trustGuestRxFilters = virXMLPropString(node, "trustGuestRxFilters");
>> + if (trustGuestRxFilters &&
>> + ((int)(def->trustGuestRxFilters
>> + = virTristateBoolTypeFromString(trustGuestRxFilters)) <= 0)) {
> No need for the (int) cast.
>
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("unknown trustGuestRxFilters value '%s'"),
>> + trustGuestRxFilters);
>> + goto error;
>> + }
>> +
>> cur = node->children;
>> while (cur != NULL) {
>> if (cur->type == XML_ELEMENT_NODE) {
>> @@ -7469,6 +7492,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>> VIR_FREE(mode);
>> VIR_FREE(linkstate);
>> VIR_FREE(addrtype);
>> + VIR_FREE(trustGuestRxFilters);
>> virNWFilterHashTableFree(filterparams);
>>
>> return def;
>> @@ -16491,6 +16515,9 @@ virDomainActualNetDefFormat(virBufferPtr buf,
>> if (hostdef && hostdef->managed)
>> virBufferAddLit(buf, " managed='yes'");
>> }
>> + if (def->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT)
>> + virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
>> + virTristateBoolTypeToString(def->trustGuestRxFilters));
> Showoff - although other places that use the tristate logic just use "if
> (def->trustGuestRxFilters)" (since BOOL_ABSENT == 0)
If that's already commonly done and expected, then I can change my code
to do the same.
>
>> virBufferAddLit(buf, ">\n");
>>
>> virBufferAdjustIndent(buf, 2);
>> @@ -16575,6 +16602,9 @@ virDomainNetDefFormat(virBufferPtr buf,
>> virBufferAsprintf(buf, "<interface type='%s'", typeStr);
>> if (hostdef && hostdef->managed)
>> virBufferAddLit(buf, " managed='yes'");
>> + if (def->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT)
>> + virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
>> + virTristateBoolTypeToString(def->trustGuestRxFilters));
> Similarly here...
>
> Also I noted that the domaincommon.rng file has the
> "trustGuestRxFilters" defined first followed by type, managed, etc.,
> while this code has it the other way (eg, last). I thought that caused
> issues for some test, but I guess it didn't. Maybe we just got lucky.
That's only problematic for *elements* that are in different order in
RNG vs. Format, and then can be solved by putting <interleave> ...
</interleave> around all of the elements involved.
(there is another potential issue, if the test case datafile has
elements or attributes in a different order than they are emitted in the
Format, the test case will fail unless there is a separate "out" file
that has everything in the correct order.)
>
>> virBufferAddLit(buf, ">\n");
>>
>> virBufferAdjustIndent(buf, 2);
>> @@ -19976,6 +20006,18 @@ virDomainNetGetActualVlan(virDomainNetDefPtr iface)
>> return NULL;
>> }
>>
>> +
>> +bool
>> +virDomainNetGetActualTrustGuestRxFilters(virDomainNetDefPtr iface)
>> +{
>> + if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
>> + iface->data.network.actual)
>> + return (iface->data.network.actual->trustGuestRxFilters
>> + == VIR_TRISTATE_BOOL_YES);
>> + return iface->trustGuestRxFilters == VIR_TRISTATE_BOOL_YES;
>> +}
>> +
>> +
>> /* Return listens[i] from the appropriate union for the graphics
>> * type, or NULL if this is an unsuitable type, or the index is out of
>> * bounds. If force0 is TRUE, i == 0, and there is no listen array,
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index d97b1f8..2d0cb06 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -880,6 +880,7 @@ struct _virDomainActualNetDef {
>> virNetDevVPortProfilePtr virtPortProfile;
>> virNetDevBandwidthPtr bandwidth;
>> virNetDevVlan vlan;
>> + int trustGuestRxFilters; /* enum virTristateBool */
>> unsigned int class_id; /* class ID for bandwidth 'floor' */
>> };
>>
>> @@ -954,6 +955,7 @@ struct _virDomainNetDef {
>> virNWFilterHashTablePtr filterparams;
>> virNetDevBandwidthPtr bandwidth;
>> virNetDevVlan vlan;
>> + int trustGuestRxFilters; /* enum virTristateBool */
>> int linkstate;
>> };
>>
>> @@ -2471,6 +2473,7 @@ virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface);
>> virNetDevBandwidthPtr
>> virDomainNetGetActualBandwidth(virDomainNetDefPtr iface);
>> virNetDevVlanPtr virDomainNetGetActualVlan(virDomainNetDefPtr iface);
>> +bool virDomainNetGetActualTrustGuestRxFilters(virDomainNetDefPtr iface);
>>
>> int virDomainControllerInsert(virDomainDefPtr def,
>> virDomainControllerDefPtr controller)
>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>> index 892bd8a..63b5917 100644
>> --- a/src/conf/network_conf.c
>> +++ b/src/conf/network_conf.c
>> @@ -1615,6 +1615,7 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def,
>> xmlNodePtr vlanNode;
>> xmlNodePtr bandwidth_node;
>> char *isDefault = NULL;
>> + char *trustGuestRxFilters = NULL;
>>
>> int result = -1;
>>
>> @@ -1632,6 +1633,18 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def,
>> isDefault = virXPathString("string(./@default)", ctxt);
>> def->isDefault = isDefault && STRCASEEQ(isDefault, "yes");
>>
>> + trustGuestRxFilters
>> + = virXPathString("string(./@trustGuestRxFilters)", ctxt);
>> + if (trustGuestRxFilters) {
>> + if ((def->trustGuestRxFilters
>> + = virTristateBoolTypeFromString(trustGuestRxFilters)) <= 0) {
> You could combine into one if like was done in domain_conf.
You mean have a single utility function used in all three places? The
problem with that is that the error message is slightly different to
point out the location of the problematic attribute in the XML.
>
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("Invalid trustGuestRxFilters setting '%s' "
>> + "in portgroup"), trustGuestRxFilters);
>> + goto cleanup;
>> + }
>> + }
>> +
>> virtPortNode = virXPathNode("./virtualport", ctxt);
>> if (virtPortNode &&
>> (!(def->virtPortProfile = virNetDevVPortProfileParse(virtPortNode, 0)))) {
>> @@ -1654,6 +1667,7 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def,
>> virPortGroupDefClear(def);
>> }
>> VIR_FREE(isDefault);
>> + VIR_FREE(trustGuestRxFilters);
>>
>> ctxt->node = save;
>> return result;
>> @@ -2013,6 +2027,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>> xmlNodePtr virtPortNode = NULL;
>> xmlNodePtr forwardNode = NULL;
>> char *ipv6nogwStr = NULL;
>> + char *trustGuestRxFilters = NULL;
>> xmlNodePtr save = ctxt->node;
>> xmlNodePtr bandwidthNode = NULL;
>> xmlNodePtr vlanNode;
>> @@ -2061,6 +2076,20 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>> VIR_FREE(ipv6nogwStr);
>> }
>>
>> + trustGuestRxFilters
>> + = virXPathString("string(./@trustGuestRxFilters)", ctxt);
>> + if (trustGuestRxFilters) {
>> + if ((def->trustGuestRxFilters
>> + = virTristateBoolTypeFromString(trustGuestRxFilters)) <= 0) {
> Ditto
>
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("Invalid trustGuestRxFilters setting '%s' "
>> + "in network '%s'"),
>> + trustGuestRxFilters, def->name);
>> + goto error;
> ^^^ Memleak ...
Oops.
>
>> + }
>> + VIR_FREE(trustGuestRxFilters);
> Memory leak if the goto error occurs. Move VIR_FREE() to error:
Actually you can't *move* it, because in this function, the stuff at
error: isn't executed in the case of success. It could benefit from
being updated to use the currently accepted "do *all* cleanup at
"cleanup:" and make sure all exits from the function go through there"
form. That can/should be a separate patch though, so I'll just duplicate
the VIR_FREE() at error: for now.
>
>> + }
>> +
>> /* Parse network domain information */
>> def->domain = virXPathString("string(./domain[1]/@name)", ctxt);
>>
>> @@ -2597,6 +2626,9 @@ virPortGroupDefFormat(virBufferPtr buf,
>> if (def->isDefault) {
>> virBufferAddLit(buf, " default='yes'");
>> }
>> + if (def->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT)
>> + virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
>> + virTristateBoolTypeToString(def->trustGuestRxFilters));
> Similar to domain_conf - the BOOL_ABSENT isn't necessary
>
>> virBufferAddLit(buf, ">\n");
>> virBufferAdjustIndent(buf, 2);
>> if (virNetDevVlanFormat(&def->vlan, buf) < 0)
>> @@ -2675,6 +2707,9 @@ virNetworkDefFormatBuf(virBufferPtr buf,
>> }
>> if (def->ipv6nogw)
>> virBufferAddLit(buf, " ipv6='yes'");
>> + if (def->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT)
>> + virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
>> + virTristateBoolTypeToString(def->trustGuestRxFilters));
> ditto
>
>
> Nothing major - just some minor cleanups and it's an ACK
>
I've made all the suggested changes. Since the freeze has started, I
won't be pushing until after 1.2.9 release anyway, so I'll repost the
entire series later.
More information about the libvir-list
mailing list