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

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: <CAJieiUgg2tNZNHCSV_DyauAM35zoDZpyEkKznWEqECMaJQSrXw@mail.gmail.com>
Date:   Mon, 17 Dec 2018 07:41:15 -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 3/5] vxlan: Don't notify about deletion of non-added
 default FDB entry

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