On Mon, Dec 17, 2018 at 10:10 AM Petr Machata <pe...@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.
>
> 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 <pe...@mellanox.com>
> ---
Acked-by: Roopa Prabhu <ro...@cumulusnetworks.com>
thanks for adding the comment. this looks good.., minor nit..can we
remove the repeated vxlan_fdb_destroy by using a
unregister_netdev_onerr bool ?
eg .,.
>
> 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(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index c9956c08edf5..f29bf7d3e682 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)
> @@ -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
> return err;
> }
>
> --
> 2.4.11
>
