vxlan: Various fixes

Default remotes are stored as FDB entries with an Ethernet address of 00:00:00:00:00:00. When a request is made to change a remote address of a VXLAN device, vxlan_changelink() first deletes the existing default remote, and then creates a new FDB entry.

This works well as long as the list of default remotes matches exactly the configuration of a VXLAN remote address. Thus when the VXLAN device has a remote of X, there should be exactly one default remote FDB entry X. If the VXLAN device has no remote address, there should be no such entry.

Besides using "ip link set", it is possible to manipulate the list of default remotes by using the "bridge fdb". It is therefore easy to break the above condition. Under such circumstances, the __vxlan_fdb_delete() call doesn't delete the FDB entry itself, but just one remote. The following vxlan_fdb_create() then creates a new FDB entry, leading to a situation where two entries exist for the address 00:00:00:00:00:00, each with a different subset of default remotes.

An even more obvious breakage rooted in the same cause can be observed when a remote address is configured for a VXLAN device that did not have one before. In that case vxlan_changelink() doesn't remove any remote, and just creates a new FDB entry for the new address:

$ ip link add name vx up type vxlan id 2000 dstport 4789
$ bridge fdb ap dev vx 00:00:00:00:00:00 dst 192.0.2.20 self permanent
$ bridge fdb ap dev vx 00:00:00:00:00:00 dst 192.0.2.30 self permanent
$ ip link set dev vx type vxlan remote 192.0.2.30
$ bridge fdb sh dev vx | grep 00:00:00:00:00:00
00:00:00:00:00:00 dst 192.0.2.30 self permanent <- new entry, 1 rdst
00:00:00:00:00:00 dst 192.0.2.20 self permanent <- orig. entry, 2 rdsts
00:00:00:00:00:00 dst 192.0.2.30 self permanent

To fix this, instead of calling vxlan_fdb_create() directly, defer to vxlan_fdb_update(). That has logic to handle the duplicates properly. Additionally, it also handles notifications, so drop that call from changelink as well.

Fixes 0241b836732f ("vxlan: fix default fdb entry netlink notify ordering during netdev create")

Signed-off-by Petr Machata <[email protected]>

---

When a failure occurs in rtnl_configure_link(), the current code calls unregister_netdevice() to roll back the earlier call to register_netdevice(), and jumps to errout, which calls vxlan_fdb_destroy().

However unregister_netdevice() calls transitively ndo_uninit, which is vxlan_uninit(), and that already takes care of deleting the default FDB entry by calling vxlan_fdb_delete_default(). Since the entry added earlier in __vxlan_dev_create() is exactly the default entry, the cleanup code in the errout block always leads to double free and thus a panic.

Besides, since vxlan_fdb_delete_default() always destroys the FDB entry with notification enabled, the deletion of the default entry is notified even before the addition was notified.

Instead, before calling unregister_netdevice(), destroy the default FDB entry by hand, which solves both problems.

Fixes 0241b836732f ("vxlan: fix default fdb entry netlink notify ordering during netdev create")

Signed-off-by Petr Machata <[email protected]>

---

Notes:
v2:

- Delete the default entry before calling unregister_netdevice(). That takes care of former patch #3, hence tweak the commit message to mention that problem as well.

 drivers/net/vxlan.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

drivers/net/vxlan.c +6 -5

index c9956c08edf5..f29bf7d3e682 100644
@@ -3282,14 +3282,15 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
        }
 
        err = register_netdevice(dev);
-       if (err)
-               goto errout;
+       if (err) {
+               if (f)
+                       vxlan_fdb_destroy(vxlan, f, false);
+               return err;
+       }
 
        err = rtnl_configure_link(dev, NULL);
-       if (err) {
-               unregister_netdevice(dev);
+       if (err)
                goto errout;
-       }
 
        /* notify default fdb entry */
        if (f)

@@ -3297,9 +3298,15 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
 
        list_add(&vxlan->next, &vn->vxlan_list);

drivers/net/vxlan.c +0 -1

        return err;
 }
 
--

2.4.11

