Re: [Bridge] [PATCH net 2/2] net: bridge: mcast: add and enforce startup query interval minimum

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Dec 27, 2021 at 07:21:16PM +0200, Nikolay Aleksandrov wrote:
> As reported[1] if startup query interval is set too low in combination with
> large number of startup queries and we have multiple bridges or even a
> single bridge with multiple querier vlans configured we can crash the
> machine. Add a 1 second minimum which must be enforced by overwriting the
> value if set lower (i.e. without returning an error) to avoid breaking
> user-space. If that happens a log message is emitted to let the admin know
> that the startup interval has been set to the minimum. It doesn't make
> sense to make the startup interval lower than the normal query interval
> so use the same value of 1 second. The issue has been present since these
> intervals could be user-controlled.
> 
> [1] https://lore.kernel.org/netdev/e8b9ce41-57b9-b6e2-a46a-ff9c791cf0ba@gmail.com/
> 
> Fixes: d902eee43f19 ("bridge: Add multicast count/interval sysfs entries")
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com>
> ---
>  net/bridge/br_multicast.c    | 16 ++++++++++++++++
>  net/bridge/br_netlink.c      |  2 +-
>  net/bridge/br_private.h      |  3 +++
>  net/bridge/br_sysfs_br.c     |  2 +-
>  net/bridge/br_vlan_options.c |  2 +-
>  5 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 998da4a2d209..de2409889489 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -4538,6 +4538,22 @@ void br_multicast_set_query_intvl(struct net_bridge_mcast *brmctx,
>  	brmctx->multicast_query_interval = intvl_jiffies;
>  }
>  
> +void br_multicast_set_startup_query_intvl(struct net_bridge_mcast *brmctx,
> +					  unsigned long val)
> +{
> +	unsigned long intvl_jiffies = clock_t_to_jiffies(val);
> +
> +	if (intvl_jiffies < BR_MULTICAST_STARTUP_QUERY_INTVL_MIN) {
> +		br_info(brmctx->br,
> +			"trying to set multicast startup query interval below minimum, setting to %lu (%ums)\n",
> +			jiffies_to_clock_t(BR_MULTICAST_STARTUP_QUERY_INTVL_MIN),
> +			jiffies_to_msecs(BR_MULTICAST_STARTUP_QUERY_INTVL_MIN));
> +		intvl_jiffies = BR_MULTICAST_STARTUP_QUERY_INTVL_MIN;
> +	}
> +
> +	brmctx->multicast_startup_query_interval = intvl_jiffies;
> +}
> +
>  /**
>   * br_multicast_list_adjacent - Returns snooped multicast addresses
>   * @dev:	The bridge port adjacent to which to retrieve addresses
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 701dd8b8455e..2ff83d84230d 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1369,7 +1369,7 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
>  	if (data[IFLA_BR_MCAST_STARTUP_QUERY_INTVL]) {
>  		u64 val = nla_get_u64(data[IFLA_BR_MCAST_STARTUP_QUERY_INTVL]);
>  
> -		br->multicast_ctx.multicast_startup_query_interval = clock_t_to_jiffies(val);
> +		br_multicast_set_startup_query_intvl(&br->multicast_ctx, val);
>  	}
>  
>  	if (data[IFLA_BR_MCAST_STATS_ENABLED]) {
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 4ed7f11042e8..2187a0c3fd22 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -29,6 +29,7 @@
>  
>  #define BR_MULTICAST_DEFAULT_HASH_MAX 4096
>  #define BR_MULTICAST_QUERY_INTVL_MIN msecs_to_jiffies(1000)
> +#define BR_MULTICAST_STARTUP_QUERY_INTVL_MIN BR_MULTICAST_QUERY_INTVL_MIN
>  
>  #define BR_HWDOM_MAX BITS_PER_LONG
>  
> @@ -966,6 +967,8 @@ size_t br_multicast_querier_state_size(void);
>  size_t br_rports_size(const struct net_bridge_mcast *brmctx);
>  void br_multicast_set_query_intvl(struct net_bridge_mcast *brmctx,
>  				  unsigned long val);
> +void br_multicast_set_startup_query_intvl(struct net_bridge_mcast *brmctx,
> +					  unsigned long val);
>  
>  static inline bool br_group_is_l2(const struct br_ip *group)
>  {
> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> index f5bd1114a434..7b0c19772111 100644
> --- a/net/bridge/br_sysfs_br.c
> +++ b/net/bridge/br_sysfs_br.c
> @@ -706,7 +706,7 @@ static ssize_t multicast_startup_query_interval_show(
>  static int set_startup_query_interval(struct net_bridge *br, unsigned long val,
>  				      struct netlink_ext_ack *extack)
>  {
> -	br->multicast_ctx.multicast_startup_query_interval = clock_t_to_jiffies(val);
> +	br_multicast_set_startup_query_intvl(&br->multicast_ctx, val);
>  	return 0;
>  }
>  
> diff --git a/net/bridge/br_vlan_options.c b/net/bridge/br_vlan_options.c
> index bf1ac0874279..a6382973b3e7 100644
> --- a/net/bridge/br_vlan_options.c
> +++ b/net/bridge/br_vlan_options.c
> @@ -535,7 +535,7 @@ static int br_vlan_process_global_one_opts(const struct net_bridge *br,
>  		u64 val;
>  
>  		val = nla_get_u64(tb[BRIDGE_VLANDB_GOPTS_MCAST_STARTUP_QUERY_INTVL]);
> -		v->br_mcast_ctx.multicast_startup_query_interval = clock_t_to_jiffies(val);
> +		br_multicast_set_startup_query_intvl(&v->br_mcast_ctx, val);
>  		*changed = true;
>  	}
>  	if (tb[BRIDGE_VLANDB_GOPTS_MCAST_QUERIER]) {
> -- 
> 2.33.1
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>