Re: [PATCH net-next v2 1/5] vxlan: vnifilter: reject VNIs outside the 24-bit space

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 0

- [Medium] Missing `Fixes:` tag.
- [Medium] Commit message inaccuracy: the message claims the offending
  request "is then rejected during netlink policy validation, before…

Pre-existing issues:
- [Medium] Residual per-request allocation storm under rtnl_lock after
  this patch alone: the new policy caps START/END at VXLAN_N_VID-1,…

--

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

The patch adds a netlink_range_validation bound of VXLAN_N_VID - 1 to the
VXLAN_VNIFILTER_ENTRY_START and VXLAN_VNIFILTER_ENTRY_END attributes in
vni_filter_entry_policy, and changes the vxlan_vni_add_del() loop counter
from int to u32.

v1: https://lore.kernel.org/netdev/20260909092645.3105263-1-alishmery18@xxxxxxxxx/

A few questions on the commit message and on the remaining behaviour of
the loop.

> Assisted-by: LLM
> Signed-off-by: Ali Firas <alishmery18@xxxxxxxxx>

[Severity: Medium]
Should this carry a Fixes: tag?  The bug being closed is reachable from a
plain RTM_NEWTUNNEL/RTM_DELTUNNEL message and makes vxlan_vni_add_del()
walk most of the 32-bit space allocating a struct vxlan_vni_node plus a
per-CPU stats block per iteration under rtnl_lock, so stable trees would
want it.

Both the "int v" loop and the unbounded NLA_U32 START/END policy arrived
with the file itself:

  Fixes: f9c4bb0b245c ("vxlan: vni filtering support on collect metadata device")

Without that tag, backport tooling has nothing to key off.

> Range-validate both attributes against the 24-bit VNI space, which is
> what vxlan_mdb.c already does for its own VNI attributes. The request is
> then rejected during netlink policy validation, before
> vxlan_process_vni_filter() is reached, and nothing is allocated.

[Severity: Medium]
Is "before vxlan_process_vni_filter() is reached, and nothing is
allocated" accurate for a multi-entry message?

The new range bound is only enforced by the nla_parse_nested() call
inside vxlan_process_vni_filter():

drivers/net/vxlan/vxlan_vnifilter.c:vxlan_process_vni_filter() {
	err = nla_parse_nested(vattrs,
			       VXLAN_VNIFILTER_ENTRY_MAX,
			       nlvnifilter, vni_filter_entry_policy,
			       extack);
	...
}

The top-level nlmsg_parse() in vxlan_vnifilter_process() uses
vni_filter_policy, where the nest has no nested_policy pointer, and
lib/nlattr.c only recurses when pt->nested_policy is set:

static const struct nla_policy vni_filter_policy[VXLAN_VNIFILTER_MAX + 1] = {
	[VXLAN_VNIFILTER_ENTRY] = { .type = NLA_NESTED },
};

So for a message like [ENTRY: START=1 END=100][ENTRY: START=0xffffffff],
does the first entry get fully applied (100 vxlan_vni_node plus per-CPU
stats allocations, rhashtable inserts and RTM_NEWTUNNEL notifications)
before the second one returns -ERANGE, with vxlan_vnifilter_process()
merely breaking out of nlmsg_for_each_attr_type() and leaving that state
installed?

"Nothing is allocated" appears to hold only for a single-entry request.

Noting for the series: the later patch "vxlan: vnifilter: bound the number
of VNIs one request may touch" adds vxlan_vnifilter_check_msg(), which
parses every entry against vni_filter_entry_policy before the dispatch
loop, so the partial-application behaviour is gone by the end of the
series.  The wording in this patch's own message, and therefore in a
standalone stable backport of just this patch, still describes something
else.

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index dd94085e08865..5aaaaeee81107 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: Medium]
This isn't a bug introduced by this patch, but the per-request allocation
behaviour described in the commit message survives it in a narrower form,
since the policy bounds each endpoint and not the distance between them.

vxlan_process_vni_filter() leaves vni_start at 0 when only END is
present:

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

So a single entry carrying only END=0xffffff passes the new policy and
still drives roughly 2^24 iterations of vxlan_vni_add_del() ->
vxlan_vni_add() -> vxlan_vni_alloc(), each doing a GFP_KERNEL
kzalloc_obj() of struct vxlan_vni_node plus netdev_alloc_pcpu_stats() over
nr_cpu_ids, an rhashtable insert and an RTM_NEWTUNNEL notification skb.

There is no cond_resched() and no fatal-signal check in that loop, and the
handlers are registered without RTNL_FLAG_DOIT_UNLOCKED, so rtnl_lock is
held throughout.  Is that span still intended to be unbounded here?

Also, when an allocation fails part way through the range the loop takes
the goto out path and the nodes already inserted into vg->vni_hash and
vg->vni_list stay behind.  That predates this patch as well.

The follow-on patch "vxlan: vnifilter: bound the number of VNIs one
request may touch" caps the summed span at VXLAN_VNI_FILTER_MSG_MAX before
the dispatch loop, and the memcg accounting patches later in the series
change both allocations to GFP_KERNEL_ACCOUNT, so this is addressed within
the series.

>  	[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)),
>  };
> @@ -814,7 +820,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);
>

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