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@redhat.com>
---
src/network/network_nftables.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
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.1On 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@redhat.com>
> ---
> 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@redhat.com>
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 :|
On 6/7/24 4:44 PM, Daniel P. Berrangé wrote: > 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@redhat.com> >> --- >> 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. Interesting point. > So > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > but would be good if you can add more test coverage in a > followup too. Sure. I'll also see if there are some other options that are never included and add tests for anything else that I (don't) find. Thanks!
© 2016 - 2024 Red Hat, Inc.