[PATCH net 0/5] vxlan: Various fixes

[PATCH net 0/5] vxlan: Various fixes

[Petr Machata]

This patch set contains four fixes for the vxlan driver.

Patch #1 fixes handling of offload mark on replaced VXLAN FDB entries. A
way to trigger this is to replace the FDB entry with one that can not be
offloaded. A future patch set should make it possible to veto such FDB
changes. However the FDB might still fail to be offloaded due to another
issue, and the offload mark should reflect that.

Patches #2 and #3 fix problems in __vxlan_dev_create() when a call to
rtnl_configure_link() fails. These failures would be tricky to hit on a
real system, the most likely vector is through an error in vxlan_open().
However, with the abovementioned vetoing patchset, vetoing the created
entry would trigger the same problems (and be easier to reproduce).

Patch #4 fixes a problem in vxlan_changelink(). In situations where the
default remote configured in the FDB table (if any) does not exactly
match the remote address configured at the VXLAN device, changing the
remote address breaks the default FDB entry. Patch #5 is then a self
test for this issue.

Petr Machata (5):
  vxlan: Unmark offloaded bit on replaced FDB entries
  vxlan: Don't double-free default FDB entry in __vxlan_dev_create()
  vxlan: Don't notify about deletion of non-added default FDB entry
  vxlan: changelink: Fix handling of default remotes
  selftests: net: Add test_vxlan_fdb_changelink.sh

 drivers/net/vxlan.c                                | 26 ++++++++++---------
 include/net/vxlan.h                                |  1 +
 tools/testing/selftests/net/Makefile               |  1 +
 .../selftests/net/test_vxlan_fdb_changelink.sh     | 29 ++++++++++++++++++++++
 4 files changed, 45 insertions(+), 12 deletions(-)
 create mode 100755 tools/testing/selftests/net/test_vxlan_fdb_changelink.sh

-- 
2.4.11

[PATCH net 1/5] vxlan: Unmark offloaded bit on replaced FDB entries

[Petr Machata]

When rdst of an offloaded FDB entry is replaced, it certainly isn't
offloaded anymore. Drivers are notified about such replacements, and can
re-mark the entry as offloaded again if they so wish. However until a
driver does so explicitly, assume a replaced FDB entry is not offloaded.

Note that replaces coming via vxlan_fdb_external_learn_add() are always
immediately followed by an explicit offload marking.

Fixes: 0efe11733356 ("vxlan: Support marking RDSTs as offloaded")
Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 drivers/net/vxlan.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 297cdeaef479..c9956c08edf5 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -568,6 +568,7 @@ static int vxlan_fdb_replace(struct vxlan_fdb *f,
 	rd->remote_port = port;
 	rd->remote_vni = vni;
 	rd->remote_ifindex = ifindex;
+	rd->offloaded = false;
 	return 1;
 }
 
-- 
2.4.11

[PATCH net 2/5] vxlan: Don't double-free default FDB entry in __vxlan_dev_create()

[Petr Machata]

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.

Fix by skipping the rollback code and just returning.

Fixes: 0241b836732f ("vxlan: fix default fdb entry netlink notify ordering during netdev create")
Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 drivers/net/vxlan.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index c9956c08edf5..139741617b90 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -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)
@@ -3298,8 +3299,8 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
 	list_add(&vxlan->next, &vn->vxlan_list);
 	return 0;
 errout:
-	if (f)
-		vxlan_fdb_destroy(vxlan, f, false);
+	unregister_netdevice(dev);
+	/* the all_zeros_mac entry was deleted at vxlan_uninit */
 	return err;
 }
 
-- 
2.4.11

[PATCH net 3/5] vxlan: Don't notify about deletion of non-added default FDB entry

[Petr Machata]

In __vxlan_dev_create(), one of the steps is to add a default FDB entry.
The default entry is created and added before the device is even
registered, and is likewise not removed until destruction of the device.
(Except when it's removed explicitly.) Consequently, the notification
about this default FDB entry is only sent belatedly, to prevent sending
neighbor notifications before notifications for the corresponding link
itself.

Before the addition notification is sent, the whole procedure can fail
due to an error return from rtnl_configure_link(). That triggers
unregister of the freshly registered device, which causes removal of the
default FDB entry, including deletion notification. However since at
that point the addition notification itself was not yet distributed, it
is incorrect to send deletion notifications.

To fix the issue, allocate a bit at VXLAN device to communicate whether
a notifier should be invoked for the sole FDB entry or not. Set to true
after the notification is sent.

Fixes: 0241b836732f ("vxlan: fix default fdb entry netlink notify ordering during netdev create")
Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 drivers/net/vxlan.c | 3 ++-
 include/net/vxlan.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 139741617b90..c60f1b420d71 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -850,7 +850,7 @@ static void vxlan_fdb_destroy(struct vxlan_dev *vxlan, struct vxlan_fdb *f,
 		    "delete %pM\n", f->eth_addr);
 
 	--vxlan->addrcnt;
-	if (do_notify)
+	if (do_notify && vxlan->cfg.do_notify)
 		list_for_each_entry(rd, &f->remotes, list)
 			vxlan_fdb_notify(vxlan, f, rd, RTM_DELNEIGH);
 
@@ -3295,6 +3295,7 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
 	/* notify default fdb entry */
 	if (f)
 		vxlan_fdb_notify(vxlan, f, first_remote_rtnl(f), RTM_NEWNEIGH);
+	vxlan->cfg.do_notify = true;
 
 	list_add(&vxlan->next, &vn->vxlan_list);
 	return 0;
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 03431c148e16..5932f89bd932 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -216,6 +216,7 @@ struct vxlan_config {
 	unsigned long		age_interval;
 	unsigned int		addrmax;
 	bool			no_share;
+	u8			do_notify:1;
 };
 
 struct vxlan_dev_node {
-- 
2.4.11

[PATCH net 4/5] vxlan: changelink: Fix handling of default remotes

[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 <petrm@mellanox.com>
---
 drivers/net/vxlan.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index c60f1b420d71..357bc00903b8 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3537,7 +3537,6 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
 	struct vxlan_rdst *dst = &vxlan->default_dst;
 	struct vxlan_rdst old_dst;
 	struct vxlan_config conf;
-	struct vxlan_fdb *f = NULL;
 	int err;
 
 	err = vxlan_nl2conf(tb, data,
@@ -3563,19 +3562,19 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
 					   old_dst.remote_ifindex, 0);
 
 		if (!vxlan_addr_any(&dst->remote_ip)) {
-			err = vxlan_fdb_create(vxlan, all_zeros_mac,
+			err = vxlan_fdb_update(vxlan, all_zeros_mac,
 					       &dst->remote_ip,
 					       NUD_REACHABLE | NUD_PERMANENT,
+					       NLM_F_APPEND | NLM_F_CREATE,
 					       vxlan->cfg.dst_port,
 					       dst->remote_vni,
 					       dst->remote_vni,
 					       dst->remote_ifindex,
-					       NTF_SELF, &f);
+					       NTF_SELF);
 			if (err) {
 				spin_unlock_bh(&vxlan->hash_lock);
 				return err;
 			}
-			vxlan_fdb_notify(vxlan, f, first_remote_rtnl(f), RTM_NEWNEIGH);
 		}
 		spin_unlock_bh(&vxlan->hash_lock);
 	}
-- 
2.4.11

[PATCH net 5/5] selftests: net: Add test_vxlan_fdb_changelink.sh

[Petr Machata]

Add a test to exercise the fix from the previous patch.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 tools/testing/selftests/net/Makefile               |  1 +
 .../selftests/net/test_vxlan_fdb_changelink.sh     | 29 ++++++++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100755 tools/testing/selftests/net/test_vxlan_fdb_changelink.sh

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 256d82d5fa87..923570a9708a 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -7,6 +7,7 @@ CFLAGS += -I../../../../usr/include/
 TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetlink.sh
 TEST_PROGS += fib_tests.sh fib-onlink-tests.sh pmtu.sh udpgso.sh ip_defrag.sh
 TEST_PROGS += udpgso_bench.sh fib_rule_tests.sh msg_zerocopy.sh psock_snd.sh
+TEST_PROGS += test_vxlan_fdb_changelink.sh
 TEST_PROGS_EXTENDED := in_netns.sh
 TEST_GEN_FILES =  socket
 TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy
diff --git a/tools/testing/selftests/net/test_vxlan_fdb_changelink.sh b/tools/testing/selftests/net/test_vxlan_fdb_changelink.sh
new file mode 100755
index 000000000000..2d442cdab11e
--- /dev/null
+++ b/tools/testing/selftests/net/test_vxlan_fdb_changelink.sh
@@ -0,0 +1,29 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# Check FDB default-remote handling across "ip link set".
+
+check_remotes()
+{
+	local what=$1; shift
+	local N=$(bridge fdb sh dev vx | grep 00:00:00:00:00:00 | wc -l)
+
+	echo -ne "expected two remotes after $what\t"
+	if [[ $N != 2 ]]; then
+		echo "[FAIL]"
+		EXIT_STATUS=1
+	else
+		echo "[ OK ]"
+	fi
+}
+
+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
+check_remotes "fdb append"
+
+ip link set dev vx type vxlan remote 192.0.2.30
+check_remotes "link set"
+
+ip link del dev vx
+exit $EXIT_STATUS
-- 
2.4.11

Re: [PATCH net 2/5] vxlan: Don't double-free default FDB entry in __vxlan_dev_create()

[Roopa Prabhu]

On Mon, Dec 17, 2018 at 6:58 AM Petr Machata <petrm@mellanox.com> wrote:
>
> 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.
>
> Fix by skipping the rollback code and just returning.

I trust your test case that pointed to a potential issue..., but can
we add a comment there on
why there is an fdb add but no destroy in the roll back code ?. Also,
the destroy code from uninit path does
look for an entry being present before attempting a destroy, its
unclear to me yet why that ends up doing a double free ?


>
> Fixes: 0241b836732f ("vxlan: fix default fdb entry netlink notify ordering during netdev create")
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---
>  drivers/net/vxlan.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index c9956c08edf5..139741617b90 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -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)
> @@ -3298,8 +3299,8 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
>         list_add(&vxlan->next, &vn->vxlan_list);
>         return 0;
>  errout:
> -       if (f)
> -               vxlan_fdb_destroy(vxlan, f, false);
> +       unregister_netdevice(dev);
> +       /* the all_zeros_mac entry was deleted at vxlan_uninit */
>         return err;
>  }
>
> --
> 2.4.11
>

Re: [PATCH net 2/5] vxlan: Don't double-free default FDB entry in __vxlan_dev_create()

[Petr Machata]

Roopa Prabhu <roopa@cumulusnetworks.com> writes:

> On Mon, Dec 17, 2018 at 6:58 AM Petr Machata <petrm@mellanox.com> wrote:
>>
>> 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.
>>
>> Fix by skipping the rollback code and just returning.
>
> I trust your test case that pointed to a potential issue..., but can
> we add a comment there on
> why there is an fdb add but no destroy in the roll back code ?. Also,

This is at a place in the code where the clean-up would normally be:

>> +       /* the all_zeros_mac entry was deleted at vxlan_uninit */

Is that what you meant?

> the destroy code from uninit path does
> look for an entry being present before attempting a destroy, its
> unclear to me yet why that ends up doing a double free ?

That code is called first, finds the FDB entry, destroys it. After it
returns, another destroy call is invoked from __vxlan_dev_create().

>> @@ -3298,8 +3299,8 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
>>         list_add(&vxlan->next, &vn->vxlan_list);
>>         return 0;
>>  errout:
>> -       if (f)
>> -               vxlan_fdb_destroy(vxlan, f, false);
>> +       unregister_netdevice(dev);
>> +       /* the all_zeros_mac entry was deleted at vxlan_uninit */
>>         return err;
>>  }
>>
>> --
>> 2.4.11
>>

Re: [PATCH net 3/5] vxlan: Don't notify about deletion of non-added default FDB entry

[Roopa Prabhu]

On Mon, Dec 17, 2018 at 6:58 AM Petr Machata <petrm@mellanox.com> wrote:
>
> In __vxlan_dev_create(), one of the steps is to add a default FDB entry.
> The default entry is created and added before the device is even
> registered, and is likewise not removed until destruction of the device.
> (Except when it's removed explicitly.) Consequently, the notification
> about this default FDB entry is only sent belatedly, to prevent sending
> neighbor notifications before notifications for the corresponding link
> itself.
>
> Before the addition notification is sent, the whole procedure can fail
> due to an error return from rtnl_configure_link(). That triggers
> unregister of the freshly registered device, which causes removal of the
> default FDB entry, including deletion notification. However since at
> that point the addition notification itself was not yet distributed, it
> is incorrect to send deletion notifications.
>
> To fix the issue, allocate a bit at VXLAN device to communicate whether
> a notifier should be invoked for the sole FDB entry or not. Set to true
> after the notification is sent.
>
> Fixes: 0241b836732f ("vxlan: fix default fdb entry netlink notify ordering during netdev create")
> Signed-off-by: Petr Machata <petrm@mellanox.com>

The initial fix used a flag argument to vxlan_fdb_destroy to not send
the notification. I understand that you are reworking it...
But a general do_notify flag in vxlan config to work around this seems
odd. pls see my comment on your other patch that
removes the vxlan_fdb_destroy call, can we fix vxlan_uninit or the
error path in vxlan create to not do call destroy twice ?

> ---
>  drivers/net/vxlan.c | 3 ++-
>  include/net/vxlan.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 139741617b90..c60f1b420d71 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -850,7 +850,7 @@ static void vxlan_fdb_destroy(struct vxlan_dev *vxlan, struct vxlan_fdb *f,
>                     "delete %pM\n", f->eth_addr);
>
>         --vxlan->addrcnt;
> -       if (do_notify)
> +       if (do_notify && vxlan->cfg.do_notify)
>                 list_for_each_entry(rd, &f->remotes, list)
>                         vxlan_fdb_notify(vxlan, f, rd, RTM_DELNEIGH);
>
> @@ -3295,6 +3295,7 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
>         /* notify default fdb entry */
>         if (f)
>                 vxlan_fdb_notify(vxlan, f, first_remote_rtnl(f), RTM_NEWNEIGH);
> +       vxlan->cfg.do_notify = true;
>
>         list_add(&vxlan->next, &vn->vxlan_list);
>         return 0;
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 03431c148e16..5932f89bd932 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -216,6 +216,7 @@ struct vxlan_config {
>         unsigned long           age_interval;
>         unsigned int            addrmax;
>         bool                    no_share;
> +       u8                      do_notify:1;
>  };
>
>  struct vxlan_dev_node {
> --
> 2.4.11
>

Re: [PATCH net 2/5] vxlan: Don't double-free default FDB entry in __vxlan_dev_create()

[Roopa Prabhu]

On Mon, Dec 17, 2018 at 7:39 AM Petr Machata <petrm@mellanox.com> wrote:
>
> Roopa Prabhu <roopa@cumulusnetworks.com> writes:
>
> > On Mon, Dec 17, 2018 at 6:58 AM Petr Machata <petrm@mellanox.com> wrote:
> >>
> >> 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.
> >>
> >> Fix by skipping the rollback code and just returning.
> >
> > I trust your test case that pointed to a potential issue..., but can
> > we add a comment there on
> > why there is an fdb add but no destroy in the roll back code ?. Also,
>
> This is at a place in the code where the clean-up would normally be:
>
> >> +       /* the all_zeros_mac entry was deleted at vxlan_uninit */
>
> Is that what you meant?
>
> > the destroy code from uninit path does
> > look for an entry being present before attempting a destroy, its
> > unclear to me yet why that ends up doing a double free ?
>
> That code is called first, finds the FDB entry, destroys it. After it
> returns, another destroy call is invoked from __vxlan_dev_create().
>

ok, I am wondering if we can just avoid calling the fdb destroy in
error path if unregister has already happened on the device with a
flag local to that function or add extra check for the fdb being
present before calling destroy in the error path. This keeps this fdb
destroy checks local to this function.

Re: [PATCH net 2/5] vxlan: Don't double-free default FDB entry in __vxlan_dev_create()

[Roopa Prabhu]

On Mon, Dec 17, 2018 at 7:45 AM Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>
> On Mon, Dec 17, 2018 at 7:39 AM Petr Machata <petrm@mellanox.com> wrote:
> >
> > Roopa Prabhu <roopa@cumulusnetworks.com> writes:
> >
> > > On Mon, Dec 17, 2018 at 6:58 AM Petr Machata <petrm@mellanox.com> wrote:
> > >>
> > >> 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.
> > >>
> > >> Fix by skipping the rollback code and just returning.
> > >
> > > I trust your test case that pointed to a potential issue..., but can
> > > we add a comment there on
> > > why there is an fdb add but no destroy in the roll back code ?. Also,
> >
> > This is at a place in the code where the clean-up would normally be:
> >
> > >> +       /* the all_zeros_mac entry was deleted at vxlan_uninit */
> >
> > Is that what you meant?
> >
> > > the destroy code from uninit path does
> > > look for an entry being present before attempting a destroy, its
> > > unclear to me yet why that ends up doing a double free ?
> >
> > That code is called first, finds the FDB entry, destroys it. After it
> > returns, another destroy call is invoked from __vxlan_dev_create().
> >
>
> ok, I am wondering if we can just avoid calling the fdb destroy in
> error path if unregister has already happened on the device with a
> flag local to that function or add extra check for the fdb being
> present before calling destroy in the error path. This keeps this fdb
> destroy checks local to this function.

i think you probably just want a call to fdb destroy before
unregister_netdevice in the error path. that should avoid the delete
in vxlan_uninit and thus avoid patch3

Re: [PATCH net 2/5] vxlan: Don't double-free default FDB entry in __vxlan_dev_create()

[Petr Machata]

Roopa Prabhu <roopa@cumulusnetworks.com> writes:

> On Mon, Dec 17, 2018 at 7:45 AM Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>>
>> On Mon, Dec 17, 2018 at 7:39 AM Petr Machata <petrm@mellanox.com> wrote:
>> >
>> > Roopa Prabhu <roopa@cumulusnetworks.com> writes:
>> >
>> > > On Mon, Dec 17, 2018 at 6:58 AM Petr Machata <petrm@mellanox.com> wrote:
>> > >>
>> > >> 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.
>> > >>
>> > >> Fix by skipping the rollback code and just returning.
>> > >
>> > > I trust your test case that pointed to a potential issue..., but can
>> > > we add a comment there on
>> > > why there is an fdb add but no destroy in the roll back code ?. Also,
>> >
>> > This is at a place in the code where the clean-up would normally be:
>> >
>> > >> +       /* the all_zeros_mac entry was deleted at vxlan_uninit */
>> >
>> > Is that what you meant?
>> >
>> > > the destroy code from uninit path does
>> > > look for an entry being present before attempting a destroy, its
>> > > unclear to me yet why that ends up doing a double free ?
>> >
>> > That code is called first, finds the FDB entry, destroys it. After it
>> > returns, another destroy call is invoked from __vxlan_dev_create().
>> >
>>
>> ok, I am wondering if we can just avoid calling the fdb destroy in
>> error path if unregister has already happened on the device with a
>> flag local to that function or add extra check for the fdb being
>> present before calling destroy in the error path. This keeps this fdb
>> destroy checks local to this function.
>
> i think you probably just want a call to fdb destroy before
> unregister_netdevice in the error path. that should avoid the delete
> in vxlan_uninit and thus avoid patch3

The reason I did it like this is that the vxlan driver seems to work
hard to have the default FDB entry present at all points in lifetime
(unless it's removed explicitly), and removes it as the last thing.

I suppose that since at this point the default FDB entry has not been
notified yet, it should be OK to silently drop it inside
__vxlan_dev_create(). I agree it's preferable to the global flag.