drivers/net/vxlan.c static int __vxlan_dev_create(struct net *net, struct net_device *dev, · line 3309

Quoted 13 lines

drivers/net/vxlan.c +6 -0

@@ -3297,9 +3298,15 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
 
        list_add(&vxlan->next, &vn->vxlan_list);
        return 0;
+
 errout:
+       /* unregister_netdevice() destroys the default FDB entry with deletion
+        * notification. But the addition notification was not sent yet, so
+        * destroy the entry by hand here.
+        */
        if (f)
                vxlan_fdb_destroy(vxlan, f, false);
+       unregister_netdevice(dev);

if (unregister_netdev_onerr)
unregister_netdevice

Quoted 34 lines · Petr Machata

Default remotes are stored as FDB entries with an Ethernet address of 00:00:00:00:00:00. When a request is made to change a remote address of a VXLAN device, vxlan_changelink() first deletes the existing default remote, and then creates a new FDB entry.

This works well as long as the list of default remotes matches exactly the configuration of a VXLAN remote address. Thus when the VXLAN device has a remote of X, there should be exactly one default remote FDB entry X. If the VXLAN device has no remote address, there should be no such entry.

Besides using "ip link set", it is possible to manipulate the list of default remotes by using the "bridge fdb". It is therefore easy to break the above condition. Under such circumstances, the __vxlan_fdb_delete() call doesn't delete the FDB entry itself, but just one remote. The following vxlan_fdb_create() then creates a new FDB entry, leading to a situation where two entries exist for the address 00:00:00:00:00:00, each with a different subset of default remotes.

An even more obvious breakage rooted in the same cause can be observed when a remote address is configured for a VXLAN device that did not have one before. In that case vxlan_changelink() doesn't remove any remote, and just creates a new FDB entry for the new address:

$ ip link add name vx up type vxlan id 2000 dstport 4789
$ bridge fdb ap dev vx 00:00:00:00:00:00 dst 192.0.2.20 self permanent
$ bridge fdb ap dev vx 00:00:00:00:00:00 dst 192.0.2.30 self permanent
$ ip link set dev vx type vxlan remote 192.0.2.30
$ bridge fdb sh dev vx | grep 00:00:00:00:00:00
00:00:00:00:00:00 dst 192.0.2.30 self permanent <- new entry, 1 rdst
00:00:00:00:00:00 dst 192.0.2.20 self permanent <- orig. entry, 2 rdsts
00:00:00:00:00:00 dst 192.0.2.30 self permanent

Quoted 8 lines

To fix this, instead of calling vxlan_fdb_create() directly, defer to vxlan_fdb_update(). That has logic to handle the duplicates properly. Additionally, it also handles notifications, so drop that call from changelink as well.

Fixes 0241b836732f ("vxlan: fix default fdb entry netlink notify ordering during netdev create")

Signed-off-by Petr Machata <[email protected]>

---

Acked-by Roopa Prabhu <[email protected]>

This fix looks fine. But it almost seems like the changelink Fixes tag is more appropriate here unless you are
sure that it worked correctly after the changelink.

Though I agree
this needs to be fixed, deployments either
configure the default remote dst on the vxlan device or manage default remote dsts via fdb api in control plane
not both. The trigger for this bug seems like the use of both.

Quoted 44 lines · Roopa Prabhu

Quoted 42 lines · Petr Machata

Default remotes are stored as FDB entries with an Ethernet address of 00:00:00:00:00:00. When a request is made to change a remote address of a VXLAN device, vxlan_changelink() first deletes the existing default remote, and then creates a new FDB entry.

This works well as long as the list of default remotes matches exactly the configuration of a VXLAN remote address. Thus when the VXLAN device has a remote of X, there should be exactly one default remote FDB entry X. If the VXLAN device has no remote address, there should be no such entry.

Besides using "ip link set", it is possible to manipulate the list of default remotes by using the "bridge fdb". It is therefore easy to break the above condition. Under such circumstances, the __vxlan_fdb_delete() call doesn't delete the FDB entry itself, but just one remote. The following vxlan_fdb_create() then creates a new FDB entry, leading to a situation where two entries exist for the address 00:00:00:00:00:00, each with a different subset of default remotes.

An even more obvious breakage rooted in the same cause can be observed when a remote address is configured for a VXLAN device that did not have one before. In that case vxlan_changelink() doesn't remove any remote, and just creates a new FDB entry for the new address:

$ ip link add name vx up type vxlan id 2000 dstport 4789
$ bridge fdb ap dev vx 00:00:00:00:00:00 dst 192.0.2.20 self permanent
$ bridge fdb ap dev vx 00:00:00:00:00:00 dst 192.0.2.30 self permanent
$ ip link set dev vx type vxlan remote 192.0.2.30
$ bridge fdb sh dev vx | grep 00:00:00:00:00:00
00:00:00:00:00:00 dst 192.0.2.30 self permanent <- new entry, 1 rdst
00:00:00:00:00:00 dst 192.0.2.20 self permanent <- orig. entry, 2 rdsts
00:00:00:00:00:00 dst 192.0.2.30 self permanent

To fix this, instead of calling vxlan_fdb_create() directly, defer to vxlan_fdb_update(). That has logic to handle the duplicates properly. Additionally, it also handles notifications, so drop that call from changelink as well.

Fixes 0241b836732f ("vxlan: fix default fdb entry netlink notify ordering during netdev create")

Signed-off-by Petr Machata <[email protected]>

---

Quoted 5 lines

Acked-by Roopa Prabhu <[email protected]>

This fix looks fine. But it almost seems like the changelink Fixes tag is more appropriate here unless you are
sure that it worked correctly after the changelink.

I don't follow. What do you mean?

Quoted 6 lines

Though I agree
this needs to be fixed, deployments either
configure the default remote dst on the vxlan device or manage default remote dsts via fdb api in control plane
not both. The trigger for this bug seems like the use of both.

Yep, you need to do both to trigger this. Which is why I think this went undetected--otherwise people would have noticed some addresses are not flooded.

Quoted 12 lines · Petr Machata

Quoted 9 lines · Roopa Prabhu

Quoted 3 lines

Fixes 0241b836732f ("vxlan: fix default fdb entry netlink notify ordering during netdev create")

Signed-off-by Petr Machata <[email protected]>

---

Acked-by Roopa Prabhu <[email protected]>

This fix looks fine. But it almost seems like the changelink Fixes tag is more appropriate here unless you are
sure that it worked correctly after the changelink.

Quoted 1 line

I don't follow. What do you mean?

I meant the notify patch in the Fixes tag had no intentions to change this. Also the vxlan changelink support
did not consider this as a test case. So i was wondering if this problem existed before the commit in the Fixes tag.

regardless, ack on the fix. thanks.