[PATCH net-next v4 0/7] vxlan: vnifilter: bound a single request and account per-VNI memory

[PATCH net-next v4 0/7] vxlan: vnifilter: bound a single request and account per-VNI memory

[Ali Firas]

The VNI filter interface accepts a START/END range with no bound on
either endpoint and no bound on how many VNIs one message may ask for,
and the memory it allocates per VNI is not charged to the caller's
cgroup.

Patch 1 links the VXLAN_VNIFILTER_ENTRY nest to its policy with
NLA_POLICY_NESTED(), as Jakub asked in v2:

  https://lore.kernel.org/netdev/20260921150904.65a704eb@kernel.org/

Without it the entry attributes were validated only as each entry was
dispatched, so a multi-entry message whose later entry was invalid had
the earlier entries applied and notified before the message was
rejected. With the nest linked, an entry the policy rejects refuses the
message as a unit; faults the policy does not cover are still handled
one entry at a time, as before.

Patch 2 range-validates both endpoints to the 24-bit VNI space.

Patch 3 makes the dump re-enterable, and does so before patch 4 caps the
input, so no commit in the series reports a range its own input path
would reject. vxlan_vnifilter_dump_dev() coalesces a contiguous run of
VNIs, and packs as many entries into a netlink message as the skb holds,
with no bound on either. It introduces VXLAN_VNI_FILTER_MSG_MAX and
bounds both: it ends a coalesced run at the limit, and carries a
per-message VNI budget that ends the message once its entries would sum
past the limit, the same way the loop already ends it when the skb is
full. The same resume path (cb->args[1], counting nodes dumped) drives
both; the dump does not drop or repeat a VNI.

Patch 4 caps the number of VNIs one request may add or delete, summed
over its entries, at VXLAN_VNI_FILTER_MSG_MAX. Both add and delete walk
the span one VNI at a time under rtnl_lock, so both are bounded; that
walk is the cost, not the memory.

