[pve-devel] [PATCH perl-rs v2] fix: sdn: fabrics: always add node-ip to all fabric interfaces

[Gabriel Goller <g.goller@proxmox.com>]

OpenFabric can handle completely unnumbered interfaces, so interfaces
which don't have an ip configured. This works great in many setups, but
can cause some intricate ARP issues in some specific setups.

The problem
===========

Consider a setup like this:

   ┌────────┐     ┌────────┐     ┌────────┐
   │ Node1  ├─────┤ Node2  ├─────┤ Node3  │
   │10.0.1.1│     │10.0.1.2│     │10.0.1.3│
   └────────┘     └────────┘     └────────┘

When pinging from Node1 to Node2 and from Node2 to Node3 everything
works. A ping from Node1 to Node3 also works, but ONLY if there has been
a ping between Node2-Node3 and Node2-Node1 in the last 60 seconds. This
is because there is a subtle difference between pinging from Node2 to
Node3 vs an ARP lookup from Node2 to Node3 happening because Node2 wants
to forward a packet from Node1 to Node3.

We can see this difference quite easily by just tcpdumping all the ARP
packages exiting Node2.

Scenario 1: Ping from Node2 to Node3
------------------------------------

A simple lookup in the routing table will tell us that to reach Node3 we
need to go through interface (e.g.) ens20 onlink (so just throw it onto
the link withouth any nexthop check) and use the src address of
10.0.1.2. So ping will create a simple ICMP packet with the destination
address of 10.0.1.3 and the source address of 10.0.1.2. To send the
package the mac address of the destination needs to be figured out, so a
ARP packet is send [0]. The arp_announce sysctl is 0 by default, so we
simply lookup if any local interface has the address 10.0.1.2 (the
source address of the packet we want to send out), if yes, then we
choose that as a source address for the ARP packet as well and send it
out [1]. This will generate an ARP request like the following:

    Request who-has 10.0.1.2 tell 10.0.1.3

We get a nice response:

    Reply 10.0.1.2 is-at bc:24:11:91:a0:49

And send a ping at the specified address, which will work and give us a
result.

The second scenario does not work that well, because the ARP packet is
constructed wrongly.

Scenario 2: Packet forward from Node2 to Node3
----------------------------------------------

In this scenario we ping from Node1 to Node3, which means that Node2
needs to forward our packet. When the packet arrives at Node2, we again
check the routing table, where we see the same entry as before, so
10.0.1.3 is available at (e.g.) ens20 onlink with the src address
10.0.1.2. This is fine, but we still need to do an ARP request to lookup
the mac address of the neighbor which is attached at ens20. So we take
the packet we get from Node1, which has a source address of 10.0.1.1, and
call arp_solicit on it to make an ARP request and find the neighbors mac
address. arp_solicit will take the source address of the packet
(10.0.1.1) and lookup to search it locally. This check fails because
10.0.1.1 is not available locally (there is a direct route to it, but
it's not configured on any local interface (RTN_LOCAL)). arp_solicit
will thus [2] call inet_select_addr, which goes through all the ip
addresses on the current interface (there are none, because this
interface is unnumbered) and then iterate through all the other
interfaces on the host and select the first one with 'scope link'. This
ip will then be selected as the source address for the outgoing ARP
package. Now if we're lucky this is the dummy interface on our node and
we select the correct source address (10.0.1.2) -- but we could also be
unlucky and it selects a completely random address from another
interface e.g. 172.16.0.26. If we're unlucky arp_solicit will send out
the following ARP packet:

    Request who-has 10.0.1.3 tell 172.16.0.26

We will get a correct response but the response will end up on another
interface (because 172.16.0.26 is not on the same interface as
10.0.1.2). This means we will send out these ARP requests repeatedly and
never get an answer, so the ping from Node1 to Node3 will get
"Destination host unreachable errors".

For more information check out the simplified version of the arp_solicit
function below:

The logic is implemented in the arp_solicit function which is shown here
in a simplified form:

    static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
    {
	    __be32 saddr = 0;
	    struct net_device *dev = neigh->dev;
	    __be32 target = *(__be32 *)neigh->primary_key;

	    switch (IN_DEV_ARP_ANNOUNCE(in_dev)) {
	    default:
	    case 0:		/* By default announce any local IP */
		    if (skb && inet_addr_type_dev_table(dev_net(dev), dev,
					      ip_hdr(skb)->saddr) == RTN_LOCAL)
			    saddr = ip_hdr(skb)->saddr;
		    break;
	    case 1:		/* Restrict announcements of saddr in same subnet */
		    if (!skb)
			    break;
		    saddr = ip_hdr(skb)->saddr;
		    if (inet_addr_type_dev_table(dev_net(dev), dev,
						 saddr) == RTN_LOCAL) {
			    /* saddr should be known to target */
			    if (inet_addr_onlink(in_dev, target, saddr))
				    break;
		    }
		    saddr = 0;
		    break;
	    case 2:		/* Avoid secondary IPs, get a primary/preferred one */
		    break;
	    }

	    if (!saddr)
		    saddr = inet_select_addr(dev, target, RT_SCOPE_LINK);

	    arp_send_dst(ARPOP_REQUEST, ETH_P_ARP, target, dev, saddr,
			 dst_hw, dev->dev_addr, NULL, dst);
    }

How to fix this
===============

We could fix this by tweaking the arp_announce option, but we would need
to be careful and only do this on outgoing interfaces. The problem is
here that we can't tell which interfaces will only be outgoing
interfaces.
A much simpler solution would be to just set the ip address of the node
(in our case for Node2 10.0.1.2) on every interfaces which is in the
fabric. In that case inet_select_addr will select the correct address
because it's set on the outgoing interface. This means all the ARP
requests will be sent out with the correct source address.

[0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv4/arp.c#n333
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv4/arp.c#n352
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv4/arp.c#n373

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---

Changelog:
v1:
 * explained the problem in the commit message better, no functional
   change.

 pve-rs/src/bindings/sdn/fabrics.rs | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/pve-rs/src/bindings/sdn/fabrics.rs b/pve-rs/src/bindings/sdn/fabrics.rs
index 587b1d68c8fb..9d5fa6c53d70 100644
--- a/pve-rs/src/bindings/sdn/fabrics.rs
+++ b/pve-rs/src/bindings/sdn/fabrics.rs
@@ -544,12 +544,21 @@ pub mod pve_rs_sdn_fabrics {
                             write!(interfaces, "{interface}")?;
                         }
 
-                        // If not ip is configured, add auto and empty iface to bring interface up
+                        // If no ip is configured, add auto and iface with node ip to bring interface up
+                        // OpenFabric doesn't really need an ip on the interface, but the problem
+                        // is that arp can't tell which source address to use in some cases, so
+                        // it's better if we set the node address on all the fabric interfaces.
                         if let (None, None) = (interface.ip(), interface.ip6()) {
+                            let cidr = Cidr::from(if let Some(ip) = node.ip() {
+                                IpAddr::from(ip)
+                            } else if let Some(ip) = node.ip6() {
+                                IpAddr::from(ip)
+                            } else {
+                                anyhow::bail!("there has to be a ipv4 or ipv6 node address");
+                            });
+                            let interface = render_interface(interface.name(), cidr, false)?;
                             writeln!(interfaces)?;
-                            writeln!(interfaces, "auto {}", interface.name())?;
-                            writeln!(interfaces, "iface {}", interface.name())?;
-                            writeln!(interfaces, "\tip-forward 1")?;
+                            write!(interfaces, "{interface}")?;
                         }
                     }
                 }
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel