netdev - Re: [PATCH net v2 2/4] vxlan: Fix error path in __vxlan_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJieiUj4Xvgi+26jfrrHJQg72rUEaRTnc1f9va=63RQaca+13Q@mail.gmail.com>
Date:   Mon, 17 Dec 2018 12:07:24 -0800
From:   Roopa Prabhu <roopa@...ulusnetworks.com>
To:     Petr Machata <petrm@...lanox.com>
Cc:     netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net v2 2/4] vxlan: Fix error path in __vxlan_dev_create()

On Mon, Dec 17, 2018 at 10:10 AM Petr Machata <petrm@...lanox.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 <petrm@...lanox.com>
> ---

Acked-by: Roopa Prabhu <roopa@...ulusnetworks.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
>