Patch 5 charges the per-VNI node and its per-CPU stats block to the
cgroup of the task that created the VNI, the way the device's own struct
and its Tx/Rx queues already are (commit c948f51c1654 ("memcg: enable
accounting for net_device and Tx/Rx queues")).

Patch 6 charges the default FDB entry a VNI with a group or remote
installs -- the vxlan_fdb, its vxlan_rdst and the per-CPU dst cache. It
threads a gfp argument through vxlan_fdb_update() down to
vxlan_fdb_alloc()/vxlan_fdb_append(), on both the create and the
remote-change paths, and a second gfp through
vxlan_update_default_fdb_entry() so each caller picks it. A per-request
add or change (RTM_NEWTUNNEL) passes GFP_NOWAIT | __GFP_ACCOUNT, which
is non-blocking like GFP_ATOMIC but hard-bounded by the memcg: at its
limit the request fails with -ENOMEM instead of charging past
memory.max, and because the new entry is allocated before the old is
freed, a failed add leaves the existing entry intact. The device-wide
default-remote change (RTM_NEWLINK, via vxlan_vnilist_update_group())
keeps GFP_ATOMIC | __GFP_ACCOUNT; that pre-existing walk is not bounded
here ("Not addressed" below). Every non-vnifilter caller --
vxlan_fdb_add(), vxlan_snoop(), vxlan_dev_create(), vxlan_changelink()
and the switchdev vxlan_fdb_external_learn_add() -- keeps plain
GFP_ATOMIC, so the data path and unrelated callers are untouched.

Patch 7 adds the selftests.

This is the reasoning of commit e2c8d550a973 ("netfilter: ebtables:
account ebt_table_info to kmemcg") and of the net_device accounting
above: memory whose lifetime is tied to the net namespace rather than to
the allocating task cannot be relieved by the OOM killer, so a
container's netadmin must not be able to grow it outside the memcg that
bounds the container.

What this series does and does not account, measured on production
builds (no lockdep/KASAN) in a memcg at 2 and 8 vCPU:

  - every persistent per-VNI allocation the vnifilter path creates is
    attributed to the cgroup of the task whose request allocates it, and
    bounded by it on the per-request path. Patch 5's node (128 B) and
    per-CPU stats (64 B/CPU), and patch 6's FDB entry -- vxlan_fdb
    (128 B), vxlan_rdst (96 B) and a per-CPU dst cache -- on the
    per-request path (RTM_NEWTUNNEL) are hard-bounded: the node and stats
    use GFP_KERNEL_ACCOUNT and the FDB entry uses
    GFP_NOWAIT | __GFP_ACCOUNT, so a cgroup at its limit fails the
    create, the first remote, or a remote change with -ENOMEM, and
    memory.current does not exceed memory.max (measured at 2 and 8 vCPU
    in an 8 MiB cgroup). Failure is clean: the new entry is allocated
    before the old is freed, so a failed add leaves the existing entry
    intact and returns the error -- no established state is destroyed and
    no false success is returned. The at-rest footprint is one node, its
    stats and one FDB entry per VNI; this memory has device and net
    namespace lifetime, not task lifetime (killing the cgroup's tasks did
    not release it, only deleting the device did), so it must be charged
    rather than left to the OOM killer;
  - the rhashtable bucket tables backing the VNI and FDB hashes are not
    charged. That is ~32 bytes per VNI across both tables, generic
    infrastructure shared by every rhashtable user, O(entries) with
    automatic shrinking (an add/delete cycle returns it, measured), and
    the series does not address it.

The cap is symmetric, applied to add and delete alike, which is what Ido
asked for when he agreed to a 4k limit. Patch 3 is what makes that safe:
no dumped message carries more VNIs than one request may ask for, so a
device holding more than the limit can still be torn down by replaying
what "bridge vni show" reports. Device teardown itself
(vxlan_vnigroup_uninit()) is not a netlink message and is not subject to
the cap.

uAPI changes, all of them:

  - a VNI at or above 2^24, previously accepted and stored (and
    truncated on the wire), is now rejected with -ERANGE;
  - a single request asking to add or delete more than 4096 VNIs, summed
    over its entries, is now rejected with -EINVAL -- "bridge vni add
    dev X vni 1-10000" used to be accepted;
  - a request rejected by the entry policy, aimed at a missing or
    non-vnifilter device, now returns that policy error rather than
    -ENODEV or -EOPNOTSUPP, because the nest is validated before the
    device is resolved (patch 1);
  - "bridge vni show" now splits a contiguous run longer than the limit
    into several entries, and across several messages, instead of one;
    the set of VNIs it reports is unchanged;
  - in a cgroup at its memory limit, adding a VNI, or adding or changing
    a VNI's remote (RTM_NEWTUNNEL), now returns -ENOMEM instead of
    charging past the limit; the existing entry, if any, is left intact.

This is a policy and hardening change, not a fix for a crash or
corruption; it targets net-next and should not be backported.

One pre-existing semantic is worth review: an entry that carries only
END and no START is treated as the range [0, END], so a lone END near
the top of the space is a whole-space request that the cap now rejects;
whether END-without-START should mean that is left as an open question.

Not addressed here:

  - the device-wide default-remote change (RTM_NEWLINK) reaches the same
    per-VNI FDB entries outside the request cap and force-charges them
    rather than hard-bounding, so this series narrows the exposure to
    the per-request route rather than closing it. Patch 6 does charge
    those entries to the requesting task's cgroup, where before the
    series they grew unaccounted host memory; bounding the walk is left
    to a follow-up.
  - vxlan_vni_add_del() still leaves the earlier VNIs of a range
    installed if an allocation fails partway through it; with the cap
    that is now bounded to fewer than 4096 VNIs. A fix needs a
    transaction and is left to a follow-up.
  - the rhashtable bucket memory described above.

Changes since v3:
  - rebased onto current net-next.
  - the dump patch (now patch 3) is ordered before the input cap (patch
    4) and introduces VXLAN_VNI_FILTER_MSG_MAX, so every commit is
    self-consistent; it also carries a per-message VNI budget, not only a
    per-entry clamp, so a dump of a run longer than the limit splits into
    messages the input path accepts (demonstrated with raw netlink).
  - new patch 6 accounts the default FDB entry, scoped to the vnifilter
    call site via a gfp argument so the data path stays on GFP_ATOMIC;
    the gfp is threaded on both the create and the remote-change paths.
    The per-request path (RTM_NEWTUNNEL) uses GFP_NOWAIT | __GFP_ACCOUNT
    so the charge is hard-bounded (fails with -ENOMEM at the cgroup
    limit); the pre-existing device-default walk (RTM_NEWLINK) keeps
    GFP_ATOMIC | __GFP_ACCOUNT.
  - patch 5 reworded to cover the node and stats only, with its
    net_device-accounting reference narrowed to net_device and its Tx/Rx
    queues.
  - patch 1 reworded to scope the "rejected as a unit" claim to entries
    the policy rejects.

Changes since v2:
  - add patch 1 linking the nest to its policy (Jakub);
  - the request cap applies to both add and delete (Ido).

Ali Firas (7):
  vxlan: vnifilter: validate the VXLAN_VNIFILTER_ENTRY nest
  vxlan: vnifilter: reject VNIs outside the 24-bit space
  vxlan: vnifilter: clamp the dumped VNI range to the request limit
  vxlan: vnifilter: bound the number of VNIs one request may touch
  vxlan: vnifilter: account per-VNI memory to memcg
  vxlan: vnifilter: account the default FDB entry to memcg
  selftests: net: test the vxlan vnifilter request limit and dump replay

 drivers/net/vxlan/vxlan_core.c                |  47 +++---
 drivers/net/vxlan/vxlan_private.h             |   5 +-
 drivers/net/vxlan/vxlan_vnifilter.c           | 132 +++++++++++++++---
 .../selftests/net/test_vxlan_vnifiltering.sh  | 119 +++++++++++++++-
 4 files changed, 270 insertions(+), 46 deletions(-)


base-commit: 05b5019b01fb9443b09ae4ed34af1135a80a84c8
--
2.53.0

[PATCH net-next v4 1/7] vxlan: vnifilter: validate the VXLAN_VNIFILTER_ENTRY nest

[Ali Firas]

vxlan_vnifilter_process() parses the message with vni_filter_policy,
whose VXLAN_VNIFILTER_ENTRY is a bare NLA_NESTED with no nested policy
attached. The entry attributes are therefore validated only later, one
entry at a time, by the nla_parse_nested() inside
vxlan_process_vni_filter(). Entries are parsed as they are dispatched:
in a message whose first entry is valid and whose second is not, the
first entry is applied and its RTM_NEWTUNNEL notification sent before
the second is rejected.

Link the nest to vni_filter_entry_policy with NLA_POLICY_NESTED() so
every entry is checked against the entry policy up front, before any
entry is acted on. A message whose entry the policy rejects -- a
malformed attribute, or, with the next patch, an out-of-range endpoint
-- is now refused as a unit and installs nothing. Faults the policy does
not cover, such as a device or handler error raised on a later entry,
are unchanged: entries are still dispatched and applied one at a time.

This reorders two faults. The nest is validated before the device is
looked up and before the vnifilter flag is checked, so a request
rejected by the entry policy, aimed at a missing or non-vnifilter
device, now returns that policy error where it previously returned
-ENODEV or -EOPNOTSUPP. The request was invalid either way; only the
errno an operator sees changes.

The nla_parse_nested() in vxlan_process_vni_filter() stays: it is what
fills the per-entry attribute table the handler reads. It can no longer
fail on a message that has reached it.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Link: https://lore.kernel.org/netdev/20260921150904.65a704eb@kernel.org/
Assisted-by: LLM
Signed-off-by: Ali Firas <alishmery18@gmail.com>
---
v4: scope the "rejected as a unit" claim to
policy-rejected entries; rebased onto net-next.

 drivers/net/vxlan/vxlan_vnifilter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
index 41095096b698..124ba47758e9 100644
--- a/drivers/net/vxlan/vxlan_vnifilter.c
+++ b/drivers/net/vxlan/vxlan_vnifilter.c
@@ -499,7 +499,7 @@ static const struct nla_policy vni_filter_entry_policy[VXLAN_VNIFILTER_ENTRY_MAX
 };
 
 static const struct nla_policy vni_filter_policy[VXLAN_VNIFILTER_MAX + 1] = {
-	[VXLAN_VNIFILTER_ENTRY] = { .type = NLA_NESTED },
+	[VXLAN_VNIFILTER_ENTRY] = NLA_POLICY_NESTED(vni_filter_entry_policy),
 };
 
 bool vxlan_vnifilter_has_multicast(const struct vxlan_dev *vxlan)

base-commit: 05b5019b01fb9443b09ae4ed34af1135a80a84c8
-- 
2.53.0

[PATCH net-next v4 2/7] vxlan: vnifilter: reject VNIs outside the 24-bit space

[Ali Firas]

VXLAN_VNIFILTER_ENTRY_START and VXLAN_VNIFILTER_ENTRY_END are bare
NLA_U32, so neither is bounded before vxlan_process_vni_filter() hands
them to vxlan_vni_add_del():

	int v, err = 0;
	...
	for (v = start_vni; v <= end_vni; v++)

v is int and end_vni is __u32, so the comparison is done unsigned. A
request carrying only START=0xffffffff has vni_start == vni_end ==
0xffffffff and looks like a single VNI; v is then -1, the comparison
promotes it to 0xffffffff and passes, v++ makes v 0, and the loop walks
the space upwards from there, creating a VNI node and a per-CPU stats
block per iteration under rtnl_lock. Any range ending at 0xffffffff
behaves the same way.

Separately, a VNI at or above VXLAN_N_VID is accepted and stored even
though the VXLAN header carries only 24 bits: vxlan_vni_field() shifts
without masking, so such an entry keeps its own rhashtable slot while
being truncated on the wire.

Range-validate both attributes against the 24-bit space, as vxlan_mdb.c
already does for its own VNI attributes. With the nest now linked to
this policy, an out-of-range endpoint is rejected during netlink policy
validation, before vxlan_process_vni_filter() runs and before anything
is allocated. How many VNIs a single in-range request may span is a
separate question, bounded by the next patch; that limit is not a policy
check and does run in the handler.

Make the loop counter u32 as well. With the range bounded it is no
longer what keeps the loop finite, but it drops the undefined signed
overflow past INT_MAX and matches the u32 vni that vxlan_vni_add() and
vxlan_vni_del() already take.

Assisted-by: LLM
Signed-off-by: Ali Firas <alishmery18@gmail.com>
---
v4: no functional change; rebased onto net-next.

 drivers/net/vxlan/vxlan_vnifilter.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
index 124ba47758e9..9e15f4de75c9 100644
--- a/drivers/net/vxlan/vxlan_vnifilter.c
+++ b/drivers/net/vxlan/vxlan_vnifilter.c
@@ -491,9 +491,15 @@ static int vxlan_vnifilter_dump(struct sk_buff *skb, struct netlink_callback *cb
 	return err;
 }
 
+static const struct netlink_range_validation vni_filter_vni_range = {
+	.max = VXLAN_N_VID - 1,
+};
+
 static const struct nla_policy vni_filter_entry_policy[VXLAN_VNIFILTER_ENTRY_MAX + 1] = {
-	[VXLAN_VNIFILTER_ENTRY_START] = { .type = NLA_U32 },
-	[VXLAN_VNIFILTER_ENTRY_END] = { .type = NLA_U32 },
+	[VXLAN_VNIFILTER_ENTRY_START] = NLA_POLICY_FULL_RANGE(NLA_U32,
+							      &vni_filter_vni_range),
+	[VXLAN_VNIFILTER_ENTRY_END] = NLA_POLICY_FULL_RANGE(NLA_U32,
+							    &vni_filter_vni_range),
 	[VXLAN_VNIFILTER_ENTRY_GROUP]	= NLA_POLICY_EXACT_LEN(sizeof_field(struct iphdr, daddr)),
 	[VXLAN_VNIFILTER_ENTRY_GROUP6]	= NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
 };
@@ -914,7 +920,8 @@ static int vxlan_vni_add_del(struct vxlan_dev *vxlan, __u32 start_vni,
 			     int cmd, struct netlink_ext_ack *extack)
 {
 	struct vxlan_vni_group *vg;
-	int v, err = 0;
+	int err = 0;
+	u32 v;
 
 	vg = rtnl_dereference(vxlan->vnigrp);
 
-- 
2.53.0

Re: [PATCH net-next v4 2/7] vxlan: vnifilter: reject VNIs outside the 24-bit space

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch fixes long-standing defects in vxlan_vni_add_del() and
  vni_filter_entry_policy, but it has no Fixes: tag and is posted to…

--

vxlan: vnifilter: reject VNIs outside the 24-bit space

This patch adds NLA_POLICY_FULL_RANGE() checks to
VXLAN_VNIFILTER_ENTRY_START and VXLAN_VNIFILTER_ENTRY_END, which limits
both to VXLAN_N_VID - 1. It also changes the loop counter in
vxlan_vni_add_del() from int to u32, so a range that ends at 0xffffffff
can no longer wrap.

> How many VNIs a single in-range request may span is a
> separate question, bounded by the next patch; that limit is not a policy
> check and does run in the handler.

[ ... ]

> Assisted-by: LLM
> Signed-off-by: Ali Firas <alishmery18@gmail.com>
>
> ---
> v4: no functional change; rebased onto net-next.

[Severity: Low]
Should this patch have a Fixes: tag and go to net instead of net-next?

The int loop counter and the plain NLA_U32 policy entries seem to date
from commit f9c4bb0b245c ("vxlan: vni filtering support on collect
metadata device"), which was merged in v5.18:

    int v, err = 0;
    ...
    for (v = start_vni; v <= end_vni; v++) {

Since then, a request that carries only
VXLAN_VNIFILTER_ENTRY_START=0xffffffff passes this check in
vxlan_process_vni_filter():

    if (!vni_start && !vni_end) {

The loop in vxlan_vni_add_del() then visits 0xffffffff, 0, 1, and so on.
Each pass allocates a vxlan_vni_node and a per-CPU stats block under
rtnl_lock, and the loop only stops on an error such as -E2BIG from the
rhashtable or -ENOMEM. VNIs at or above VXLAN_N_VID are also stored,
while vxlan_vni_field() truncates them on the wire.

Anyone with CAP_NET_ADMIN over the netns can reach this, including root
in a user namespace.

Without a Fixes: tag, will the 24-bit aliasing fix and the signed
overflow fix reach stable kernels?

A backport would also need the preceding commit "vxlan: vnifilter:
validate the VXLAN_VNIFILTER_ENTRY nest". Without its NLA_POLICY_NESTED()
link, the new range policy is only applied per entry, by the
nla_parse_nested() call in vxlan_process_vni_filter().

The commit message also says the span is "bounded by the next patch".
The next patch in the series looks like "vxlan: vnifilter: clamp the
dumped VNI range to the request limit". The cap itself is in "vxlan:
vnifilter: bound the number of VNIs one request may touch", and that
patch says it is not a backport candidate.

Could the reference be corrected so backporters get accurate dependency
information?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014123814.388557-1-alishmery18%40gmail.com

[PATCH net-next v4 3/7] vxlan: vnifilter: clamp the dumped VNI range to the request limit

[Ali Firas]

vxlan_vnifilter_dump_dev() coalesces a contiguous run of VNIs sharing a
remote into one VXLAN_VNIFILTER_ENTRY, and packs as many entries into a
netlink message as the skb holds, with no bound on either. A later patch
bounds a single request to VXLAN_VNI_FILTER_MSG_MAX VNIs summed over its
entries; without a matching bound on the dump, a device holding more than
that -- populated by several requests -- would dump a run, or a message,
that the input path then refuses on replay.

Introduce VXLAN_VNI_FILTER_MSG_MAX here and clamp the dump to it first, so
the series never passes through a commit where the kernel reports a range
its own input path rejects. Bound the two ways the dump can exceed the
limit:

  - end a coalesced run once it reaches the limit, in the merge condition,
    even when the next VNI is contiguous and shares the remote; and
  - carry a per-message VNI budget, ending the message once its entries
    would sum past the limit, the same way the loop already ends it when
    the skb is full.

cb->args[1] continues to count the VNI nodes already dumped and is advanced
only when a completed entry is written, so the budget break and the
skb-full break share one resume path; a run stays a gapless block, so the
dump never drops a VNI and never emits one twice.

Assisted-by: LLM
Signed-off-by: Ali Firas <alishmery18@gmail.com>
---
v4: moved before the input cap; now introduces
VXLAN_VNI_FILTER_MSG_MAX and adds a per-message VNI budget (v3 clamped
per entry only), so the dump never emits a message the input path
rejects.

 drivers/net/vxlan/vxlan_vnifilter.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
index 9e15f4de75c9..5beec29eb84a 100644
--- a/drivers/net/vxlan/vxlan_vnifilter.c
+++ b/drivers/net/vxlan/vxlan_vnifilter.c
@@ -17,6 +17,13 @@
 
 #include "vxlan_private.h"
 
+/* The dump coalesces a contiguous run of VNIs into entries, and packs entries
+ * into netlink messages, of at most this many VNIs each, so every message the
+ * dump emits stays within what the input path will accept on replay. A later
+ * patch bounds a single RTM_NEWTUNNEL/RTM_DELTUNNEL request to the same number.
+ */
+#define VXLAN_VNI_FILTER_MSG_MAX	4096
+
 static inline int vxlan_vni_cmp(struct rhashtable_compare_arg *arg,
 				const void *ptr)
 {
@@ -342,6 +349,7 @@ static int vxlan_vnifilter_dump_dev(const struct net_device *dev,
 	struct nlmsghdr *nlh;
 	int idx = 0, s_idx;
 	bool dump_stats;
+	u32 vnis = 0;
 	int err = 0;
 
 	if (cb->args[2] != dev->ifindex) {
@@ -388,15 +396,29 @@ static int vxlan_vnifilter_dump_dev(const struct net_device *dev,
 			continue;
 		}
 		if (!dump_stats && vnirange(vend, v) == 1 &&
+		    vnirange(vbegin, v) < VXLAN_VNI_FILTER_MSG_MAX &&
 		    vxlan_addr_equal(&v->remote_ip, &vend->remote_ip)) {
 			goto update_end;
 		} else {
+			u32 sz = vnirange(vbegin, vend) + 1;
+
+			/* End this message on its VNI budget the same way we
+			 * end it when the skb is full, so a dumped message
+			 * never sums to more VNIs than the input path accepts
+			 * in one request. Resume re-emits this run as the
+			 * first entry of the next message.
+			 */
+			if (vnis + sz > VXLAN_VNI_FILTER_MSG_MAX) {
+				err = -EMSGSIZE;
+				break;
+			}
 			if (!vxlan_fill_vni_filter_entry(skb, vbegin, vend,
 							 dump_stats)) {
 				err = -EMSGSIZE;
 				break;
 			}
-			idx += vnirange(vbegin, vend) + 1;
+			vnis += sz;
+			idx += sz;
 			vbegin = v;
 		}
 update_end:
@@ -404,7 +426,10 @@ static int vxlan_vnifilter_dump_dev(const struct net_device *dev,
 	}
 
 	if (!err && vbegin) {
-		if (!vxlan_fill_vni_filter_entry(skb, vbegin, vend, dump_stats))
+		if (vnis + vnirange(vbegin, vend) + 1 > VXLAN_VNI_FILTER_MSG_MAX)
+			err = -EMSGSIZE;
+		else if (!vxlan_fill_vni_filter_entry(skb, vbegin, vend,
+						      dump_stats))
 			err = -EMSGSIZE;
 	}
 
-- 
2.53.0

Re: [PATCH net-next v4 3/7] vxlan: vnifilter: clamp the dumped VNI range to the request limit

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The comment above VXLAN_VNI_FILTER_MSG_MAX in
  drivers/net/vxlan/vxlan_vnifilter.c (lines 20-24) describes the order…

--

vxlan: vnifilter: clamp the dumped VNI range to the request limit

This commit adds VXLAN_VNI_FILTER_MSG_MAX and limits
vxlan_vnifilter_dump_dev() in two ways. A coalesced run of VNIs ends once
it reaches the limit, and each netlink message has a VNI budget. As a
result, no dumped message holds more VNIs than the input path accepts in
one request after a later patch in the series.

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index 9e15f4de75c99..5beec29eb84a8 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c
> @@ -17,6 +17,13 @@
>  
>  #include "vxlan_private.h"
>  
> +/* The dump coalesces a contiguous run of VNIs into entries, and packs entries
> + * into netlink messages, of at most this many VNIs each, so every message the
> + * dump emits stays within what the input path will accept on replay. A later
> + * patch bounds a single RTM_NEWTUNNEL/RTM_DELTUNNEL request to the same number.
> + */
> +#define VXLAN_VNI_FILTER_MSG_MAX	4096

[Severity: Low]
This isn't a bug, but will the "A later patch bounds a single
RTM_NEWTUNNEL/RTM_DELTUNNEL request to the same number" sentence go stale
once the series is merged?

At the end of the series this comment is unchanged. By then, the commit
"vxlan: vnifilter: bound the number of VNIs one request may touch" has
added the request cap. vxlan_vnifilter_process() calls
vxlan_vnifilter_check_msg(), which does this:

    if (vnis > VXLAN_VNI_FILTER_MSG_MAX) {

Once merged, "a later patch" no longer points to anything. The comment
also doesn't say where the input-side limit is enforced.

Could the comment describe the invariant instead? For example, "must
match the per-request limit enforced by vxlan_vnifilter_check_msg()".
Another option is to move that sentence into the patch that adds the
cap.

The dump and input paths agree in the final tree, so this is only about
the comment text.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014123814.388557-1-alishmery18%40gmail.com

[PATCH net-next v4 4/7] vxlan: vnifilter: bound the number of VNIs one request may touch

[Ali Firas]

A single RTM_NEWTUNNEL or RTM_DELTUNNEL message can ask for the whole
24-bit space in one range. vxlan_vni_add_del() then walks it one VNI at
a time under rtnl_lock, holding the lock for the length of the walk.
That walk is the cost being bounded, not the memory: an add allocates a
VNI node and a per-CPU stats block per VNI, a delete creates neither,
but both walk the same span and hold rtnl the same way, so both are
bounded.

The span of one VXLAN_VNIFILTER_ENTRY is not the quantity to bound. An
entry carrying just START and END is 20 bytes on the wire and a message
may carry many of them, vxlan_process_vni_filter() being called once per
entry, so bounding each entry alone would still let one message ask for
thousands of times the limit. Sum the spans of every entry and reject
the message as a whole in vxlan_vnifilter_check_msg(), before the
dispatch loop: entries are applied and notified one at a time, so a
limit enforced during dispatch would return an error only after every
preceding entry had been acted on. The range extraction is factored
into vxlan_vni_filter_entry_range() so the count and the range later
acted on cannot drift.

The limit, VXLAN_VNI_FILTER_MSG_MAX (4096), is the one the preceding
patch introduced for the dump; it follows the usable VLAN ID space,
since vnifilter is mainly used on bridged devices where the VNI is
derived from the VLAN. It bounds one request, not how many VNIs a
device may hold: the whole space can still be installed in more than
one request, and a
device holding it is still torn down in one step by
vxlan_vnigroup_uninit(), which is device teardown, not a netlink
message, and is not subject to this cap.

This is a policy narrowing of what a single message may ask for, not a
fix for a crash or corruption, and is not a backport candidate.

Suggested-by: Ido Schimmel <idosch@nvidia.com>
Assisted-by: LLM
Signed-off-by: Ali Firas <alishmery18@gmail.com>
---
v4: use the constant the dump patch introduces; rebased.

 drivers/net/vxlan/vxlan_vnifilter.c | 80 ++++++++++++++++++++++++++---
 1 file changed, 72 insertions(+), 8 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
index 5beec29eb84a..2999dd536625 100644
--- a/drivers/net/vxlan/vxlan_vnifilter.c
+++ b/drivers/net/vxlan/vxlan_vnifilter.c
@@ -971,13 +971,79 @@ static int vxlan_vni_add_del(struct vxlan_dev *vxlan, __u32 start_vni,
 	return err;
 }
 
+/* Derive the VNI range one VXLAN_VNIFILTER_ENTRY selects. Shared so that the
+ * count taken by vxlan_vnifilter_check_msg() cannot drift from the range
+ * vxlan_process_vni_filter() then acts on.
+ */
+static void vxlan_vni_filter_entry_range(struct nlattr **vattrs, u32 *vni_start,
+					 u32 *vni_end)
+{
+	*vni_start = 0;
+	*vni_end = 0;
+
+	if (vattrs[VXLAN_VNIFILTER_ENTRY_START]) {
+		*vni_start = nla_get_u32(vattrs[VXLAN_VNIFILTER_ENTRY_START]);
+		*vni_end = *vni_start;
+	}
+
+	if (vattrs[VXLAN_VNIFILTER_ENTRY_END])
+		*vni_end = nla_get_u32(vattrs[VXLAN_VNIFILTER_ENTRY_END]);
+}
+
+/* Reject a request touching more than VXLAN_VNI_FILTER_MSG_MAX VNIs before any
+ * of its entries is acted on. Both add and delete walk the span one VNI at a
+ * time under rtnl_lock, so both are bounded. Entries are applied one at a time
+ * and each one notifies as it goes, so a limit checked inside the dispatch loop
+ * would leave the entries ahead of the offending one already applied.
+ */
+static int vxlan_vnifilter_check_msg(const struct nlmsghdr *nlh,
+				     struct netlink_ext_ack *extack)
+{
+	struct nlattr *vattrs[VXLAN_VNIFILTER_ENTRY_MAX + 1];
+	struct nlattr *attr;
+	u32 vnis = 0;
+	int err, rem;
+
+	nlmsg_for_each_attr_type(attr, VXLAN_VNIFILTER_ENTRY, nlh,
+				 sizeof(struct tunnel_msg), rem) {
+		u32 vni_start, vni_end;
+
+		err = nla_parse_nested(vattrs, VXLAN_VNIFILTER_ENTRY_MAX, attr,
+				       vni_filter_entry_policy, extack);
+		if (err)
+			return err;
+
+		vxlan_vni_filter_entry_range(vattrs, &vni_start, &vni_end);
+
+		/* A start above the end selects no VNI and costs nothing;
+		 * leave it behaving as it does today.
+		 */
+		if (vni_end < vni_start)
+			continue;
+
+		/* vni_filter_entry_policy has already bounded both endpoints
+		 * below VXLAN_N_VID, so one entry adds at most VXLAN_N_VID and
+		 * vnis cannot wrap before the test below rejects it.
+		 */
+		vnis += vni_end - vni_start + 1;
+		if (vnis > VXLAN_VNI_FILTER_MSG_MAX) {
+			NL_SET_ERR_MSG_ATTR_FMT(extack, attr,
+						"Request asks for more than %u VNIs",
+						VXLAN_VNI_FILTER_MSG_MAX);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static int vxlan_process_vni_filter(struct vxlan_dev *vxlan,
 				    struct nlattr *nlvnifilter,
 				    int cmd, struct netlink_ext_ack *extack)
 {
 	struct nlattr *vattrs[VXLAN_VNIFILTER_ENTRY_MAX + 1];
 	const struct vxlan_config *cfg;
-	u32 vni_start = 0, vni_end = 0;
+	u32 vni_start, vni_end;
 	union vxlan_addr group;
 	int err;
 
@@ -988,13 +1054,7 @@ static int vxlan_process_vni_filter(struct vxlan_dev *vxlan,
 	if (err)
 		return err;
 
-	if (vattrs[VXLAN_VNIFILTER_ENTRY_START]) {
-		vni_start = nla_get_u32(vattrs[VXLAN_VNIFILTER_ENTRY_START]);
-		vni_end = vni_start;
-	}
-
-	if (vattrs[VXLAN_VNIFILTER_ENTRY_END])
-		vni_end = nla_get_u32(vattrs[VXLAN_VNIFILTER_ENTRY_END]);
+	vxlan_vni_filter_entry_range(vattrs, &vni_start, &vni_end);
 
 	if (!vni_start && !vni_end) {
 		NL_SET_ERR_MSG_ATTR(extack, nlvnifilter,
@@ -1104,6 +1164,10 @@ static int vxlan_vnifilter_process(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (!(cfg->flags & VXLAN_F_VNIFILTER))
 		return -EOPNOTSUPP;
 
+	err = vxlan_vnifilter_check_msg(nlh, extack);
+	if (err)
+		return err;
+
 	nlmsg_for_each_attr_type(attr, VXLAN_VNIFILTER_ENTRY, nlh,
 				 sizeof(*tmsg), rem) {
 		err = vxlan_process_vni_filter(vxlan, attr, nlh->nlmsg_type,
-- 
2.53.0

Re: [PATCH net-next v4 4/7] vxlan: vnifilter: bound the number of VNIs one request may touch

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The comment above VXLAN_VNI_FILTER_MSG_MAX in
  drivers/net/vxlan/vxlan_vnifilter.c says: "A later patch bounds a…

--

vxlan: vnifilter: bound the number of VNIs one request may touch

This commit adds vxlan_vnifilter_check_msg(). It adds up the VNI spans of
every VXLAN_VNIFILTER_ENTRY in an RTM_NEWTUNNEL or RTM_DELTUNNEL request.
If the total is more than VXLAN_VNI_FILTER_MSG_MAX, the whole message is
rejected before any entry is applied. The range extraction is moved into
vxlan_vni_filter_entry_range() so the check and the dispatch path use the
same logic.

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index 5beec29eb84a8..2999dd536625e 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c

[ ... ]

> +/* Reject a request touching more than VXLAN_VNI_FILTER_MSG_MAX VNIs before any
> + * of its entries is acted on. Both add and delete walk the span one VNI at a
> + * time under rtnl_lock, so both are bounded. Entries are applied one at a time
> + * and each one notifies as it goes, so a limit checked inside the dispatch loop
> + * would leave the entries ahead of the offending one already applied.
> + */
> +static int vxlan_vnifilter_check_msg(const struct nlmsghdr *nlh,
> +				     struct netlink_ext_ack *extack)

[Severity: Low]
This isn't a bug, but should the comment above VXLAN_VNI_FILTER_MSG_MAX
be updated now that this function exists?

The preceding patch added this to the top of vxlan_vnifilter.c:

/* The dump coalesces a contiguous run of VNIs into entries, and packs entries
 * into netlink messages, of at most this many VNIs each, so every message the
 * dump emits stays within what the input path will accept on replay. A later
 * patch bounds a single RTM_NEWTUNNEL/RTM_DELTUNNEL request to the same number.
 */
#define VXLAN_VNI_FILTER_MSG_MAX	4096

This commit is the later patch that sentence refers to. Once it is
applied, the comment still describes the per-request limit as future work.
The text is also the same at the end of the series.

Could the last sentence say that vxlan_vnifilter_check_msg() applies the
same bound to a single request?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014123814.388557-1-alishmery18%40gmail.com

[PATCH net-next v4 5/7] vxlan: vnifilter: account per-VNI memory to memcg

[Ali Firas]

vxlan_vni_alloc() allocates a struct vxlan_vni_node and a per-CPU stats
block for every VNI, both with plain GFP_KERNEL, so neither is charged
to the cgroup of the process that created the VNI. A netdevice's own
struct and its Tx/Rx queues are already GFP_KERNEL_ACCOUNT in
alloc_netdev_mqs() (commit c948f51c1654 ("memcg: enable accounting for
net_device and Tx/Rx queues")), so these per-VNI allocations are an
unaccounted part of a device that a cgroup can grow one VNI at a time.

Charge both to the caller's cgroup with GFP_KERNEL_ACCOUNT in
vxlan_vni_alloc(). __GFP_ACCOUNT only charges an allocation made from a
task in a non-root memcg, so the sole caller, which already handles a
NULL return, is unaffected outside a memcg.

The charge lands in the caller's memcg: adding VNIs from inside one
moves each node into the accounted slab and raises the cgroup's per-CPU
memory. This covers the node and its stats only; a VNI carrying a group
or remote also installs a default FDB entry, whose allocations are not
charged here and are accounted by the next patch.

Assisted-by: LLM
Signed-off-by: Ali Firas <alishmery18@gmail.com>
---
v4: reworded to cover the node and stats only; the
c948f51c1654 reference narrowed to net_device and its Tx/Rx queues;
rebased.

 drivers/net/vxlan/vxlan_vnifilter.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
index 2999dd536625..4a631fcdf23a 100644
--- a/drivers/net/vxlan/vxlan_vnifilter.c
+++ b/drivers/net/vxlan/vxlan_vnifilter.c
@@ -825,10 +825,11 @@ static struct vxlan_vni_node *vxlan_vni_alloc(struct vxlan_dev *vxlan,
 {
 	struct vxlan_vni_node *vninode;
 
-	vninode = kzalloc_obj(*vninode);
+	vninode = kzalloc_obj(*vninode, GFP_KERNEL_ACCOUNT);
 	if (!vninode)
 		return NULL;
-	vninode->stats = netdev_alloc_pcpu_stats(struct vxlan_vni_stats_pcpu);
+	vninode->stats = __netdev_alloc_pcpu_stats(struct vxlan_vni_stats_pcpu,
+						   GFP_KERNEL_ACCOUNT);
 	if (!vninode->stats) {
 		kfree(vninode);
 		return NULL;
-- 
2.53.0

[PATCH net-next v4 6/7] vxlan: vnifilter: account the default FDB entry to memcg

[Ali Firas]

A VNI added through the vnifilter interface with a group or remote
installs a default FDB entry for that VNI: vxlan_vni_update_group()
calls vxlan_update_default_fdb_entry(), which calls vxlan_fdb_update()
and vxlan_fdb_create(). The entry allocates a struct vxlan_fdb and, per
remote, a struct vxlan_rdst plus its per-CPU dst cache, all with
GFP_ATOMIC and none charged to a memory cgroup.

The previous patch accounts the per-VNI node and its stats; these FDB
objects are the rest of what one vnifilter request allocates per VNI.
Charge them to the caller, as commit c948f51c1654 ("memcg: enable
accounting for net_device and Tx/Rx queues") and commit e2c8d550a973
("netfilter: ebtables: account ebt_table_info to kmemcg") do: the
lifetime of this memory is the net namespace, not the allocating task,
so the OOM killer cannot reclaim it and it must be charged to the cgroup
of the task whose request allocates it.

Thread a gfp argument through vxlan_fdb_update() to
vxlan_fdb_alloc()/vxlan_fdb_append() on both paths it can take -- the
create path (vxlan_fdb_update_create() -> vxlan_fdb_create()) and the
path that changes an existing entry's remote
(vxlan_fdb_update_existing()) -- and a second gfp through
vxlan_update_default_fdb_entry(), so each caller chooses it. Every
non-vnifilter caller of vxlan_fdb_update() -- vxlan_fdb_add(),
vxlan_snoop(), vxlan_dev_create(), vxlan_changelink() and the switchdev
vxlan_fdb_external_learn_add() -- keeps plain GFP_ATOMIC, so the data
path and unrelated callers are unchanged. dst_cache_init() already takes
a gfp argument, so no change outside the driver is needed.

A per-request add or change of a VNI's remote (RTM_NEWTUNNEL, via
vxlan_vni_update_group()) passes GFP_NOWAIT | __GFP_ACCOUNT. These
allocations run under the hash spinlock and cannot sleep, but a
configuration path does not need the atomic reserves GFP_ATOMIC grants,
and GFP_NOWAIT makes the memcg charge hard-bounded rather than
force-charged: a cgroup at its limit fails the add or change there with
-ENOMEM instead of pushing memory.current past memory.max. 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). Measured on production builds
at 2 and 8 vCPU in an 8 MiB cgroup: over-limit adds and changes return
-ENOMEM and memory.current does not exceed memory.max.

The device-wide default-remote change (RTM_NEWLINK, via
vxlan_vnilist_update_group()) keeps GFP_ATOMIC | __GFP_ACCOUNT. That
walk touches every VNI inheriting the device default in one message,
outside the per-request cap, and its existing error unwind re-installs
entries; hard-bounding it would turn a cgroup limit into a mid-walk
failure that the unwind cannot cleanly reverse. Bounding and unwinding
that pre-existing walk is left to a separate change; here it stays
force-charged, as before this series, and is charged to the task
issuing the RTM_NEWLINK.

This adds no new failure path on the data or learning paths: their
allocations already existed and keep GFP_ATOMIC; only the vnifilter
configuration path gains an -ENOMEM return, which its callers already
propagate.

Assisted-by: LLM
Signed-off-by: Ali Firas <alishmery18@gmail.com>
---
v4: new patch.

 drivers/net/vxlan/vxlan_core.c      | 47 ++++++++++++++++-------------
 drivers/net/vxlan/vxlan_private.h   |  7 +++--
 drivers/net/vxlan/vxlan_vnifilter.c | 14 ++++++---
 3 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 278d41f58d89..7991b544af5a 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -577,7 +577,7 @@ static int vxlan_fdb_replace(struct vxlan_fdb *f,
 /* Add/update destinations for multicast */
 static int vxlan_fdb_append(struct vxlan_fdb *f,
 			    union vxlan_addr *ip, __be16 port, __be32 vni,
-			    __u32 ifindex, struct vxlan_rdst **rdp)
+			    __u32 ifindex, struct vxlan_rdst **rdp, gfp_t gfp)
 {
 	struct vxlan_rdst *rd;
 
@@ -585,14 +585,14 @@ static int vxlan_fdb_append(struct vxlan_fdb *f,
 	if (rd)
 		return 0;
 
-	rd = kmalloc_obj(*rd, GFP_ATOMIC);
+	rd = kmalloc_obj(*rd, gfp);
 	if (rd == NULL)
 		return -ENOMEM;
 
 	/* The driver can work correctly without a dst cache, so do not treat
 	 * dst cache initialization errors as fatal.
 	 */
-	dst_cache_init(&rd->dst_cache, GFP_ATOMIC | __GFP_NOWARN);
+	dst_cache_init(&rd->dst_cache, gfp | __GFP_NOWARN);
 
 	rd->remote_ip = *ip;
 	rd->remote_port = port;
@@ -779,11 +779,11 @@ static int vxlan_gpe_gro_complete(struct sock *sk, struct sk_buff *skb, int nhof
 
 static struct vxlan_fdb *vxlan_fdb_alloc(struct vxlan_dev *vxlan, const u8 *mac,
 					 __u16 state, __be32 src_vni,
-					 __u16 ndm_flags)
+					 __u16 ndm_flags, gfp_t gfp)
 {
 	struct vxlan_fdb *f;
 
-	f = kmalloc_obj(*f, GFP_ATOMIC);
+	f = kmalloc_obj(*f, gfp);
 	if (!f)
 		return NULL;
 	memset(&f->key, 0, sizeof(f->key));
@@ -869,7 +869,7 @@ int vxlan_fdb_create(struct vxlan_dev *vxlan,
 		     __u16 state, __be16 port, __be32 src_vni,
 		     __be32 vni, __u32 ifindex, __u16 ndm_flags,
 		     u32 nhid, struct vxlan_fdb **fdb,
-		     struct netlink_ext_ack *extack)
+		     struct netlink_ext_ack *extack, gfp_t gfp)
 {
 	const struct vxlan_config *cfg = rcu_dereference_rtnl(vxlan->cfg);
 	struct vxlan_rdst *rd = NULL;
@@ -881,14 +881,14 @@ int vxlan_fdb_create(struct vxlan_dev *vxlan,
 		return -ENOSPC;
 
 	netdev_dbg(vxlan->dev, "add %pM -> %pIS\n", mac, ip);
-	f = vxlan_fdb_alloc(vxlan, mac, state, src_vni, ndm_flags);
+	f = vxlan_fdb_alloc(vxlan, mac, state, src_vni, ndm_flags, gfp);
 	if (!f)
 		return -ENOMEM;
 
 	if (nhid)
 		rc = vxlan_fdb_nh_update(vxlan, f, nhid, extack);
 	else
-		rc = vxlan_fdb_append(f, ip, port, vni, ifindex, &rd);
+		rc = vxlan_fdb_append(f, ip, port, vni, ifindex, &rd, gfp);
 	if (rc < 0)
 		goto errout;
 
@@ -984,7 +984,7 @@ static int vxlan_fdb_update_existing(struct vxlan_dev *vxlan,
 				     __u32 ifindex, __u16 ndm_flags,
 				     struct vxlan_fdb *f, u32 nhid,
 				     bool swdev_notify,
-				     struct netlink_ext_ack *extack)
+				     struct netlink_ext_ack *extack, gfp_t gfp)
 {
 	__u16 fdb_flags = (ndm_flags & ~NTF_USE);
 	struct vxlan_rdst *rd = NULL;
@@ -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;
@@ -1088,7 +1089,7 @@ static int vxlan_fdb_update_create(struct vxlan_dev *vxlan,
 				   __be16 port, __be32 src_vni, __be32 vni,
 				   __u32 ifindex, __u16 ndm_flags, u32 nhid,
 				   bool swdev_notify,
-				   struct netlink_ext_ack *extack)
+				   struct netlink_ext_ack *extack, gfp_t gfp)
 {
 	__u16 fdb_flags = (ndm_flags & ~NTF_USE);
 	struct vxlan_fdb *f;
@@ -1101,7 +1102,7 @@ static int vxlan_fdb_update_create(struct vxlan_dev *vxlan,
 
 	netdev_dbg(vxlan->dev, "add %pM -> %pIS\n", mac, ip);
 	rc = vxlan_fdb_create(vxlan, mac, ip, state, port, src_vni,
-			      vni, ifindex, fdb_flags, nhid, &f, extack);
+			      vni, ifindex, fdb_flags, nhid, &f, extack, gfp);
 	if (rc < 0)
 		return rc;
 
@@ -1124,7 +1125,7 @@ int vxlan_fdb_update(struct vxlan_dev *vxlan,
 		     __be16 port, __be32 src_vni, __be32 vni,
 		     __u32 ifindex, __u16 ndm_flags, u32 nhid,
 		     bool swdev_notify,
-		     struct netlink_ext_ack *extack)
+		     struct netlink_ext_ack *extack, gfp_t gfp)
 {
 	struct vxlan_fdb *f;
 
@@ -1138,7 +1139,8 @@ int vxlan_fdb_update(struct vxlan_dev *vxlan,
 
 		return vxlan_fdb_update_existing(vxlan, ip, state, flags, port,
 						 vni, ifindex, ndm_flags, f,
-						 nhid, swdev_notify, extack);
+						 nhid, swdev_notify, extack,
+						 gfp);
 	} else {
 		if (!(flags & NLM_F_CREATE))
 			return -ENOENT;
@@ -1146,7 +1148,7 @@ int vxlan_fdb_update(struct vxlan_dev *vxlan,
 		return vxlan_fdb_update_create(vxlan, mac, ip, state, flags,
 					       port, src_vni, vni, ifindex,
 					       ndm_flags, nhid, swdev_notify,
-					       extack);
+					       extack, gfp);
 	}
 }
 
@@ -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);
 
 	if (!err)
@@ -1507,7 +1509,8 @@ static enum skb_drop_reason vxlan_snoop(struct net_device *dev,
 					 cfg->dst_port,
 					 vni,
 					 cfg->vni,
-					 ifindex, NTF_SELF, 0, true, NULL);
+					 ifindex, NTF_SELF, 0, true, NULL,
+					 GFP_ATOMIC);
 		spin_unlock(&vxlan->hash_lock);
 	}
 
@@ -4154,7 +4157,7 @@ static int vxlan_dev_create(struct net *net, struct net_device *dev,
 				       cfg->vni,
 				       cfg->vni,
 				       cfg->remote_ifindex,
-				       NTF_SELF, 0, true, extack);
+				       NTF_SELF, 0, true, extack, GFP_ATOMIC);
 		spin_unlock_bh(&vxlan->hash_lock);
 		if (err)
 			goto unlink;
@@ -4617,7 +4620,8 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
 					       cfg->dst_port,
 					       conf.vni, conf.vni,
 					       conf.remote_ifindex,
-					       NTF_SELF, 0, true, extack);
+					       NTF_SELF, 0, true, extack,
+					       GFP_ATOMIC);
 			if (err) {
 				spin_unlock_bh(&vxlan->hash_lock);
 				if (lowerdev_changed)
@@ -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);
@@ -4972,7 +4977,7 @@ vxlan_fdb_external_learn_add(struct net_device *dev,
 			       fdb_info->remote_vni,
 			       fdb_info->remote_ifindex,
 			       NTF_USE | NTF_SELF | NTF_EXT_LEARNED,
-			       0, false, extack);
+			       0, false, extack, GFP_ATOMIC);
 	spin_unlock_bh(&vxlan->hash_lock);
 
 	return err;
diff --git a/drivers/net/vxlan/vxlan_private.h b/drivers/net/vxlan/vxlan_private.h
index f0ee85f83732..b5a80ed3eff2 100644
--- a/drivers/net/vxlan/vxlan_private.h
+++ b/drivers/net/vxlan/vxlan_private.h
@@ -192,7 +192,7 @@ int vxlan_fdb_create(struct vxlan_dev *vxlan,
 		     __u16 state, __be16 port, __be32 src_vni,
 		     __be32 vni, __u32 ifindex, __u16 ndm_flags,
 		     u32 nhid, struct vxlan_fdb **fdb,
-		     struct netlink_ext_ack *extack);
+		     struct netlink_ext_ack *extack, gfp_t gfp);
 int __vxlan_fdb_delete(struct vxlan_dev *vxlan,
 		       const unsigned char *addr, union vxlan_addr ip,
 		       __be16 port, __be32 src_vni, __be32 vni,
@@ -202,7 +202,8 @@ int vxlan_fdb_update(struct vxlan_dev *vxlan,
 		     __u16 state, __u16 flags,
 		     __be16 port, __be32 src_vni, __be32 vni,
 		     __u32 ifindex, __u16 ndm_flags, u32 nhid,
-		     bool swdev_notify, struct netlink_ext_ack *extack);
+		     bool swdev_notify, struct netlink_ext_ack *extack,
+		     gfp_t gfp);
 void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		    const struct vxlan_config *cfg,
 		    __be32 default_vni, struct vxlan_rdst *rdst, bool did_rsc);
@@ -229,7 +230,7 @@ int vxlan_update_default_fdb_entry(struct vxlan_dev *vxlan, __be32 vni,
 				   const union vxlan_addr *old_remote_ip,
 				   const union vxlan_addr *remote_ip,
 				   u32 old_ifindex, u32 new_ifindex,
-				   struct netlink_ext_ack *extack);
+				   struct netlink_ext_ack *extack, gfp_t gfp);
 int vxlan_vnilist_update_group(struct vxlan_dev *vxlan,
 			       const union vxlan_addr *old_remote_ip,
 			       const union vxlan_addr *new_remote_ip,
diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
index 4a631fcdf23a..4b43674cceff 100644
--- a/drivers/net/vxlan/vxlan_vnifilter.c
+++ b/drivers/net/vxlan/vxlan_vnifilter.c
@@ -553,7 +553,7 @@ int vxlan_update_default_fdb_entry(struct vxlan_dev *vxlan, __be32 vni,
 				   const union vxlan_addr *old_remote_ip,
 				   const union vxlan_addr *remote_ip,
 				   u32 old_ifindex, u32 new_ifindex,
-				   struct netlink_ext_ack *extack)
+				   struct netlink_ext_ack *extack, gfp_t gfp)
 {
 	const struct vxlan_config *cfg = rtnl_dereference(vxlan->cfg);
 	int err = 0;
@@ -575,7 +575,8 @@ int vxlan_update_default_fdb_entry(struct vxlan_dev *vxlan, __be32 vni,
 				       vni,
 				       vni,
 				       new_ifindex,
-				       NTF_SELF, 0, true, extack);
+				       NTF_SELF, 0, true, extack,
+				       gfp);
 		if (err && extack) {
 			spin_unlock_bh(&vxlan->hash_lock);
 			return err;
@@ -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;
 
@@ -707,7 +709,8 @@ int vxlan_vnilist_update_group(struct vxlan_dev *vxlan,
 		ret = vxlan_update_default_fdb_entry(vxlan, vent->vni,
 						     oldrip, newrip,
 						     old_ifindex, new_ifindex,
-						     extack);
+						     extack,
+						     GFP_ATOMIC | __GFP_ACCOUNT);
 		if (ret)
 			goto err_unwind;
 	}
@@ -729,7 +732,8 @@ int vxlan_vnilist_update_group(struct vxlan_dev *vxlan,
 		vxlan_update_default_fdb_entry(vxlan, vent->vni,
 					       newrip, oldrip,
 					       new_ifindex, old_ifindex,
-					       NULL);
+					       NULL,
+					       GFP_ATOMIC | __GFP_ACCOUNT);
 	}
 	return ret;
 }
-- 
2.53.0

Re: [PATCH net-next v4 6/7] vxlan: vnifilter: account the default FDB entry to memcg

[netdev-bot+sashiko]

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

[PATCH net-next v4 7/7] selftests: net: test the vxlan vnifilter request limit and dump replay

[Ali Firas]

Extend the vnifilter tests for the request-size limit, the 24-bit VNI
range, and the dump/replay round trip the limit requires.

The API test gains: the largest accepted add and one VNI past it; a
rejected oversized add and a rejected multi-entry add whose entries are
each under the limit but sum over it, each checked to install nothing
(the range is asserted absent first, since vxlan_vni_add() folds an
existing VNI into the update path and would return 0 either way). The cap
is symmetric, so it also checks a maximum-size delete accepted and a
max+1 delete rejected, with every VNI in the max+1 range installed first
so the refusal can only come from the cap; a within-cap delete of a
never-installed range, which must fail on the missing VNI; an inverted
range accepted as a no-op; and the 24-bit bound exercised through END,
not only START.

bridge(8) places one VXLAN_VNIFILTER_ENTRY per comma-separated item into
a single message, so the multi-entry case is reachable without a
hand-built netlink message.

vxlan_vnifilter_dump_replay() builds a contiguous run twice the limit
from two requests, then replays every range the dump reports into a
second device and requires all to be accepted and the two dumps to
match. Without the dump clamp the run dumps as one over-limit entry that
replay rejects; with it the run dumps as limit-sized entries that
replay.

vxlan_vnifilter_api() had no teardown of its own device and namespace;
add veth-host and the test netns to cleanup(), which runs on EXIT, so a
failing case does not leave them or its entries behind.

Suggested-by: Ido Schimmel <idosch@nvidia.com>
Assisted-by: LLM
Signed-off-by: Ali Firas <alishmery18@gmail.com>
---
v4: assert the dump is non-empty before replay, so
the replay case cannot pass vacuously; rebased.

 .../selftests/net/test_vxlan_vnifiltering.sh  | 119 +++++++++++++++++-
 1 file changed, 118 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/test_vxlan_vnifiltering.sh b/tools/testing/selftests/net/test_vxlan_vnifiltering.sh
index 6fb5af013dac..bb35e89d2d03 100755
--- a/tools/testing/selftests/net/test_vxlan_vnifiltering.sh
+++ b/tools/testing/selftests/net/test_vxlan_vnifiltering.sh
@@ -84,6 +84,7 @@ ret=0
 # all tests in this script. Can be overridden with -t option
 TESTS="
 	vxlan_vnifilter_api
+	vxlan_vnifilter_dump_replay
 	vxlan_vnifilter_datapath
 	vxlan_vnifilter_datapath_pervni
 	vxlan_vnifilter_datapath_mgroup
@@ -139,8 +140,9 @@ check_vm_connectivity() {
 cleanup() {
 	ip link del veth-hv-1 2>/dev/null || true
 	ip link del vethhv-11 vethhv-12 vethhv-21 vethhv-22 2>/dev/null || true
+	ip link del veth-host 2>/dev/null || true
 
-	cleanup_ns $hv_1 $hv_2 $vm_11 $vm_21 $vm_12 $vm_22 $vm_31 $vm_32
+	cleanup_ns $hv_1 $hv_2 $vm_11 $vm_21 $vm_12 $vm_22 $vm_31 $vm_32 $testns
 }
 
 trap cleanup EXIT
@@ -347,6 +349,121 @@ vxlan_vnifilter_api()
 	# change vxlan vnifilter flag
 	run_cmd "ip -netns $testns link set dev vxlan-ext1 type vxlan external novnifilter"
 	log_test $? 2 "Cannot unset vnifilter flag on a device"
+
+	# A single request may add at most 4096 VNIs. bridge(8) puts one
+	# VXLAN_VNIFILTER_ENTRY per comma-separated item into a single message,
+	# so both the single-entry and the multi-entry paths are reachable here.
+
+	# The largest accepted add, and one VNI more rejected.
+	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 10000-14095"
+	log_test $? 0 "Add a request of the maximum VNI count"
+
+	# The rejected oversized add must install nothing. Assert the range is
+	# absent first: vxlan_vni_add() folds an already-present VNI into the
+	# update path and returns 0, so a later probe cannot tell "installed
+	# nothing" from "installed part" unless it started absent.
+	run_cmd "bridge -netns $testns vni show dev vxlan-ext1 | grep -qw 20000"
+	log_test $? 1 "VNI 20000 absent before the oversized add"
+	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 20000-24096"
+	log_test $? 255 "Cannot add a request over the maximum VNI count"
+	run_cmd "bridge -netns $testns vni show dev vxlan-ext1 | grep -qw 20000"
+	log_test $? 1 "The rejected oversized add installed nothing"
+
+	# Two entries each under the limit but summing over it: the per-message
+	# total is what is bounded, not the span of one entry. This is the shape
+	# a per-entry check would have let through.
+	run_cmd "bridge -netns $testns vni show dev vxlan-ext1 | grep -qw 30000"
+	log_test $? 1 "VNI 30000 absent before the oversized multi-entry add"
+	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 30000-32047,32048-34097"
+	log_test $? 255 "Cannot add a multi-entry request summing over the maximum"
+	run_cmd "bridge -netns $testns vni show dev vxlan-ext1 | grep -qw 30000"
+	log_test $? 1 "The rejected multi-entry add installed nothing"
+
+	# The cap is symmetric: a delete may touch at most the maximum too.
+	# Install a maximum-size range and the VNI past it, so the max+1 delete
+	# below has every VNI present and can only be refused by the cap, not by
+	# a missing VNI.
+	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 40000-44095"
+	log_test $? 0 "Populate a maximum-size range to delete"
+	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 44096"
+	log_test $? 0 "Add the VNI past the maximum range"
+	run_cmd "bridge -netns $testns vni del dev vxlan-ext1 vni 40000-44096"
+	log_test $? 255 "Cannot delete a request over the maximum VNI count"
+	run_cmd "bridge -netns $testns vni del dev vxlan-ext1 vni 40000-44095"
+	log_test $? 0 "Delete a request of the maximum VNI count"
+	run_cmd "bridge -netns $testns vni del dev vxlan-ext1 vni 44096"
+	log_test $? 0 "Delete the VNI past the maximum range"
+
+	# A within-cap delete of a never-installed range reaches the handler and
+	# fails there on the missing VNI, not on the cap.
+	run_cmd "bridge -netns $testns vni del dev vxlan-ext1 vni 50000-50010"
+	log_test $? 255 "Cannot delete a range that was never installed"
+
+	# A start above the end selects nothing and is accepted as a no-op.
+	# Use a VNI no earlier case installs, so the absence check is meaningful.
+	run_cmd "bridge -netns $testns vni show dev vxlan-ext1 | grep -qw 55000"
+	log_test $? 1 "VNI 55000 absent before the inverted range"
+	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 55000-50000"
+	log_test $? 0 "An inverted range is accepted as a no-op"
+	run_cmd "bridge -netns $testns vni show dev vxlan-ext1 | grep -qw 55000"
+	log_test $? 1 "The inverted range installed nothing"
+
+	# The VXLAN header carries 24 bits. The bound is on both endpoints, so a
+	# range whose END alone leaves the space is rejected too.
+	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 16777215"
+	log_test $? 0 "Add the highest VNI the header can carry"
+	run_cmd "bridge -netns $testns vni del dev vxlan-ext1 vni 16777215"
+	log_test $? 0 "Delete the highest VNI the header can carry"
+	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 16777215-16777216"
+	log_test $? 255 "Cannot add a range whose END leaves the 24-bit space"
+	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 100-4294967295"
+	log_test $? 255 "Cannot add a range whose END wraps past the 24-bit space"
+}
+
+# A device may hold a contiguous run longer than one request's limit, built
+# from several requests. The dump coalesces it, so the dump must break the run
+# into entries each no larger than the limit, or the configuration it reports
+# cannot be replayed. Install such a run, then feed every range the dump
+# reports back into a second device and require all to be accepted.
+vxlan_vnifilter_dump_replay()
+{
+	local opts="external vnifilter local 172.16.0.1 dev veth-testns"
+	local range rc=0 src_ranges dst_ranges
+
+	cleanup_vnifilter_api &>/dev/null
+	setup_vnifilter_api
+
+	# The destination is on a different dstport so re-adding the same VNIs
+	# does not collide with the source under vxlan_vni_in_use().
+	run_cmd "ip -netns $testns link add vxlan-src type vxlan $opts dstport 4789"
+	log_test $? 0 "dump/replay: create source device"
+	run_cmd "ip -netns $testns link add vxlan-dst type vxlan $opts dstport 4790"
+	log_test $? 0 "dump/replay: create destination device"
+
+	# 8192 contiguous VNIs sharing the default remote: one run, twice the
+	# limit, installed in two accepted requests.
+	run_cmd "bridge -netns $testns vni add dev vxlan-src vni 10000-14095"
+	run_cmd "bridge -netns $testns vni add dev vxlan-src vni 14096-18191"
+	log_test $? 0 "dump/replay: populate a run larger than the limit"
+
+	# Assert the dump is non-empty before replaying it, so an empty or
+	# failed dump cannot let the replay check below pass without testing
+	# anything.
+	src_ranges=$(bridge -netns $testns vni show dev vxlan-src | \
+		     grep -oE '[0-9]+-[0-9]+|[0-9]{2,}' | sort)
+	test -n "$src_ranges"
+	log_test $? 0 "dump/replay: source dump is non-empty"
+
+	for range in $src_ranges; do
+		bridge -netns $testns vni add dev vxlan-dst vni "$range" \
+			2>/dev/null || rc=$?
+	done
+	log_test $rc 0 "dump/replay: every dumped entry is accepted on replay"
+
+	dst_ranges=$(bridge -netns $testns vni show dev vxlan-dst | \
+		     grep -oE '[0-9]+-[0-9]+|[0-9]{2,}' | sort)
+	[ "$src_ranges" = "$dst_ranges" ]
+	log_test $? 0 "dump/replay: source and destination dump the same ranges"
 }
 
 # Sanity test vnifilter datapath
-- 
2.53.0

Re: [PATCH net-next v4 7/7] selftests: net: test the vxlan vnifilter request limit and dump replay

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 4 (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_vnifilter_dump_replay()
  (tools/testing/selftests/net/test_vxlan_vnifiltering.sh), the exit…
- [Low] Each new 'absent before' / 'installed nothing' probe in
  vxlan_vnifilter_api() runs `bridge -netns $testns vni show dev vxlan-…
- [Low] vxlan_vnifilter_dump_replay() replays the dump one range at a
  time, each in its own `bridge vni add` (lines 457-460), using ranges…
- [Low] The 24-bit bound cases in vxlan_vnifilter_api() do not isolate
  the START half of vni_filter_entry_policy, and one case cannot tell…
- [Low] In vxlan_vnifilter_dump_replay(), the replay result variable `rc`
  is overwritten by run_cmd() before the replay loop.

Pre-existing issues:
- [Low] The comment on VXLAN_VNI_FILTER_MSG_MAX in
  drivers/net/vxlan/vxlan_vnifilter.c (lines 20-24) still says 'A later…

--

selftests: net: test the vxlan vnifilter request limit and dump replay

This extends test_vxlan_vnifiltering.sh to cover the per-request VNI cap
on add and delete and the 24-bit VNI bound. It also adds a dump/replay
test that builds a contiguous run twice the limit and replays the dumped
ranges into a second device. The cleanup() handler now also removes
veth-host and the test netns.

> diff --git a/tools/testing/selftests/net/test_vxlan_vnifiltering.sh b/tools/testing/selftests/net/test_vxlan_vnifiltering.sh
> index 6fb5af013dacf..bb35e89d2d036 100755
> --- a/tools/testing/selftests/net/test_vxlan_vnifiltering.sh
> +++ b/tools/testing/selftests/net/test_vxlan_vnifiltering.sh

[ ... ]

> @@ -347,6 +349,121 @@ vxlan_vnifilter_api()
>  	# change vxlan vnifilter flag
>  	run_cmd "ip -netns $testns link set dev vxlan-ext1 type vxlan external novnifilter"
>  	log_test $? 2 "Cannot unset vnifilter flag on a device"
> +
> +	# A single request may add at most 4096 VNIs. bridge(8) puts one
> +	# VXLAN_VNIFILTER_ENTRY per comma-separated item into a single message,
> +	# so both the single-entry and the multi-entry paths are reachable here.

[Severity: Low]
This problem was not introduced by this patch, but the comment on
VXLAN_VNI_FILTER_MSG_MAX in drivers/net/vxlan/vxlan_vnifilter.c still
describes the request cap as future work:

drivers/net/vxlan/vxlan_vnifilter.c:
 * dump emits stays within what the input path will accept on replay. A later
 * patch bounds a single RTM_NEWTUNNEL/RTM_DELTUNNEL request to the same number.
 */
#define VXLAN_VNI_FILTER_MSG_MAX	4096

That cap is already in the tree. vxlan_vnifilter_process() calls
vxlan_vnifilter_check_msg() before it applies any entry:

	err = vxlan_vnifilter_check_msg(nlh, extack);
	if (err)
		return err;

Could the "A later patch" sentence be updated, since this selftest is
exercising that cap?

> +
> +	# The largest accepted add, and one VNI more rejected.
> +	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 10000-14095"
> +	log_test $? 0 "Add a request of the maximum VNI count"
> +
> +	# The rejected oversized add must install nothing. Assert the range is
> +	# absent first: vxlan_vni_add() folds an already-present VNI into the
> +	# update path and returns 0, so a later probe cannot tell "installed
> +	# nothing" from "installed part" unless it started absent.
> +	run_cmd "bridge -netns $testns vni show dev vxlan-ext1 | grep -qw 20000"
> +	log_test $? 1 "VNI 20000 absent before the oversized add"

[Severity: Low]
Can this probe pass when bridge vni show itself fails?

run_cmd() evaluates the whole pipeline and returns its status:

	out=$(eval $cmd $stderr)
	rc=$?

That status comes from grep. Neither this script nor lib.sh sets
pipefail. If bridge vni show fails, grep reads no input and exits 1, which
is the value log_test expects.

The same pattern is used for each of the "absent before" and "installed
nothing" probes for 20000, 30000 and 55000. That means these probes cannot
tell a VNI that is absent from a show that failed.

The src_ranges and dst_ranges captures in vxlan_vnifilter_dump_replay() also
drop the exit status of bridge.

Would it make sense to check the exit status of bridge vni show on its own,
or to set pipefail for these probes?

> +	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 20000-24096"
> +	log_test $? 255 "Cannot add a request over the maximum VNI count"

[ ... ]

> +	# The VXLAN header carries 24 bits. The bound is on both endpoints, so a
> +	# range whose END alone leaves the space is rejected too.
> +	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 16777215"
> +	log_test $? 0 "Add the highest VNI the header can carry"
> +	run_cmd "bridge -netns $testns vni del dev vxlan-ext1 vni 16777215"
> +	log_test $? 0 "Delete the highest VNI the header can carry"
> +	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 16777215-16777216"
> +	log_test $? 255 "Cannot add a range whose END leaves the 24-bit space"
> +	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 100-4294967295"
> +	log_test $? 255 "Cannot add a range whose END wraps past the 24-bit space"

[Severity: Low]
Does the 100-4294967295 case depend on the END range policy? Its span is
4294967196 VNIs, so vxlan_vnifilter_check_msg() would reject it with
-EINVAL even if the END bound in vni_filter_entry_policy were removed:

	vnis += vni_end - vni_start + 1;
	if (vnis > VXLAN_VNI_FILTER_MSG_MAX) {

The test comment ("The bound is on both endpoints") and the commit message
("exercised through END, not only START") suggest that an out-of-range
START is tested. No such case exists here or elsewhere under
tools/testing/selftests.

A single VNI such as 16777216 has no END attribute. That gives a span of
one, so check_msg accepts it, and only the START policy keeps it out of
vxlan_vni_add_del():

	[VXLAN_VNIFILTER_ENTRY_START] = NLA_POLICY_FULL_RANGE(NLA_U32,
							      &vni_filter_vni_range),

If START=END=0xffffffff ever got past that policy, this loop would wrap
under rtnl_lock:

	for (v = start_vni; v <= end_vni; v++) {

Could a case like "vni add ... vni 16777216" be added so a regression in
the START bound is caught?

> +}
> +
> +# A device may hold a contiguous run longer than one request's limit, built
> +# from several requests. The dump coalesces it, so the dump must break the run
> +# into entries each no larger than the limit, or the configuration it reports
> +# cannot be replayed. Install such a run, then feed every range the dump
> +# reports back into a second device and require all to be accepted.
> +vxlan_vnifilter_dump_replay()
> +{
> +	local opts="external vnifilter local 172.16.0.1 dev veth-testns"
> +	local range rc=0 src_ranges dst_ranges

[ ... ]

> +	# 8192 contiguous VNIs sharing the default remote: one run, twice the
> +	# limit, installed in two accepted requests.
> +	run_cmd "bridge -netns $testns vni add dev vxlan-src vni 10000-14095"
> +	run_cmd "bridge -netns $testns vni add dev vxlan-src vni 14096-18191"
> +	log_test $? 0 "dump/replay: populate a run larger than the limit"

[Severity: Medium]
Is the status of the first populate command lost here? The $? passed to
log_test is only the status of the 14096-18191 add.

If the 10000-14095 add fails, vxlan-src holds exactly 4096 VNIs. In that
case vxlan_vnifilter_dump_dev() never reaches the clamp:

	if (!dump_stats && vnirange(vend, v) == 1 &&
	    vnirange(vbegin, v) < VXLAN_VNI_FILTER_MSG_MAX &&

The dump is then a single limit-sized entry. The non-empty check, the
replay loop and the src/dst comparison all pass, whether or not the dump
clamp exists.

The v4 note says the non-empty check was added so the replay case cannot
pass vacuously, but this path still does.

There is a related gap. src_ranges is never compared against the expected
split (10000-14095 and 14096-18191). Both the replay input and the expected
result come from the same source dump.

A resume regression in vxlan_vnifilter_dump_dev() could drop or repeat VNIs
at the 4096 boundary, or lose the second message:

	if (!err && vbegin) {
		if (vnis + vnirange(vbegin, vend) + 1 > VXLAN_VNI_FILTER_MSG_MAX)
			err = -EMSGSIZE;

Wouldn't that produce the same wrong ranges on both devices and still
pass?

Could each populate command get its own log_test, and could src_ranges be
checked against the literal expected ranges?

> +
> +	# Assert the dump is non-empty before replaying it, so an empty or
> +	# failed dump cannot let the replay check below pass without testing
> +	# anything.
> +	src_ranges=$(bridge -netns $testns vni show dev vxlan-src | \
> +		     grep -oE '[0-9]+-[0-9]+|[0-9]{2,}' | sort)
> +	test -n "$src_ranges"
> +	log_test $? 0 "dump/replay: source dump is non-empty"
> +
> +	for range in $src_ranges; do
> +		bridge -netns $testns vni add dev vxlan-dst vni "$range" \
> +			2>/dev/null || rc=$?
> +	done

[Severity: Low]
This loop replays one range per bridge vni add, using ranges parsed from
the text output of bridge vni show. That output loses netlink message
boundaries, so the per-message VNI budget in vxlan_vnifilter_dump_dev() is
never tested:

	if (vnis + sz > VXLAN_VNI_FILTER_MSG_MAX) {
		err = -EMSGSIZE;
		break;
	}

vxlan_vnifilter_check_msg() caps the sum across all entries in a request.
Suppose that budget were removed and the merge clamp kept. The 8192-VNI
run would be dumped as 10000-14095 and 14096-18191 in one message, and that
message would be rejected if replayed as a single request.

Each range would still replay on its own here, the dumps would match, and
the test would pass. Is the per-message half of the dump guarantee meant
to be covered by this test?

> +	log_test $rc 0 "dump/replay: every dumped entry is accepted on replay"

[Severity: Low]
Can rc be non-zero here even when every replay add succeeded?

run_cmd() assigns rc=$? without declaring it local. Because bash scoping is
dynamic, that assignment writes to the local rc in
vxlan_vnifilter_dump_replay(). After the 14096-18191 populate run_cmd, rc
holds that command's exit status.

The replay loop only assigns rc when an add fails. So if the populate
failed, this line also reports FAIL even when all replays succeeded, and
the rc=0 initializer has no effect.

Would resetting rc=0 just before the loop, or using a separate variable,
avoid this?

> +
> +	dst_ranges=$(bridge -netns $testns vni show dev vxlan-dst | \
> +		     grep -oE '[0-9]+-[0-9]+|[0-9]{2,}' | sort)
> +	[ "$src_ranges" = "$dst_ranges" ]
> +	log_test $? 0 "dump/replay: source and destination dump the same ranges"
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014123814.388557-1-alishmery18%40gmail.com

Re: [PATCH net-next v4 0/7] vxlan: vnifilter: bound a single request and account per-VNI memory

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.