Re: [PATCH net-next v1 1/4] net: hsr: serialize seq_blocks merge across nodes

[Felix Maurer <fmaurer@redhat.com>]

On Tue, Mar 24, 2026 at 03:35:00PM +0100, luka.gejak@linux.dev wrote:
> From: Luka Gejak <luka.gejak@linux.dev>
>
> During node merging, hsr_handle_sup_frame() walks node_curr->seq_blocks
> to update node_real without holding node_curr->seq_out_lock. This
> allows concurrent mutations from duplicate registration paths, risking
> inconsistent state or XArray/bitmap corruption.

I'm not sure if I see "duplicate registration" happening (without a more
serious problem in the network).

> Fix this by locking both nodes' seq_out_lock during the merge.
> To prevent ABBA deadlocks, locks are acquired in order of memory
> address.

But I agree that this is probably a good idea, as long as the node with
wrong macaddressA is still in the node table and might still see updates
in the sequence number tracking because we can still receive traffic for
it. I originally didn't consider this to be too bad, but if a block is
reused we could be dropping some valid packets.

Reviewed-by: Felix Maurer <fmaurer@redhat.com>

> Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
> ---
>  net/hsr/hsr_framereg.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> index 577fb588bc2f..d09875b33588 100644
> --- a/net/hsr/hsr_framereg.c
> +++ b/net/hsr/hsr_framereg.c
> @@ -123,6 +123,40 @@ static void hsr_free_node_rcu(struct rcu_head *rn)
>  	hsr_free_node(node);
>  }
>
> +static void hsr_lock_seq_out_pair(struct hsr_node *node_a,
> +				  struct hsr_node *node_b)
> +{
> +	if (node_a == node_b) {
> +		spin_lock_bh(&node_a->seq_out_lock);
> +		return;
> +	}
> +
> +	if (node_a < node_b) {
> +		spin_lock_bh(&node_a->seq_out_lock);
> +		spin_lock_nested(&node_b->seq_out_lock, SINGLE_DEPTH_NESTING);
> +	} else {
> +		spin_lock_bh(&node_b->seq_out_lock);
> +		spin_lock_nested(&node_a->seq_out_lock, SINGLE_DEPTH_NESTING);
> +	}
> +}
> +
> +static void hsr_unlock_seq_out_pair(struct hsr_node *node_a,
> +				    struct hsr_node *node_b)
> +{
> +	if (node_a == node_b) {
> +		spin_unlock_bh(&node_a->seq_out_lock);
> +		return;
> +	}
> +
> +	if (node_a < node_b) {
> +		spin_unlock(&node_b->seq_out_lock);
> +		spin_unlock_bh(&node_a->seq_out_lock);
> +	} else {
> +		spin_unlock(&node_a->seq_out_lock);
> +		spin_unlock_bh(&node_b->seq_out_lock);
> +	}
> +}
> +
>  void hsr_del_nodes(struct list_head *node_db)
>  {
>  	struct hsr_node *node;
> @@ -432,7 +466,7 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame)
>  	}
>
>  	ether_addr_copy(node_real->macaddress_B, ethhdr->h_source);
> -	spin_lock_bh(&node_real->seq_out_lock);
> +	hsr_lock_seq_out_pair(node_real, node_curr);
>  	for (i = 0; i < HSR_PT_PORTS; i++) {
>  		if (!node_curr->time_in_stale[i] &&
>  		    time_after(node_curr->time_in[i], node_real->time_in[i])) {
> @@ -455,7 +489,7 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame)
>  				  src_blk->seq_nrs[i], HSR_SEQ_BLOCK_SIZE);
>  		}
>  	}
> -	spin_unlock_bh(&node_real->seq_out_lock);
> +	hsr_unlock_seq_out_pair(node_real, node_curr);
>  	node_real->addr_B_port = port_rcv->type;
>
>  	spin_lock_bh(&hsr->list_lock);
> --
> 2.53.0
>