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

[henry martin]

Both findings are correct, thanks.

The Medium one matters: placed in the append block, the new check
returned -EOPNOTSUPP only after the entry state and flags had been
rewritten, skipping the notification pass and the err_notify
rollback and leaving the entry half-updated (f->flags losing
NTF_VXLAN_ADDED_BY_USER for one).

v2 moves the check next to the existing nexthop guards at the top
of vxlan_fdb_update_existing(), before anything is changed, and
rewords the extack message to "Cannot add a remote to a nexthop fdb
entry" so it also reads sanely for the vxlan_changelink() and
vnifilter callers; the changelog now documents that these two
rtnetlink paths change behavior (silent wrong-remote attachment ->
explicit failure).

pw-bot: cr

Regards,
Henry

<netdev-bot+sashiko@xxxxxxxxxx> 于2026年10月11日周日 15:03写道：
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] In vxlan_fdb_update_existing()
>   (drivers/net/vxlan/vxlan_core.c), the new `if…
> - [Low] The commit message presents the change only as rejecting a
>   userspace `bridge fdb append` onto a nexthop entry.
>
> --
>
> vxlan: reject appending a remote to a nexthop fdb entry
>
> This patch adds a check to vxlan_fdb_update_existing(). An NLM_F_APPEND
> request on a multicast or zero MAC fdb entry now fails with -EOPNOTSUPP
> when the entry is backed by a nexthop group. Before this,
> vxlan_fdb_append() would link a remote onto such an entry, which breaks
> the rule that nexthop entries have no remote list.
>
> > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> > index 278d41f58d895..e9c5ba7a74172 100644
> > --- a/drivers/net/vxlan/vxlan_core.c
> > +++ b/drivers/net/vxlan/vxlan_core.c
> > @@ -1047,6 +1047,11 @@ static int vxlan_fdb_update_existing(struct vxlan_dev *vxlan,
> >       if ((flags & NLM_F_APPEND) &&
> >           (is_multicast_ether_addr(f->key.eth_addr) ||
> >            is_zero_ether_addr(f->key.eth_addr))) {
> > +             if (rcu_access_pointer(f->nh)) {
> > +                     NL_SET_ERR_MSG(extack,
> > +                                    "Cannot append a remote to a nexthop fdb");
> > +                     return -EOPNOTSUPP;
> > +             }
>
> [Severity: Medium]
> Can this early return leave the entry partly updated? When this check
> runs, vxlan_fdb_update_existing() has already written the new state and
> flags:
>
>         if (!(fdb_flags & NTF_EXT_LEARNED) ||
>             !(f->flags & NTF_VXLAN_ADDED_BY_USER)) {
>                 if (f->state != state) {
>                         f->state = state;
>                         notify = 1;
>                 }
>                 if (f->flags != fdb_flags) {
>                         f->flags = fdb_flags;
>                         notify = 1;
>                 }
>         }
>
> Returning -EOPNOTSUPP here skips the if (notify) block, so no
> RTM_NEWNEIGH or switchdev notification goes out. It also skips the
> err_notify rollback.
>
> For example, create a zero MAC nexthop entry with:
>
>   bridge fdb add 00:00:00:00:00:00 dev vx0 nhid N self permanent
>
> Then run:
>
>   bridge fdb append 00:00:00:00:00:00 dev vx0 dst X self permanent router
>
> The append passes the earlier nexthop checks, because nhid is 0 and the
> state includes NUD_PERMANENT. It changes f->flags and then fails.
> vxlan_fdb_add() returns the error and restores nothing.
>
> The in-kernel callers vxlan_changelink() and
> vxlan_update_default_fdb_entry() also reach this point. They pass
> ndm_flags = NTF_SELF and state = NUD_REACHABLE | NUD_PERMANENT.
>
> On a nexthop default entry created by the user, f->flags is overwritten
> with NTF_SELF, which clears NTF_VXLAN_ADDED_BY_USER, and then the call
> fails. vxlan_changelink() treats the failure as if nothing changed and
> frees new_cfg.
>
> The other three nexthop checks near the top of this function all return
> before anything is changed. Could this check move up next to them, so
> that the failed request has no side effects?
>
> [Severity: Low]
> The commit message presents this as rejecting a user's bridge fdb append
> onto a nexthop entry. Two in-kernel paths also reach this check with
> NLM_F_APPEND | NLM_F_CREATE on all_zeros_mac and nhid = 0:
>
>   vxlan_changelink()
>     vxlan_fdb_update()
>       vxlan_fdb_update_existing()
>
>   vxlan_update_default_fdb_entry()
>     vxlan_fdb_update()
>       vxlan_fdb_update_existing()
>
> The first runs on "ip link set dev vx0 type vxlan remote <ip>". The
> second runs on vnifilter group add or change, for example
> "bridge vni add ... group ...".
>
> On a non-metadata device, the zero MAC lookup is keyed on cfg->vni. A
> nexthop entry created with "bridge fdb add 00:00:00:00:00:00 dev vx0
> nhid N self permanent" is therefore the same entry these paths update.
>
> Before this patch, these link and VNI changes succeeded, though they left
> an unused remote on the entry. With this patch they fail with
> -EOPNOTSUPP and the extack "Cannot append a remote to a nexthop fdb".
> That message describes an fdb operation the user never issued.
>
> Rejecting these requests is probably the right result. Since the patch
> is tagged for stable, should the commit message mention this change in
> rtnetlink link and vnifilter behavior? Could the error message also be
> made accurate for these callers?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010064802.2741221-1-bsdhenrymartin%40gmail.com