Re: [PATCH net] net: bridge: don't under-estimate the VLAN tunnel info size

[netdev-bot+sashiko@kernel.org]

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

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

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] The fix is incomplete.
- [High] The commit message says the change 'keeps br_nlmsg_size() an
  upper bound of what the fill needs', but the IFLA_BRIDGE_VLAN_INFO…
- [Medium] On any bridge port with vlan_tunnel enabled,
  br_get_vlan_tunnel_info_size() now reserves __get_vlan_tinfo_size()…

--

net: bridge: don't under-estimate the VLAN tunnel info size

br_get_vlan_tunnel_info_size() used to count the compressed ranges of VLANs
that currently have a tunnel mapping. This patch makes it reserve one
IFLA_BRIDGE_VLAN_TUNNEL_INFO attribute for every usable VLAN in the group.
The goal is to stop br_info_notify() from hitting
WARN_ON(err == -EMSGSIZE) when mappings change between br_nlmsg_size() and
br_fill_ifinfo() on the STP timer path, which runs without RTNL.

> Size the tunnel info for the maximum number of attributes that
> br_fill_vlan_tunnel_info() can emit for the VLAN group instead of for the
> mappings that are currently installed.  Consecutive VIDs with consecutive
> tunnel IDs are compressed into two attributes, so the fill never emits more
> attributes than there are usable VLANs, which keeps br_nlmsg_size() an upper
> bound of what the fill needs.

[Severity: High]
Does br_nlmsg_size() really become an upper bound with this change? The
IFLA_BRIDGE_VLAN_INFO part of the same message is still sized from a
snapshot.

br_get_link_af_size_filtered() gets that size from __get_num_vlan_infos().
That function counts compressed ranges using the current PVID, the
BRIDGE_VLAN_INFO_UNTAGGED flag of each VLAN and the current VLAN set:

net/bridge/br_netlink.c:br_get_link_af_size_filtered() {
    ...
	num_vlan_infos = br_get_num_vlan_infos(vg, filter_mask);
    ...
	vinfo_sz += num_vlan_infos * nla_total_size(sizeof(struct bridge_vlan_info));
    ...
}

br_fill_ifvlaninfo_compressed() later works out the ranges again from live
state. br_ifinfo_notify() always passes RTEXT_FILTER_BRVLAN_COMPRESSED,
including on the br_forward_delay_timer_expired() path in the trace.

Suppose an RTM_SETLINK under RTNL does one of the following while the timer
is between sizing and filling:

- toggles UNTAGGED on a VLAN in the middle of a range (one 2-attribute
  range becomes 2+1+2)
- changes the PVID
- adds non-adjacent VLANs through __vlan_add()->__vlan_add_list()

Can't br_fill_ifvlaninfo_compressed() still return -EMSGSIZE and hit the
same WARN_ON in br_info_notify()? This applies to tunnel and non-tunnel
ports alike.

About 30 extra 8-byte attributes are enough to go past the kmalloc slack.
A single setlink with many IFLA_BRIDGE_VLAN_INFO entries can produce that
many.

The patch only makes the tunnel half of br_get_link_af_size_filtered()
pessimistic. The two halves now follow different sizing rules.

> diff --git a/net/bridge/br_netlink_tunnel.c b/net/bridge/br_netlink_tunnel.c
> index e7eceab5b515d..74627323711c1 100644
> --- a/net/bridge/br_netlink_tunnel.c
> +++ b/net/bridge/br_netlink_tunnel.c
> @@ -35,39 +35,25 @@ bool vlan_tunid_inrange(const struct net_bridge_vlan *v_curr,
>  	return (be32_to_cpu(tunid_curr) - be32_to_cpu(tunid_last)) == 1;
>  }
>  
> -static int __get_num_vlan_tunnel_infos(struct net_bridge_vlan_group *vg)
> +/* Upper bound of the number of IFLA_BRIDGE_VLAN_TUNNEL_INFO attributes that
> + * br_fill_vlan_tunnel_info() can emit for @vg.
> + *
> + * br_info_notify() is also called without RTNL (e.g. from the STP timers), so
> + * the set of VLANs that have a tunnel mapping can change between the size
> + * calculation done by br_nlmsg_size() and the actual fill.  Account for the
> + * maximum instead of the currently used mappings: consecutive VIDs with
> + * consecutive tunnel IDs are compressed into two attributes, so the fill never
> + * emits more attributes than there are usable VLANs in the group.
> + */
> +static int __get_max_num_vlan_tunnel_infos(struct net_bridge_vlan_group *vg)
>  {
> -	struct net_bridge_vlan *v, *vtbegin = NULL, *vtend = NULL;
> +	struct net_bridge_vlan *v;
>  	int num_tinfos = 0;
>  
> -	/* Count number of vlan infos */
>  	list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
>  		/* only a context, bridge vlan not activated */
> -		if (!br_vlan_should_use(v) || !v->tinfo.tunnel_id)
> -			continue;

[ ... ]

> +		if (br_vlan_should_use(v))
> +			num_tinfos++;
>  	}

