Message-ID: <CAJieiUjUONos+5V2tZRtx_aGuMLOdwbUR74HwNVT5P8GdzJhbw@mail.gmail.com>
Date:   Mon, 17 Dec 2018 17:30:33 -0800
From:   Roopa Prabhu <roopa@...ulusnetworks.com>
To:     Petr Machata <petrm@...lanox.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net v2 3/4] vxlan: changelink: Fix handling of default remotes

On Mon, Dec 17, 2018 at 3:44 PM Petr Machata <petrm@...lanox.com> wrote:
>
> Roopa Prabhu <roopa@...ulusnetworks.com> writes:
>
> > On Mon, Dec 17, 2018 at 10:10 AM Petr Machata <petrm@...lanox.com> wrote:
> >>
> >> 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@...lanox.com>
> >> ---
> >
> > Acked-by: Roopa Prabhu <roopa@...ulusnetworks.com>
> >
> > 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?

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.

>
> > 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.

Powered by blists - more mailing lists