Re: [PATCH net-next v2 2/5] vxlan: vnifilter: bound the number of VNIs one request may touch

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The kernel's own dump output is no longer accepted by the
  kernel's own input path.
- [Low] Ambiguous framing: the changelog describes a concrete kernel
  defect — one netlink message making vxlan_vni_add_del()…

--

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

The patch adds vxlan_vnifilter_check_msg(), which sums the START/END span
of every VXLAN_VNIFILTER_ENTRY in an RTM_NEWTUNNEL or RTM_DELTUNNEL
message and rejects the message with -EINVAL if the total exceeds a new
driver-local limit of 4096.  The range extraction used by
vxlan_process_vni_filter() is factored out into
vxlan_vni_filter_entry_range() so both sides compute the same range.

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

Two questions, one about the changelog and one about the new check.

> vxlan: vnifilter: bound the number of VNIs one request may touch
>
> With both endpoints bounded to the 24-bit space, a single RTM_NEWTUNNEL
> or RTM_DELTUNNEL message can still ask for all of it.
> vxlan_vni_add_del() loops over the span creating one VNI node and one
> per-CPU stats block per iteration, all under rtnl_lock.

[Severity: Low]
This part of the changelog reads like the description of a user triggerable
defect: one netlink message makes vxlan_vni_add_del() iterate over up to
the whole 24-bit space, allocating a vxlan_vni_node plus a per-CPU stats
block per iteration while holding rtnl_lock.

There is no Fixes: tag naming the commit that introduced that loop
(f9c4bb0b245c "vxlan: vni filtering support on collect metadata device"),
and no statement that this is a policy/hardening change that should not be
backported.

Since the patch also narrows what userspace may send in one message, could
the changelog say which of the two it is, so stable triage does not have to
guess?

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index 5aaaaeee81107..9a1baca39d8b0 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c
> @@ -17,6 +17,15 @@
>  
>  #include "vxlan_private.h"
>  
> +/* Maximum number of VNIs one RTM_NEWTUNNEL or RTM_DELTUNNEL message may add or
> + * delete, summed over all of its VXLAN_VNIFILTER_ENTRY attributes. VNI
> + * filtering is mainly used on bridged VXLAN devices where the VNI is derived
> + * from the VLAN, so a message touching more VNIs than the VLAN ID space has no
> + * practical use, while an unbounded message can walk the whole 24-bit space
> + * under rtnl_lock.
> + */
> +#define VXLAN_VNI_FILTER_MSG_MAX	4096
> +

[ ... ]

> @@ -846,12 +855,77 @@ static int vxlan_vni_add_del(struct vxlan_dev *vxlan, __u32 start_vni,

[ ... ]

> +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) {

[ ... ]

> +		/* vni_filter_entry_policy has already bounded both endpoints
> +		 * to 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;
> +		}
> +	}

[Severity: Medium]
Does this make the driver emit a VXLAN_VNIFILTER_ENTRY that it will then
refuse to parse back?

The changelog states that a device may still hold the whole 24-bit space as
long as it is populated by several messages.  On dump,
vxlan_vnifilter_dump_dev() merges every contiguous run sharing a remote
into one entry with no clamp:

	if (!dump_stats && vnirange(vend, v) == 1 &&
	    vxlan_addr_equal(&v->remote_ip, &vend->remote_ip)) {
		goto update_end;

and vxlan_fill_vni_filter_entry() serializes that whole run into the same
START/END pair that update requests use:

	if (nla_put_u32(skb, VXLAN_VNIFILTER_ENTRY_START, vs))
		goto out_err;

	if (ve && nla_put_u32(skb, VXLAN_VNIFILTER_ENTRY_END, ve))
		goto out_err;

So a device populated with more than 4096 contiguous VNIs sharing a remote
dumps as a single entry whose span vxlan_vnifilter_check_msg() rejects with
-EINVAL on replay.  Should the dump side be clamped to the same constant so
the two representations agree?

A second question on the same check: it does not look at nlh->nlmsg_type,
so RTM_DELTUNNEL is bounded as well.  vxlan_vni_del() only looks the VNI up
and frees it:

	vninode = rhashtable_lookup_fast(&vg->vni_hash, &v,
					 vxlan_vni_rht_params);
	if (!vninode) {
		err = -ENOENT;

Since the delete path allocates nothing, is bounding RTM_DELTUNNEL
intended?  As written, a device holding more than 4096 VNIs can no longer
be torn down with the single range the dump reported.

> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -975,6 +1043,10 @@ static int vxlan_vnifilter_process(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (!(vxlan->cfg.flags & VXLAN_F_VNIFILTER))
>  		return -EOPNOTSUPP;
>  
> +	err = vxlan_vnifilter_check_msg(nlh, extack);
> +	if (err)
> +		return err;
> +

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