On Fri, Jun 07, 2024 at 01:33:30PM -0400, Laine Stump wrote:
> A user reported that if they set <forward mode='nat|route' dev='blah'>
> starting the network would fail if the device 'blah' didn't already
> exist.
>
> This is caused by using "iif" and "oif" in nftables rules to check for
> the forwarding device - these two commands work by saving the named
> interface's ifindex (an unsigned integer) when the rule is added, and
> comparing it to the ifindex associated with the packet's path at
> runtime. This works great if the interface both 1) exists when the
> rule is added, and 2) is never deleted and re-created after the rule
> is added (since it would end up with a different ifindex).
>
> When checking for the network's bridge device, it is okay for us to
> use "iif" and "oif", because the bridge device is created before the
> firewall rules are added, and will continue to exist until just after
> the firewall rules are deleted when the network is shutdown.
>
> But since the forward device might be deleted/re-added during the
> lifetime of the network's firewall rules, we must instead us "oifname"
> and "iifname" - these are much less efficient than "Xif" because they
> do a string compare of the interface's name rather than just comparing
> two integers (ifindex), but they don't require the interface to exist
> when the rule is added, and they can properly cope with the named
> interface being deleted and re-added later.
>
> Fixes: a4f38f6ffe6a9edc001d18890ccfc3f38e72fb94
> Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
> ---
> src/network/network_nftables.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
Surprised that tests/networkxml2firewalldata didn't need an
update - guess we've got missing coverage of this feature.
So
Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
but would be good if you can add more test coverage in a
followup too.
>
> diff --git a/src/network/network_nftables.c b/src/network/network_nftables.c
> index 59ab231a06..268d1f12ca 100644
> --- a/src/network/network_nftables.c
> +++ b/src/network/network_nftables.c
> @@ -362,7 +362,7 @@ nftablesAddForwardAllowOut(virFirewall *fw,
> "iif", iface, NULL);
>
> if (physdev && physdev[0])
> - virFirewallCmdAddArgList(fw, fwCmd, "oif", physdev, NULL);
> + virFirewallCmdAddArgList(fw, fwCmd, "oifname", physdev, NULL);
>
> virFirewallCmdAddArgList(fw, fwCmd, "counter", "accept", NULL);
>
> @@ -398,7 +398,7 @@ nftablesAddForwardAllowRelatedIn(virFirewall *fw,
> VIR_NFTABLES_FWD_IN_CHAIN, NULL);
>
> if (physdev && physdev[0])
> - virFirewallCmdAddArgList(fw, fwCmd, "iif", physdev, NULL);
> + virFirewallCmdAddArgList(fw, fwCmd, "iifname", physdev, NULL);
>
> virFirewallCmdAddArgList(fw, fwCmd, "oif", iface,
> layerStr, "daddr", networkstr,
> @@ -437,7 +437,7 @@ nftablesAddForwardAllowIn(virFirewall *fw,
> layerStr, "daddr", networkstr, NULL);
>
> if (physdev && physdev[0])
> - virFirewallCmdAddArgList(fw, fwCmd, "iif", physdev, NULL);
> + virFirewallCmdAddArgList(fw, fwCmd, "iifname", physdev, NULL);
>
> virFirewallCmdAddArgList(fw, fwCmd, "oif", iface,
> "counter", "accept", NULL);
> @@ -566,7 +566,7 @@ nftablesAddForwardMasquerade(virFirewall *fw,
> layerStr, "daddr", "!=", networkstr, NULL);
>
> if (physdev && physdev[0])
> - virFirewallCmdAddArgList(fw, fwCmd, "oif", physdev, NULL);
> + virFirewallCmdAddArgList(fw, fwCmd, "oifname", physdev, NULL);
>
> if (protocol && protocol[0]) {
> if (port->start == 0 && port->end == 0) {
> @@ -634,7 +634,7 @@ nftablesAddDontMasquerade(virFirewall *fw,
> VIR_NFTABLES_NAT_POSTROUTE_CHAIN, NULL);
>
> if (physdev && physdev[0])
> - virFirewallCmdAddArgList(fw, fwCmd, "oif", physdev, NULL);
> + virFirewallCmdAddArgList(fw, fwCmd, "oifname", physdev, NULL);
>
> virFirewallCmdAddArgList(fw, fwCmd,
> layerStr, "saddr", networkstr,
> --
> 2.45.1
>
With 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 :|
