Re: [PATCH net v2] vxlan: reject adding a remote to a nexthop fdb entry

[henry martin]

Thanks for the review.

No crash and no leak: the appended vxlan_rdst is freed with the
entry, and vxlan_xmit() follows the nexthop whenever f->nh is set.
What breaks is state consistency:

- vxlan_fdb_notify(), RTM_GETNEIGH dumps and the switchdev FDB
  programming all prefer the remote list over the nexthop, so user
  space is told traffic goes to the appended remote while the
  datapath follows the nexthop group.

- Once a stray remote is attached to an all-zeros nexthop entry,
  __vxlan_fdb_delete() destroys the whole entry when the matched
  remote is the only one on the list, so a dst-scoped "bridge fdb
  del ..." or "ip link set ... remote 0.0.0.0" tears down the
  nexthop entry, not just the stray remote.  Same class of
  inconsistency as commit 98fc57d16744.

Test cases added to fib_nexthops.sh in both ipv4_fdb_grp_fcnal() and
ipv6_fdb_grp_fcnal(), mirroring 98fc57d16744's style (rejected
append and rejected non-unicast replace on a nexthop entry).

A sashiko pass on v2 also caught that the rejection would leave
"bridge vni add" half-installed, so v4 is a 2-patch series: [1/2]
the fdb guard with the above message and tests, [2/2] a pre-install
rejection in vxlan_vni_add().  If the consequence description still
reads as net-next-only to you, I will re-send there once it reopens.

pw-bot: cr

Regards,
Henry

Ido Schimmel <idosch@xxxxxxxxxx> 于2026年10月21日周三 18:26写道：
>
> On Tue, Oct 20, 2026 at 11:45:31AM +0800, Henry Martin wrote:
> > FDB entries backed by a nexthop group must not carry a remote
> > destination list.  vxlan_fdb_update_existing() currently appends a
> > remote via vxlan_fdb_append() on an NLM_F_APPEND request with a
> > multicast or zero MAC even when the entry is backed by a nexthop,
> > breaking that invariant through three different callers:
> >
> > - "bridge fdb append" with a dst on a nexthop entry;
> > - "ip link set dev vx0 type vxlan remote <ip>" (vxlan_changelink());
> > - "bridge vni add/change ... group <mcast>" on a vnifilter device
> >   (vxlan_update_default_fdb_entry()).
> >
> > All three reach the append block with nhid == 0 and a PERMANENT
> > state, so they pass the earlier nexthop checks.
> >
> > Reject the combination at the top of vxlan_fdb_update_existing(),
> > next to the existing nexthop guards, so a refused request has no
> > side effects: placing the check in the append block instead would
> > return -EOPNOTSUPP only after the entry state and flags have been
> > rewritten, skipping both the notification pass and the err_notify
> > rollback.
> >
> > Note the resulting behavior change for the two rtnetlink callers:
> > operations that previously succeeded while silently attaching an
> > unused remote to a nexthop entry now fail with extack
> > "Cannot add a remote to a nexthop fdb entry".
> >
> > This issue was discovered by Tencent CodeBuddy Security.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 1274e1cc4226 ("vxlan: ecmp support for mac fdb entries")
> > Signed-off-by: Henry Martin <bsdhenrymartin@xxxxxxxxx>
>
> I agree that it doesn't make sense to append a remote to an FDB entry
> that points to a nexthop, but what are the consequences of allowing it?
> Does this patch fix a crash / memory leak? If so, the commit message
> should describe it. Otherwise, this looks like net-next material
> (currently closed).
>
> Also, please add a test case in fib_nexthops.sh, in a similar fashion to
> commit 98fc57d16744 ("vxlan: reject dynamic fdb entries that reference a
> nexthop id").