[Severity: High]
Isn't this still only a bound on vlan_list as it looks at sizing time?

When called from br_forward_delay_timer_expired(), br_info_notify() holds
only br->lock and RCU. br_fill_ifinfo()->br_fill_vlan_tunnel_info() then
walks the list again and checks BR_VLAN_TUNNEL_BIT again on its own:

net/bridge/br_netlink.c:br_info_notify() {
    ...
	skb = nlmsg_new(br_nlmsg_size(dev, filter), GFP_ATOMIC);
    ...
	err = br_fill_ifinfo(skb, port, 0, 0, event, 0, filter, dev, false);
	if (err < 0) {
		/* -EMSGSIZE implies BUG in br_nlmsg_size() */
		WARN_ON(err == -EMSGSIZE);
    ...
}

The RTNL writers don't take br->lock, so two inputs can still change
between those steps.

First, VLANs can be added. __vlan_add()->__vlan_add_list() publishes them
with list_add_rcu() under RTNL only. One RTM_SETLINK can carry
IFLA_BRIDGE_VLAN_INFO followed by IFLA_BRIDGE_VLAN_TUNNEL_INFO for the new
VIDs, and br_afspec() handles both. A VLAN that wasn't on the list when
this loop ran isn't counted, but br_fill_vlan_tunnel_info() will emit it.

Second, the flag can change. br_setport() sets BR_VLAN_TUNNEL with a
lockless try_cmpxchg(), and br_setlink() then calls br_afspec() to install
mappings. If sizing saw the bit clear, nothing is reserved:

net/bridge/br_netlink.c:br_get_link_af_size_filtered() {
    ...
	if (p && test_bit(BR_VLAN_TUNNEL_BIT, &p->flags))
		vinfo_sz += br_get_vlan_tunnel_info_size(vg);
    ...
}

The fill can then see the bit set and emit one 28-byte attribute per
mapped VLAN:

net/bridge/br_netlink.c:br_fill_ifinfo() {
    ...
		if (port && test_bit(BR_VLAN_TUNNEL_BIT, &port->flags))
			err = br_fill_vlan_tunnel_info(skb, vg);
    ...
}

In either case, wouldn't br_fill_vlan_tinfo() return -EMSGSIZE and hit the
WARN_ON quoted in the commit message? Nine or more tunnel attributes that
can't be merged would exceed the usual kmalloc slack. This needs
CAP_NET_ADMIN in a network namespace, which includes unprivileged user
namespaces.

The "upper bound" in the new comment only holds while the VLAN list and
the flag stay the same, and nothing guarantees that without RTNL.

Could the size and the fill be computed from one consistent view, or use a
bound that doesn't depend on current membership? Another option is for the
non-RTNL callers to stop treating -EMSGSIZE as a bug, for example by
retrying with a larger skb.

[ ... ]

> @@ -81,7 +67,7 @@ int br_get_vlan_tunnel_info_size(struct net_bridge_vlan_group *vg)
>  		return 0;
>  
>  	rcu_read_lock();
> -	num_tinfos = __get_num_vlan_tunnel_infos(vg);
> +	num_tinfos = __get_max_num_vlan_tunnel_infos(vg);
>  	rcu_read_unlock();
>  
>  	return num_tinfos * __get_vlan_tinfo_size();

[Severity: Medium]
On a vlan_tunnel port, this now reserves __get_vlan_tinfo_size() (28
bytes) for every usable VLAN, whether it is mapped or not. Two examples:

- VLANs 1000-1999 mapped to consecutive VNIs go from 56 bytes to about
  28KB.
- A port with 4094 VLANs and few mappings now reserves about 114KB.

Is this cost intended on the br_info_notify() path itself? That path calls
nlmsg_new(br_nlmsg_size(), GFP_ATOMIC) from STP timer softirq context, so
these sizes become order-3 to order-5 atomic page allocations. When one
fails, the notification is dropped and rtnl_set_sk_err() reports ENOBUFS.

The inflated size also reaches messages outside the bridge code.
br_get_link_af_size_filtered() adds it regardless of filter_mask. It is
registered as get_link_af_size in br_af_ops, which has no fill_link_af:

static struct rtnl_af_ops br_af_ops __read_mostly = {
	.family			= AF_BRIDGE,
	.get_link_af_size	= br_get_link_af_size_filtered,
};

That has two effects:

- if_nlmsg_size()->rtnl_link_get_af_size() reserves the space for every
  generic rtmsg_ifinfo() or RTM_GETLINK message for the port, and nothing
  ever fills it.
- rtnl_calcit() takes the max of if_nlmsg_size() over every netdev in the
  netns as min_dump_alloc for RTM_GETLINK dumps that carry IFLA_EXT_MASK.
  One such port inflates those dumps for the whole namespace.

The commit message doesn't mention this cost. As noted above, the
over-reservation still doesn't guarantee the bound.

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