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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2026-04-22 10:31:39 [+0200], Felix Maurer wrote:
> On Tue, Apr 21, 2026 at 10:50:01PM +0800, Ren Wei wrote:
> > 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
> > @@ -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?

The node is allocated upfront. Then it iterates here and we only end up
counting through the full list if there is no match. This is under a
lock so "many clients" are serialized. If we allocate the node later
then we need to do it under the lock.

I don't think the node count exceeds 100 in production. So having a
counter which is incremented while adding to the list and decremented
while removing items from the list would optimize the "worst case". So
instead traversing the list with 1000 we would just give up.

The "oom block" works regardless. This does not affect the common case
where we have far less nodes.

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

netdev_warn_once() probably.

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

Sebastian