Re: [PATCH net v3 1/1] net: hsr: limit node table growth

[Felix Maurer <fmaurer@redhat.com>]

On Tue, Apr 21, 2026 at 10:50:01PM +0800, Ren Wei wrote:
> From: Haoze Xie <royenheart@gmail.com>
>
> The HSR/PRP node learning paths allocate one persistent entry per
> previously unseen source MAC. Since learned entries stay alive until the
> prune timer catches up, the node tables can otherwise grow without a
> bound under high churn of learned senders.
>
> Limit the number of learned entries in each node table and stop adding
> new ones once the configured limit is reached. This keeps node-table
> resource use bounded across the affected learning paths.
Hi,

thank you for giving this approach a try!

[snip]
> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> index d09875b33588..8a5a2a54a81f 100644
> --- a/net/hsr/hsr_framereg.c
> +++ b/net/hsr/hsr_framereg.c
> @@ -14,12 +14,18 @@
>  #include <kunit/visibility.h>
>  #include <linux/if_ether.h>
>  #include <linux/etherdevice.h>
> +#include <linux/moduleparam.h>
>  #include <linux/slab.h>
>  #include <linux/rculist.h>
>  #include "hsr_main.h"
>  #include "hsr_framereg.h"
>  #include "hsr_netlink.h"
>
> +static unsigned int hsr_node_table_size = 1024;
> +module_param_named(node_table_size, hsr_node_table_size, uint, 0644);
> +MODULE_PARM_DESC(node_table_size,
> +		 "Maximum number of learned entries in each HSR/PRP node table (0 = unlimited)");
> +
>  bool hsr_addr_is_redbox(struct hsr_priv *hsr, unsigned char *addr)
>  {
>  	if (!hsr->redbox || !is_valid_ether_addr(hsr->macaddress_redbox))
> @@ -189,6 +195,7 @@ static struct hsr_node *hsr_add_node(struct hsr_priv *hsr,
>  				     enum hsr_port_type rx_port)
>  {
>  	struct hsr_node *new_node, *node = NULL;
> +	unsigned int node_count = 0;
>  	unsigned long now;
>  	size_t block_sz;
>  	int i;
> @@ -226,20 +233,31 @@ static struct hsr_node *hsr_add_node(struct hsr_priv *hsr,
>  	spin_lock_bh(&hsr->list_lock);
>  	list_for_each_entry_rcu(node, node_db, mac_list,
>  				lockdep_is_held(&hsr->list_lock)) {
> +		node_count++;

I'm not sure if this on-the-fly node counting is the best solution here.
My concern is that it comes quite late in the process, i.e., after we
already allocated a bunch of memory, etc. As we are discussing a
scenario where a lot of entries are created, maybe we shouldn't even
allocate a new_node if the table is already full? For example by storing
the node_count in hsr_priv and checking it early in the function?

>  		if (ether_addr_equal(node->macaddress_A, addr))
> -			goto out;
> +			goto out_found;
>  		if (ether_addr_equal(node->macaddress_B, addr))
> -			goto out;
> +			goto out_found;
>  	}
> +
> +	if (hsr_node_table_size && node_count >= hsr_node_table_size)
> +		goto out_drop;

I think it would be good to somehow make this situation transparent to
the user, so they can react if this an undesired behavior (for example,
because they simply have a large network and need a large node table).

>  	list_add_tail_rcu(&new_node->mac_list, node_db);
>  	spin_unlock_bh(&hsr->list_lock);
>  	return new_node;
> -out:
> +out_found:
>  	spin_unlock_bh(&hsr->list_lock);
> +	xa_destroy(&new_node->seq_blocks);
>  	kfree(new_node->block_buf);
> -free:
>  	kfree(new_node);
>  	return node;
> +out_drop:
> +	spin_unlock_bh(&hsr->list_lock);
> +	xa_destroy(&new_node->seq_blocks);
> +	kfree(new_node->block_buf);
> +free:
> +	kfree(new_node);
> +	return NULL;
>  }

The two cleanup paths are almost the same now. We usually attempt to
keep them unified to make sure that we do the correct cleanup steps in
all situations. So please keep them unified here as well.

Thanks,
   Felix