Re: [PATCH net-next v3 3/6] 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 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 changelog justifies capping delete as well as add by saying
  the two "both walk the same span and hold rtnl the same way".
- [Low] The commit message says the 4096 limit follows "the usable VLAN
  ID space".
- [Low] At this commit, the dump path can emit a VXLAN_VNIFILTER_ENTRY
  that the new request parser refuses.

--

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

This patch caps the number of VNIs that one RTM_NEWTUNNEL or RTM_DELTUNNEL
message may add or delete at 4096. The count is summed across all
VXLAN_VNIFILTER_ENTRY attributes and checked in a new
vxlan_vnifilter_check_msg() before any entry is applied. The range
extraction moves into vxlan_vni_filter_entry_range() so the check and the
dispatch path use the same range.

> 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.

[Severity: Low]
Is "both walk the same span" accurate for delete? vxlan_vni_del() returns
-ENOENT for a VNI that is not installed:

drivers/net/vxlan/vxlan_vnifilter.c:vxlan_vni_del() {
    ...
	vninode = rhashtable_lookup_fast(&vg->vni_hash, &v,
					 vxlan_vni_rht_params);
	if (!vninode) {
		err = -ENOENT;
		goto out;
	}
    ...
}

vxlan_vni_add_del() then stops the walk at the first error:

		if (err)
			goto out;

So an RTM_DELTUNNEL walk covers the full span only when every VNI in it is
installed. That worst case can happen after several capped adds, so there
is still a reason to bound delete. The wording could be more precise,
though.

The changelog says that installs can be split across requests and that
vxlan_vnigroup_uninit() is exempt. It does not say that a user who
installed a contiguous run of more than 4096 VNIs can no longer delete it
in one message. The same applies to re-issuing RTM_NEWTUNNEL over that run
to change the group. vxlan_vnifilter_check_msg() charges the full requested
span, whatever is actually installed.

Could the changelog spell out this user-visible narrowing?

> The limit is 4096, following the usable VLAN ID space, since vnifilter
> is mainly used on bridged devices where the VNI is derived from the
> VLAN.

[Severity: Low]
This isn't a bug, but is "usable" the right word here? The usable 802.1Q
VLAN IDs are 1..4094. br_vlan_valid_id() in net/bridge/br_private.h only
accepts:

	bool ret = vid > 0 && vid < VLAN_VID_MASK;

4096 is the size of the full 12-bit VID field, which includes the reserved
IDs 0 and 4095. The in-code comment on VXLAN_VNI_FILTER_MSG_MAX says "the
VLAN ID space", which reads fine.

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index 9cffaf4998a98..13f4e115701ac 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c

[ ... ]

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

[ ... ]

> +		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: Low]
At this point in the series, can a dump emit an entry that this check then
rejects?

vxlan_vnifilter_dump_dev() merges any contiguous run of VNIs with the same
remote into a single START/END entry. It puts no bound on the span:

drivers/net/vxlan/vxlan_vnifilter.c:vxlan_vnifilter_dump_dev() {
    ...
		if (!dump_stats && vnirange(vend, v) == 1 &&
		    vxlan_addr_equal(&v->remote_ip, &vend->remote_ip)) {
			goto update_end;
    ...
}

A device can still hold more than 4096 contiguous VNIs installed over
several requests, because vxlan_vni_rht_params uses
.max_size = VXLAN_N_VID. If such a dumped entry is replayed in one
RTM_NEWTUNNEL, vxlan_vnifilter_check_msg() returns -EINVAL.

The next patch in the series, "vxlan: vnifilter: clamp the dumped VNI
range to the request limit", fixes this. It adds
vnirange(vbegin, v) < VXLAN_VNI_FILTER_MSG_MAX to the merge condition, and
the selftest commit checks the dump/replay round trip.

Would it make sense to put the clamp before this patch, or fold it in, so
dump and request stay consistent at every commit in the series?

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

[ ... ]

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