10:46 a.m.
On Tue, Sep 17, 2024 at 10:27:40AM -0400, Laine Stump wrote:
On 9/16/24 4:06 AM, Martin Kletzander wrote:
> On Mon, Sep 16, 2024 at 09:49:44AM +0200, Martin Kletzander wrote:
>> On Thu, Sep 05, 2024 at 01:07:59PM -0400, Laine Stump wrote:
>>> When a bridge device for a virtual network had been placed in a
>>> firewalld zone while starting the network, then even after the network
>>> is shut down and the bridge device is deleted, its name will show up in
>>> the list of interfaces for whichever zone it had been in.
>>>
>>> Usually this isn't a problem, but in the case of forward
mode='open',
>>> someone might start the network once with a zone specified, then
>>> shut down the network, remove vthe zone from its config, and start it
>>> again; in this case the bridge device would come up using the zone
>>> from the previous time it was started.
>>>
>>> The solution to this is to remove the interface from whatever zone it
>>> is in as the network is being shut down. There is no downside to doing
>>> this, since the device is going to be deleted anyway. Note that
>>> forward mode='bridge' uses a bridge device that was created outside
of
>>> libvirt, and libvirt won't be deleting that bridge, so we take care to
>>> not unset the zone in that case.
>>>
>>> Signed-off-by: Laine Stump <laine(a)redhat.com>
>>> ---
>>> src/libvirt_private.syms | 1 +
>>> src/network/bridge_driver.c | 4 ++++
>>> src/network/bridge_driver_linux.c | 14 ++++++++++++++
>>> src/network/bridge_driver_nop.c | 6 ++++++
>>> src/network/bridge_driver_platform.h | 2 ++
>>> src/util/virfirewalld.c | 23 +++++++++++++++++++++++
>>> src/util/virfirewalld.h | 2 ++
>>> 7 files changed, 52 insertions(+)
>>>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index af40e5dca3..f15d16c292 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -2451,6 +2451,7 @@ virFirewallDGetPolicies;
>>> virFirewallDGetVersion;
>>> virFirewallDGetZones;
>>> virFirewallDInterfaceSetZone;
>>> +virFirewallDInterfaceUnsetZone;
>>> virFirewallDIsRegistered;
>>> virFirewallDPolicyExists;
>>> virFirewallDSynchronize;
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index 3504d512a0..e457c3bf5e 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -2085,6 +2085,8 @@
>>> networkStartNetworkVirtual(virNetworkDriverState *driver,
>>> def->forward.type != VIR_NETWORK_FORWARD_OPEN)
>>> networkRemoveFirewallRules(obj);
>>>
>>> + networkUnsetBridgeZone(def);
>>> +
>>> virNetworkObjUnrefMacMap(obj);
>>>
>>> ignore_value(virNetDevBridgeDelete(def->bridge));
>>> @@ -2123,6 +2125,8 @@ networkShutdownNetworkVirtual(virNetworkObj *obj)
>>> if (def->forward.type != VIR_NETWORK_FORWARD_OPEN)
>>> networkRemoveFirewallRules(obj);
>>>
>>> + networkUnsetBridgeZone(def);
>>> +
>>> ignore_value(virNetDevBridgeDelete(def->bridge));
>>>
>>> /* See if its still alive and really really kill it */
>>> diff --git a/src/network/bridge_driver_linux.c b/src/network/
>>> bridge_driver_linux.c
>>> index af758d4f3d..3b3608c085 100644
>>> --- a/src/network/bridge_driver_linux.c
>>> +++ b/src/network/bridge_driver_linux.c
>>> @@ -392,6 +392,20 @@ networkSetBridgeZone(virNetworkDef *def)
>>> }
>>>
>>>
>>> +void
>>> +networkUnsetBridgeZone(virNetworkDef *def)
>>> +{
>>> + /* If there is a libvirt-managed bridge device remove it from any
>>> + * zone it had been placed in as a part of deleting the bridge.
>>> + * DO NOT CALL THIS FOR 'bridge' forward mode, since that
>>> + * bridge is not managed by libvirt.
>>> + */
>>> + if (def->bridge && def->forward.type !=
VIR_NETWORK_FORWARD_BRIDGE
>>> + && virFirewallDIsRegistered() == 0) {
>>
>> This only unsets the zone if def->bridge is set, but for various
>> scenarios it might not be set by default. For example if
>> def->forward.type != VIR_NETWORK_FORWARD_OPEN and def->bridge is NULL we
>> still set the zone to "libvirt-routed" for VIR_NETWORK_FORWARD_ROUTE
and
>> "libvirt" for all the others.
By definition, def->bridge will be non-NULL if any of those forward
modes is set. (this is verified in networkValidate() with the call to
networkBridgeNameValidate()) Also it would be impossible to get far
enough though networkStartNetworkVirtual() (which is the way networks
with all of these modes are started) to set the zone if def->bridge was
NULL - it would fail right at the top of the function). Finally, even if
it was possible to get that far, it is the bridge device itself's name
that is being added to the given zone, so "setting the zone" when the
device name is NULL wouldn't do anything (other than generate an error).
I am quite sure I was not under any influence when writing my review but
reading it now after myself I feel like I had to invent some new really
heavy stuff. Sorry, I have no idea what I possibly could've meant, how
could we set a zone for a bridge/interface for which we do not have the
name? Anyway, I'll postpone the questioning of my existence to a later
date and for now just add a
Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com>
>> At least that's how it looks when reading
>> networkSetBridgeZone() after patch 3/5 or networkAddFirewallRules() in
>> current master.
>>
>
> Having said that it would still be beneficial to do some extra clean up
> to remove the zone if, for example, the bridge disappeared while
> virtnetworkd was not running (shameless plug of [1] O:-) )
Isn't it enough to put a call to networkUnsetBridgeZone() in
networkShutdownNetworkVirtual()? If so, then you've already done that
work for me. :-)
Well, that was meant mostly as a hint because even though anyone
could've reviewed that series I was secretly hoping to get a review from
our networking guru. And it worked! ;) Thanks a lot for the review.
>
> Martin
>
> [1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/
> thread/D7F6OYTG2N4ZVNDILPJFOQPMYBZ43LYM/
>
>>> + virFirewallDInterfaceUnsetZone(def->bridge);
>>> + }
>>> +}
>>> +
>>> int
>>> networkAddFirewallRules(virNetworkDef *def,
>>> virFirewallBackend firewallBackend,
>>> diff --git a/src/network/bridge_driver_nop.c b/src/network/
>>> bridge_driver_nop.c
>>> index 20c7a2a595..180ff30134 100644
>>> --- a/src/network/bridge_driver_nop.c
>>> +++ b/src/network/bridge_driver_nop.c
>>> @@ -51,6 +51,12 @@ networkSetBridgeZone(virNetworkDef *def)
>>> }
>>>
>>>
>>> +void
>>> +networkUnsetBridgeZone(virNetworkDef *def)
>>> +{
>>> +}
>>> +
>>> +
>>> int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
>>> virFirewallBackend firewallBackend,
>>> virFirewall **fwRemoval G_GNUC_UNUSED)
>>> diff --git a/src/network/bridge_driver_platform.h b/src/network/
>>> bridge_driver_platform.h
>>> index 02abdc197f..a0291532a1 100644
>>> --- a/src/network/bridge_driver_platform.h
>>> +++ b/src/network/bridge_driver_platform.h
>>> @@ -38,4 +38,6 @@ int networkAddFirewallRules(virNetworkDef *def,
>>> virFirewallBackend firewallBackend,
>>> virFirewall **fwRemoval);
>>>
>>> +void networkUnsetBridgeZone(virNetworkDef *def);
>>> +
>>> void networkRemoveFirewallRules(virNetworkObj *obj);
>>> diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c
>>> index 827e201dbb..4aec33ac45 100644
>>> --- a/src/util/virfirewalld.c
>>> +++ b/src/util/virfirewalld.c
>>> @@ -449,6 +449,29 @@ virFirewallDInterfaceSetZone(const char *iface,
>>> }
>>>
>>>
>>> +int
>>> +virFirewallDInterfaceUnsetZone(const char *iface)
>>> +{
>>> + GDBusConnection *sysbus = virGDBusGetSystemBus();
>>> + g_autoptr(GVariant) message = NULL;
>>> +
>>> + if (!sysbus)
>>> + return -1;
>>> +
>>> + message = g_variant_new("(ss)", "", iface);
>>> +
>>> + return virGDBusCallMethod(sysbus,
>>> + NULL,
>>> + NULL,
>>> + NULL,
>>> + VIR_FIREWALL_FIREWALLD_SERVICE,
>>> + "/org/fedoraproject/FirewallD1",
>>> + "org.fedoraproject.FirewallD1.zone",
>>> + "removeInterface",
>>> + message);
>>> +}
>>> +
>>> +
>>> void
>>> virFirewallDSynchronize(void)
>>> {
>>> diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h
>>> index 0e94d3507b..0dbe66d435 100644
>>> --- a/src/util/virfirewalld.h
>>> +++ b/src/util/virfirewalld.h
>>> @@ -46,4 +46,6 @@ int virFirewallDApplyRule(virFirewallLayer layer,
>>> int virFirewallDInterfaceSetZone(const char *iface,
>>> const char *zone);
>>>
>>> +int virFirewallDInterfaceUnsetZone(const char *iface);
>>> +
>>> void virFirewallDSynchronize(void);
>>> --
>>> 2.46.0
>>>
>
>