Re: [PATCH net v2] vxlan: vnifilter: validate the VNI range in vni_filter_entry_policy

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Incomplete fix relative to the impact its own changelog
  describes: after the range clamp to VXLAN_N_VID-1, a single…

--

vxlan: vnifilter: validate the VNI range in vni_filter_entry_policy

This adds an NLA_POLICY_FULL_RANGE constraint with .max = VXLAN_N_VID - 1
to VXLAN_VNIFILTER_ENTRY_START and VXLAN_VNIFILTER_ENTRY_END, matching
what vxlan_mdb.c already does for its VNI attributes.  The commit carries
Fixes: f9c4bb0b245c and Cc: stable@vger.kernel.org.

No Link: tags in the commit header; the v1 posting is referenced in the
patch notes as
https://lore.kernel.org/netdev/20260829030041.940594-1-alishmery18@gmail.com/

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index dd94085e0886..9e86ac39cf9d 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c
> @@ -459,9 +459,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),

[Severity: High]
Does this clamp close the problem the changelog describes, or only the
U32_MAX endpoint?

With the new .max, a single PF_BRIDGE RTM_NEWTUNNEL message carrying only
VXLAN_VNIFILTER_ENTRY_END = 0xFFFFFF still passes validation, because
vni_start defaults to 0 in vxlan_process_vni_filter():

	u32 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]);

	if (!vni_start && !vni_end) {
		NL_SET_ERR_MSG_ATTR(extack, nlvnifilter,
				    "vni start nor end found in vni entry");
		return -EINVAL;
	}

vni_end is non-zero so the guard does not fire, and vxlan_vni_add_del() is
entered with the full 0..0xFFFFFF span.

Each of those 2^24 iterations goes through vxlan_vni_add() ->
vxlan_vni_alloc(), which allocates a node plus a full per-CPU stats block:

	vninode = kzalloc_obj(*vninode);
	if (!vninode)
		return NULL;
	vninode->stats = netdev_alloc_pcpu_stats(struct vxlan_vni_stats_pcpu);

followed by an rhashtable insert, a sorted list insert and a
vxlan_vnifilter_notify().  Neither allocation uses __GFP_ACCOUNT, so the
memory is charged globally rather than to the requesting cgroup.

There is also no cond_resched() in vxlan_vni_add_del() (grep finds none in
the file), no per-request work cap and no ceiling on vg->num_vnis, and
vxlan_vnifilter_rtnl_msg_handlers registers the handler without
RTNL_FLAG_DOIT_UNLOCKED, so the whole span runs under the global rtnl_lock.

Given that, is the Fixes: plus Cc: stable framing accurate?  The changelog
states:

	It runs under rtnl_lock, which is global rather than per-netns, so
	every network configuration operation on the host blocks for as long
	as it runs, in every namespace.

	The interface is reachable without privilege: creating the device and
	adding VNIs only requires CAP_NET_ADMIN in the network namespace's
	user namespace

and then:

	A request spanning the full in-range space, START=0 END=0xFFFFFF,
	still exhausts memory on that guest both before and after, as
	described above.

Would a backporter reading the first part conclude that the rtnl_lock
starvation and unprivileged memory exhaustion are closed, when the second
part says they are not?

Separately, is this description of the U32_MAX case right?

	With end_vni == U32_MAX the loop cannot terminate through its own
	condition: v reaches U32_MAX, wraps to 0, and 0 <= U32_MAX is true
	again, so the request never returns.

The test result in the same changelog says that request "drives a global OOM
with the allocating task in vxlan_vnifilter_process()".  Doesn't the loop
actually leave early once vxlan_vni_alloc() fails?

	err = vxlan_vni_add(vxlan, vg, v, group, extack);
	...
	if (err)
		goto out;

so the observed exit is -ENOMEM rather than a loop that never returns.

One more question on the partial-failure path: when vxlan_vni_add() fails
part way through a range, nothing unwinds the VNIs already added, and they
stay in vg->vni_hash and vg->vni_list until vxlan_vnigroup_uninit().  Is a
partially applied range the intended behaviour here?

Would it make sense to bound the per-request work in the same change, for
example capping the number of VNIs one message may span, adding a reschedule
point to the vxlan_vni_add_del() loop, and using __GFP_ACCOUNT for the node
and stats allocations, so that the tags match what the patch removes?

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