Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 (1) · Low: 2
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).
- [High] This patch adds a deliberate, deterministic -ENOMEM to the VNI
create path: vxlan_vni_update_group() calls…
- [Low] Commit message inaccuracy.
- [Low] In vxlan_fdb_update_existing() (drivers/net/vxlan/vxlan_core.c),
f->state and f->flags are overwritten before vxlan_fdb_append(), which…
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the
RTM_NEWNEIGH path vxlan_fdb_add() still allocates the same objects…
--
vxlan: vnifilter: account the default FDB entry to memcg
This patch adds a gfp_t argument to vxlan_fdb_update() and
vxlan_update_default_fdb_entry(). The argument is passed down to the
struct vxlan_fdb, struct vxlan_rdst and dst_cache allocations. The
per-VNI RTM_NEWTUNNEL path uses GFP_NOWAIT | __GFP_ACCOUNT, the
device-wide RTM_NEWLINK walk uses GFP_ATOMIC | __GFP_ACCOUNT, and all
other callers keep GFP_ATOMIC.
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 278d41f58d895..7991b544af5a4 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
[ ... ]
> @@ -1047,7 +1047,8 @@ 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))) {
> - rc = vxlan_fdb_append(f, ip, port, vni, ifindex, &rd);
> + rc = vxlan_fdb_append(f, ip, port, vni, ifindex, &rd,
> + gfp);
>
> if (rc < 0)
> return rc;
[Severity: Low]
The vnifilter change path now passes GFP_NOWAIT | __GFP_ACCOUNT, so this
append can fail once the memcg is at its limit. By that point,
vxlan_fdb_update_existing() has already overwritten f->state and f->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;
}
}
On the vxlan_vni_update_group(create=false) ->
vxlan_update_default_fdb_entry() path, fdb_flags is NTF_SELF without
NTF_EXT_LEARNED. So the existing all-zeros entry always has its state and
flags reset. For example, a user-set NTF_VXLAN_ADDED_BY_USER is dropped.
If vxlan_fdb_append() then returns -ENOMEM, the return rc path does not
restore these fields, and no RTM_NEWNEIGH is sent.
Could the state/flags update move to after a successful append, or be
rolled back here? This ordering is older than this patch, but the failure
is now deterministic. The commit message also says "no established FDB
state is destroyed", which doesn't quite hold for these fields.
[ ... ]
> @@ -1287,7 +1289,7 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct
> nlattr *tb[],
> err = vxlan_fdb_update(vxlan, addr, &ip, ndm->ndm_state, flags,
> port, src_vni, vni, ifindex,
> ndm->ndm_flags | NTF_VXLAN_ADDED_BY_USER,
> - nhid, true, extack);
> + nhid, true, extack, GFP_ATOMIC);
> spin_unlock_bh(&vxlan->hash_lock);
[Severity: Medium]
This isn't a bug introduced by this patch, but this RTM_NEWNEIGH path
still allocates the same struct vxlan_fdb, struct vxlan_rdst and per-CPU
dst_cache without charging them to a memcg.
With NLM_F_APPEND on the all-zeros MAC or a multicast MAC, each distinct
(ip, port, vni, ifindex) adds a new rdst plus dst_cache:
vxlan_fdb_add()
vxlan_fdb_update(..., GFP_ATOMIC)
vxlan_fdb_update_existing()
vxlan_fdb_append()
This path never reaches the cfg->addrmax check in vxlan_fdb_create(). A
task with CAP_NET_ADMIN in the netns can keep growing this memory
uncharged, including by appending to the vnifilter default entries that
this patch now charges.
The commit message limits the scope to vnifilter, so this may be planned
as a follow-up. Would it make sense to account this path as well?
[ ... ]
> @@ -4652,7 +4656,8 @@ static int vxlan_changelink(struct net_device *dev,
> struct nlattr *tb[],
> &cfg->remote_ip,
>
> conf.remote_ifindex,
>
> cfg->remote_ifindex,
> - NULL);
> + NULL,
> + GFP_ATOMIC |
> __GFP_ACCOUNT);
> if (lowerdev_changed)
>
> netdev_adjacent_change_abort(vxlan->lowerdev,
> lowerdev,
> dev);
[Severity: Low]
The commit message says:
The device-wide default-remote change (RTM_NEWLINK, via
vxlan_vnilist_update_group()) keeps GFP_ATOMIC | __GFP_ACCOUNT.
...
here it stays force-charged, as before this series, and is charged to
the task issuing the RTM_NEWLINK.
Before this patch, vxlan_fdb_alloc() and vxlan_fdb_append() hard-coded
GFP_ATOMIC without __GFP_ACCOUNT, so nothing on the
vxlan_vnilist_update_group() walk was charged. Isn't the accounting, and
the force-charging, new behaviour added by this patch rather than kept?
The message also doesn't mention this vxlan_changelink() rollback call,
which now gets GFP_ATOMIC | __GFP_ACCOUNT. The rollback re-creates the
conf.vni default entry as accounted. vxlan_dev_create() and the forward
vxlan_fdb_update() call in vxlan_changelink() create the same kind of
entry unaccounted. Is that inconsistency intended?
[ ... ]
> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c
> b/drivers/net/vxlan/vxlan_vnifilter.c
> index 4a631fcdf23ab..4b43674cceff0 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c
[ ... ]
> @@ -636,7 +637,8 @@ static int vxlan_vni_update_group(struct vxlan_dev *vxlan,
> oldrip, newrip,
> cfg->remote_ifindex,
> cfg->remote_ifindex,
> - extack);
> + extack,
> + GFP_NOWAIT | __GFP_ACCOUNT);
> if (ret)
> goto out;
[Severity: High]
With GFP_NOWAIT | __GFP_ACCOUNT, try_charge_memcg() fails without reclaim
once the memcg is at its limit. That means this call can now routinely
return -ENOMEM on the create path. Does vxlan_vni_add() handle that
failure?
By the time vxlan_vni_update_group() runs, the node is already published:
drivers/net/vxlan/vxlan_vnifilter.c:vxlan_vni_add() {
...
err = rhashtable_lookup_insert_fast(&vg->vni_hash,
&vninode->vnode,
vxlan_vni_rht_params);
...
__vxlan_vni_add_list(vxlan, vg, vninode);
if (vxlan->dev->flags & IFF_UP)
vxlan_vs_add_del_vninode(vxlan, vninode, false);
err = vxlan_vni_update_group(vxlan, vninode, group, true, &changed,
extack);
vxlan_vnifilter_notify(vxlan, vninode, RTM_NEWTUNNEL);
return err;
}
On failure, goto out skips recording vninode->remote_ip and skips the
IGMP join. The node still stays in vni_hash, vni_list and the socket VNI
hash. RTM_NEWTUNNEL is sent anyway, while the requester gets -ENOMEM. The
VNI then receives traffic but has no default all-zeros FDB entry.
Suppose the request is retried without a group while cfg->remote_ip is
set. vxlan_vni_add() finds the node and calls vxlan_vni_update() ->
vxlan_vni_update_group(create=false). old_remote_ip is still any, so
oldrip and newrip both point at &cfg->remote_ip, and this check returns 0:
if (!create && oldrip && newrip && vxlan_addr_equal(oldrip, newrip))
return 0;
Wouldn't the retry report success without ever installing the default FDB
entry? Deleting and re-adding the VNI seems to be the only way to recover.
For a range add, vxlan_vni_add_del() also stops at the first error. The
failing VNI and all VNIs added before it stay installed.
The missing unwind in vxlan_vni_add() predates this patch. It was hard to
hit while GFP_ATOMIC forced the charge through. The commit message says:
The failure
is clean -- the new entry is allocated before the old is freed, so an
add that fails leaves the existing entry intact and returns the error;
no established FDB state is destroyed and no false success is returned.
Creating a VNI is gated the same way by the previous patch's node and
stats (GFP_KERNEL_ACCOUNT, hard-bounded).
Should vxlan_vni_add() unlink and free the node, and skip the
notification, when vxlan_vni_update_group() fails, before this path
switches to GFP_NOWAIT?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014123814.388557-1-alishmery18%40gmail.com
