hsr: Use only one MAC address per node
Subsystems (4)
cover hsr: Use only one MAC address per node
Many places in the hsr module assumed that a single node could be using multiple MAC addresses to communicate in the network. The standard is explicit that PRP nodes should use the same MAC for frames on both ports and commit b65999e7238e ("net: hsr: sync hw addr of slave2 according to slave1 hw addr on PRP") made this clear. For HSR, the standard is less explicit. But after quite some discussions and reading standards, Fernando, Sebastian, and I concluded that it never mentions different MAC addresses for HSR either and instead often suggests using equal addresses for both ports and this should be what hsr interfaces do.
A short history of how this assumption formed in the kernel supports this as well: the original HSRv0 code implemented IEC 62439-3:2010 where node tables had two MAC addresses per node. It was an optional feature to support an unspecified address substitution mechanism for _PRP_, also referred to as PICS_SUBS. Note that we never even supported PRPv0 from the :2010 standard. In IEC 62439-3:2012, the feature was explicitly removed. But with two addresses per node in the node table and selftests setting different addresses for both interfaces, the assumption emerged that all nodes can have two addresses. In :2010, this was optional and the standard is written so that a node not supporting PICS_SUBS could just ignore it. Since 2012:, nodes must use the same address for both ports in the ring.
To prevent misconfiguration and simplify the hsr code, this patchset removes the notion of two different MAC addresses for one node in the network entirely. The first patch sets equal addresses on both ports so that we are not running in invalid configurations. I also updates the selftest to not use/expect different addresses on the two ports. The second patch removes MAC address B from the node table and thereby eliminates a lot of code, including the node merging.
I am posting as an RFC for now mostly for two reasons: First, I want to make sure it's generally accepted to have only a single MAC address per node and give everyone the chance to speak up against this.
Second, I adapted the PRP address handling to HSR as well, i.e., to copy the address from port A to port B and master, mostly because it is low effort. I am not sure though if this is the best approach now that we touch this part of the code anyways. I have two alternative ideas how addresses should be handled:
1) Make the master control the port MAC addresses: when the hsr interface is created, generate a MAC address and assign it to both ports. Changing the address afterwards would also only go through the hsr interface which would update both ports.
2) Don't change the MAC addresses of the port interfaces at all. Instead behave similar to bridge where the ports keep their addresses and traffic from the bridge/master gets its own MAC address assigned.
What do you think? Do you prefer any of these approaches?
Thanks,
Felix
Felix Maurer (2):
hsr: Set equal MAC addresses on port A and B for HSR
hsr: Remove second MAC address from node table
net/hsr/hsr_debugfs.c | 9 +- net/hsr/hsr_device.c | 23 +- net/hsr/hsr_forward.c | 8 +- net/hsr/hsr_framereg.c | 250 ++---------------- net/hsr/hsr_framereg.h | 14 +- net/hsr/hsr_main.c | 18 +- net/hsr/hsr_main.h | 5 +- net/hsr/hsr_netlink.c | 18 +- tools/testing/selftests/net/hsr/hsr_ping.sh | 33 +-- tools/testing/selftests/net/hsr/hsr_redbox.sh | 13 +- .../testing/selftests/net/hsr/link_faults.sh | 26 -- 11 files changed, 62 insertions(+), 355 deletions(-)
base-commit: f6f3b36c15ed44de1fbb44e645e4fae8c4a4453e --
2.55.0
RFC 1/2 hsr: Set equal MAC addresses on port A and B for HSR
All hsr interfaces are created with their MAC address set to the MAC address of port A. Since commit b65999e7238e ("net: hsr: sync hw addr of slave2 according to slave1 hw addr on PRP"), we are copying the MAC address of port A to port B as well for PRP interfaces because for correct operation, a node in a PRP network must use the same MAC address on both ports. IOW, the frames sent over port A and port B must only differ in the LAN ID in their PRP redundancy control trailer, i.e., must not differ in their source MAC address.
The same applies to HSR: for correct operation of the ring, the frames sent from port A and port B must not differ in their source MAC address. Therefore, extend the address copying behavior from PRP to HSR interfaces.
In the selftests, we now set only the addresses of port A which is how HSR is intended to be used. The address is copied over to port B anyways. With only a single address per node, there is also no reason anymore to wait for nodes in the table to get merged. While touching the MAC addresses in the hsr_redbox test, I noticed that the bridge in ns3 and its ports have the same addresses; fixing that as well.
--- net/hsr/hsr_device.c | 6 ++-- net/hsr/hsr_main.c | 12 +++---- tools/testing/selftests/net/hsr/hsr_ping.sh | 33 +++---------------- tools/testing/selftests/net/hsr/hsr_redbox.sh | 13 +++----- .../testing/selftests/net/hsr/link_faults.sh | 26 --------------- 5 files changed, 16 insertions(+), 74 deletions(-)
net/hsr/hsr_device.c +2 -4
@@ -795,10 +795,8 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],if (res) goto err_unregister; -if (protocol_version ==PRP_V1 ) { -eth_hw_addr_set (slave[1 ], slave[0 ]->dev_addr); -call_netdevice_notifiers (NETDEV_CHANGEADDR , slave[1 ]); - } +eth_hw_addr_set (slave[1 ], slave[0 ]->dev_addr); +call_netdevice_notifiers (NETDEV_CHANGEADDR , slave[1 ]);if (interlink) { res =hsr_add_port (hsr, interlink,HSR_PT_INTERLINK , extack);
net/hsr/hsr_main.c +5 -7
@@ -79,13 +79,11 @@ static int hsr_netdev_notify(struct notifier_block *nb, unsigned long event,call_netdevice_notifiers (NETDEV_CHANGEADDR , master->dev); -if (hsr->prot_version ==PRP_V1 ) { - port =hsr_port_get_hsr (hsr,HSR_PT_SLAVE_B ); -if (port) { -eth_hw_addr_set (port->dev, dev->dev_addr); -call_netdevice_notifiers (NETDEV_CHANGEADDR , - port->dev); - } + port =hsr_port_get_hsr (hsr,HSR_PT_SLAVE_B ); +if (port) { +eth_hw_addr_set (port->dev, dev->dev_addr); +call_netdevice_notifiers (NETDEV_CHANGEADDR , + port->dev); } }
tools/testing/selftests/net/hsr/hsr_ping.sh +5 -28
@@ -51,25 +51,6 @@ do_ping_tests() do_ping "$ns3" "dead:beef:$netid::2" stop_if_error "Initial validation failed on IPv6." -# Wait until supervisor all supervision frames have been processed and the node -# entries have been merged. Otherwise duplicate frames will be observed which is -# valid at this stage. - echo "INFO: Wait for node table entries to be merged." - WAIT=5 - while [ ${WAIT} -gt 0 ] - do - grep 00:00:00:00:00:00 /sys/kernel/debug/hsr/hsr*/node_table - if [ $? -ne 0 ] - then - break - fi - sleep 1 - let "WAIT = WAIT - 1" - done - -# Just a safety delay in case the above check didn't handle it. - sleep 1 - echo "INFO: Longer ping test." do_ping_long "$ns1" "100.64.$netid.2" do_ping_long "$ns1" "dead:beef:$netid::2"
@@ -110,6 +91,11 @@ setup_hsr_interfaces() ip link add ns1eth2 netns "$ns1" type veth peer name ns3eth1 netns "$ns3" ip link add ns3eth2 netns "$ns3" type veth peer name ns2eth2 netns "$ns2" + # MAC addresses will be copied from LAN A interfaces + ip -net "$ns1" link set address 00:11:22:00:01:01 dev ns1eth1 + ip -net "$ns2" link set address 00:11:22:00:02:01 dev ns2eth1 + ip -net "$ns3" link set address 00:11:22:00:03:01 dev ns3eth1 + # HSRv0/1 ip -net "$ns1" link add name hsr1 type hsr slave1 ns1eth1 \ slave2 ns1eth2 supervision 45 version "$HSRv" proto 0
@@ -126,15 +112,6 @@ setup_hsr_interfaces() ip -net "$ns3" addr add 100.64.0.3/24 dev hsr3 ip -net "$ns3" addr add dead:beef:0::3/64 dev hsr3 nodad - ip -net "$ns1" link set address 00:11:22:00:01:01 dev ns1eth1 - ip -net "$ns1" link set address 00:11:22:00:01:02 dev ns1eth2 - - ip -net "$ns2" link set address 00:11:22:00:02:01 dev ns2eth1 - ip -net "$ns2" link set address 00:11:22:00:02:02 dev ns2eth2 - - ip -net "$ns3" link set address 00:11:22:00:03:01 dev ns3eth1 - ip -net "$ns3" link set address 00:11:22:00:03:02 dev ns3eth2 - # All Links up ip -net "$ns1" link set ns1eth1 up ip -net "$ns1" link set ns1eth2 up
tools/testing/selftests/net/hsr/hsr_redbox.sh +4 -9
@@ -23,10 +23,6 @@ do_complete_ping_test() do_ping "${ns5}" 100.64.0.1 stop_if_error "Initial validation failed." - # Wait for MGNT HSR frames being received and nodes being - # merged. - sleep 5 - echo "INFO: Longer ping test (HSR-SAN/RedBox)." # Ping from SAN to hsr1 (via hsr2) do_ping_long "${ns3}" 100.64.0.1
@@ -96,16 +92,15 @@ setup_hsr_interfaces() ip -n "${ns4}" link set ns4eth1 up ip -n "${ns5}" link set ns5eth1 up + # Setting MAC addresses to help with debugging ip -net "$ns1" link set address 00:11:22:00:01:01 dev ns1eth1 - ip -net "$ns1" link set address 00:11:22:00:01:02 dev ns1eth2 ip -net "$ns2" link set address 00:11:22:00:02:01 dev ns2eth1 - ip -net "$ns2" link set address 00:11:22:00:02:02 dev ns2eth2 ip -net "$ns2" link set address 00:11:22:00:02:03 dev ns2eth3 - ip -net "$ns3" link set address 00:11:22:00:03:11 dev ns3eth1 - ip -net "$ns3" link set address 00:11:22:00:03:11 dev ns3eth2 - ip -net "$ns3" link set address 00:11:22:00:03:11 dev ns3eth3 + ip -net "$ns3" link set address 00:11:22:00:03:01 dev ns3eth1 + ip -net "$ns3" link set address 00:11:22:00:03:02 dev ns3eth2 + ip -net "$ns3" link set address 00:11:22:00:03:03 dev ns3eth3 ip -net "$ns3" link set address 00:11:22:00:03:11 dev ns3br1 ip -net "$ns4" link set address 00:11:22:00:04:01 dev ns4eth1
tools/testing/selftests/net/hsr/link_faults.sh +0 -26
@@ -59,13 +59,8 @@ setup_hsr_topo() # MAC addresses (not needed for HSR operation, but helps with debugging) ip -net "$node1" link set address 00:11:22:00:01:01 dev vethA - ip -net "$node1" link set address 00:11:22:00:01:02 dev vethB - ip -net "$node2" link set address 00:11:22:00:02:01 dev vethA - ip -net "$node2" link set address 00:11:22:00:02:02 dev vethB - ip -net "$node3" link set address 00:11:22:00:03:01 dev vethA - ip -net "$node3" link set address 00:11:22:00:03:02 dev vethB # HSR interfaces ip -net "$node1" link add name hsr1 type hsr proto 0 version "$ver" \
@@ -134,35 +129,14 @@ setup_prp_topo() ip -net "$node2" link set prp2 up } -wait_for_hsr_node_table() -{ - log_info "Wait for node table entries to be merged." - WAIT=5 - while [ "${WAIT}" -gt 0 ]; do - nts=$(cat /sys/kernel/debug/hsr/hsr*/node_table) - - # We need entries in the node tables, and they need to be merged - if (echo "$nts" | grep -qE "^([0-9a-f]{2}:){5}") && \ - ! (echo "$nts" | grep -q "00:00:00:00:00:00"); then - return - fi - - sleep 1 - ((WAIT--)) - done - check_err 1 "Failed to wait for merged node table entries" -} - setup_topo() { local proto="$1" if [ "$proto" = "HSRv0" ]; then setup_hsr_topo 0 - wait_for_hsr_node_table elif [ "$proto" = "HSRv1" ]; then setup_hsr_topo 1 - wait_for_hsr_node_table elif [ "$proto" = "PRP" ]; then setup_prp_topo else
Signature · 1 line
2.55.0
RFC 2/2 hsr: Remove second MAC address from node table
In reply to Felix MaurerSince its creation, the hsr module supported two MAC addresses per node in the node table. Presumably, this was initially implemented to support an address substitution mechanism for PRP. The IEC 62439-3:2010 calls this property PICS_SUBS and it is optional to implement:
"Some applications require different MAC addresses on the redundant ports, and these MAC addresses may be different from the default MAC address of that node. This involves address substitution mechanisms which are not specified in this International Standard. However, the basic protocol and the frame format are prepared for such extension." (IEC 62439-3:2010, 4.1.7)
Note that this is from IEC 62439-3:2010, which standardizes PRPv0 which we do not support at all. IEC 62439-3:2012 explicitly removed this address substitution mechanism completely, leaving only a single MAC address per node in the node table.
While the initial implementation aimed at supporting PICS_SUBS, this developed into a situation where a lot of the code in the hsr module assumed that nodes can/should have different MAC addresses on their two ports. This is not the case.
Therefore, remove all references to MAC address B and concepts depending on it (like the addr_B_port, i.e., the port where the second MAC address should be used; and notably the address A terminology). This allows to get rid of the node merging code as well. As the supervision frame parsing was mostly superficial afterwards and we don't do anything with the data, it is removed as well. Proper parsing of supervision frames can be added at a later point again (probably after reworking the whole rx path a bit).
--- net/hsr/hsr_debugfs.c | 9 +- net/hsr/hsr_device.c | 17 ++- net/hsr/hsr_forward.c | 8 +- net/hsr/hsr_framereg.c | 250 +++++------------------------------------ net/hsr/hsr_framereg.h | 14 +-- net/hsr/hsr_main.c | 6 +- net/hsr/hsr_main.h | 5 +- net/hsr/hsr_netlink.c | 18 +-- 8 files changed, 46 insertions(+), 281 deletions(-)
net/hsr/hsr_debugfs.c +3 -6
@@ -23,8 +23,7 @@ hsr_node_table_show(struct seq_file *sfp, void *data)seq_printf (sfp,"Node Table entries for (%s) device\n" , (priv->prot_version ==PRP_V1 ?"PRP" :"HSR" )); -seq_puts (sfp,"MAC-Address-A, MAC-Address-B, time_in[A], " ); -seq_puts (sfp,"time_in[B], Address-B port, " ); +seq_puts (sfp,"MAC-Address-A, time_in[A], time_in[B], " );if (priv->prot_version ==PRP_V1 )seq_puts (sfp,"SAN-A, SAN-B, DAN-P\n" );else
@@ -33,13 +32,11 @@ hsr_node_table_show(struct seq_file *sfp, void *data)rcu_read_lock ();list_for_each_entry_rcu (node, &priv->node_db, mac_list) {/* skip self node */ -if (hsr_addr_is_self (priv, node->macaddress_A)) +if (hsr_addr_is_self (priv, node->macaddress))continue ; -seq_printf (sfp,"%pM " , &node->macaddress_A[0 ]); -seq_printf (sfp,"%pM " , &node->macaddress_B[0 ]); +seq_printf (sfp,"%pM " , &node->macaddress[0 ]);seq_printf (sfp,"%10lx, " , node->time_in[HSR_PT_SLAVE_A ]);seq_printf (sfp,"%10lx, " , node->time_in[HSR_PT_SLAVE_B ]); -seq_printf (sfp,"%14x, " , node->addr_B_port);if (priv->prot_version ==PRP_V1 ) seq_printf(sfp, "%5x, %5x, %5x\n",
net/hsr/hsr_device.c +8 -9
@@ -341,9 +341,9 @@ static void send_hsr_supervision_frame(struct hsr_port *port, hsr_stag->tlv.HSR_TLV_length = hsr->prot_version ?sizeof (struct hsr_sup_payload ) :12 ; -/* Payload: MacAddressA / SAN MAC from ProxyNodeTable */ +/* Payload: MacAddress / SAN MAC from ProxyNodeTable */ hsr_sp =skb_put (skb,sizeof (struct hsr_sup_payload )); -ether_addr_copy (hsr_sp->macaddress_A, addr); +ether_addr_copy (hsr_sp->macaddress, addr);if (hsr->redbox &&hsr_is_node_in_db (&hsr->proxy_node_db, addr)) {
@@ -353,7 +353,7 @@ static void send_hsr_supervision_frame(struct hsr_port *port,/* Payload: MacAddressRedBox */ hsr_sp =skb_put (skb,sizeof (struct hsr_sup_payload )); -ether_addr_copy (hsr_sp->macaddress_A, hsr->macaddress_redbox); +ether_addr_copy (hsr_sp->macaddress, hsr->macaddress_redbox); }if (skb_put_padto (skb,ETH_ZLEN )) {
@@ -393,9 +393,9 @@ static void send_prp_supervision_frame(struct hsr_port *master, hsr_stag->tlv.HSR_TLV_type =PRP_TLV_LIFE_CHECK_DD ; hsr_stag->tlv.HSR_TLV_length =sizeof (struct hsr_sup_payload ); -/* Payload: MacAddressA */ +/* Payload: MacAddress */ hsr_sp =skb_put (skb,sizeof (struct hsr_sup_payload )); -ether_addr_copy (hsr_sp->macaddress_A, master->dev->dev_addr); +ether_addr_copy (hsr_sp->macaddress, master->dev->dev_addr);if (skb_put_padto (skb,ETH_ZLEN )) {spin_unlock_bh (&hsr->seqnr_lock);
@@ -445,10 +445,10 @@ static void hsr_proxy_announce(struct timer_list *t) goto done;list_for_each_entry_rcu (node, &hsr->proxy_node_db, mac_list) { -if (hsr_addr_is_redbox (hsr, node->macaddress_A)) +if (hsr_addr_is_redbox (hsr, node->macaddress))continue ; hsr->proto_ops->send_sv_frame (interlink, &interval, - node->macaddress_A); + node->macaddress); }if (is_admin_up (interlink->dev)) {
@@ -745,8 +745,7 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2], }/* Make sure we recognize frames from ourselves in hsr_handle_frame() */ - res =hsr_create_self_node (hsr, hsr_dev->dev_addr, - slave[1 ]->dev_addr); + res =hsr_create_self_node (hsr, hsr_dev->dev_addr);if (res <0 )return res;
net/hsr/hsr_forward.c +1 -7
@@ -28,9 +28,6 @@ struct hsr_node;* 2) Use the LifeCheck frames to detect ring breaks. I.e. if no LifeCheck * frame is received from a particular node, we know something is wrong. * We just register these (as with normal frames) and throw them away. -* -* 3) Allow different MAC addresses for the two slave interfaces, using the -* MacAddressA field. */ static bool is_supervision_frame (struct hsr_priv *hsr,struct sk_buff *skb) {
@@ -143,7 +140,7 @@ static bool is_proxy_supervision_frame(struct hsr_priv *hsr,* frame with MAC addresses from own ProxyNodeTable. */ return hsr_is_node_in_db (&hsr->proxy_node_db, - payload->macaddress_A); + payload->macaddress); } static struct sk_buff *create_stripped_skb_hsr(struct sk_buff *skb_in,
@@ -403,7 +400,6 @@ static void hsr_deliver_master(struct sk_buff *skb, struct net_device *dev,int res, recv_len; was_multicast_frame = (skb->pkt_type ==PACKET_MULTICAST ); -hsr_addr_subst_source (node_src, skb);skb_pull (skb,ETH_HLEN ); recv_len = skb->len; res =netif_rx (skb);
@@ -421,8 +417,6 @@ static int hsr_xmit(struct sk_buff *skb, struct hsr_port *port, struct hsr_frame_info *frame) {if (frame->port_rcv->type ==HSR_PT_MASTER ) { -hsr_addr_subst_dest (frame->node_src, skb, port); -/* Address substitution (IEC62439-3 pp 26, 50): replace mac * address of outgoing frame with that of the outgoing slave's. */
net/hsr/hsr_framereg.c +28 -222
413 lines not on this page · raw message
net/hsr/hsr_framereg.h +2 -12
@@ -38,10 +38,6 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame);bool hsr_addr_is_self (struct hsr_priv *hsr,unsigned char *addr);bool hsr_addr_is_redbox (struct hsr_priv *hsr,unsigned char *addr); -void hsr_addr_subst_source (struct hsr_node *node,struct sk_buff *skb); -void hsr_addr_subst_dest (struct hsr_node *node_src,struct sk_buff *skb, -struct hsr_port *port); -void hsr_register_frame_in (struct hsr_node *node,struct hsr_port *port,u16 sequence_nr);int hsr_register_frame_out (struct hsr_port *port,struct hsr_frame_info *frame);
@@ -50,16 +46,13 @@ void hsr_prune_nodes(struct timer_list *t);void hsr_prune_proxy_nodes (struct timer_list *t);int hsr_create_self_node (struct hsr_priv *hsr, -const unsigned char addr_a[ETH_ALEN ], -const unsigned char addr_b[ETH_ALEN ]); +const unsigned char addr[ETH_ALEN ]);void *hsr_get_next_node (struct hsr_priv *hsr,void *_pos,unsigned char addr[ETH_ALEN ]); int hsr_get_node_data(struct hsr_priv *hsr, const unsigned char *addr, - unsigned char addr_b[ETH_ALEN], - unsigned int *addr_b_ifindex, int *if1_age, u16 *if1_seq, int *if2_age,
@@ -99,10 +92,7 @@ struct hsr_node {struct list_head mac_list;/* Protect R/W access seq_blocks */ spinlock_t seq_out_lock; -unsigned char macaddress_A[ETH_ALEN ]; -unsigned char macaddress_B[ETH_ALEN ]; -/* Local slave through which AddrB frames are received from this node */ -enum hsr_port_type addr_B_port; +unsigned char macaddress[ETH_ALEN ];unsigned long time_in[HSR_PT_PORTS ];bool time_in_stale[HSR_PT_PORTS ];/* if the node is a SAN */
net/hsr/hsr_main.c +1 -5
@@ -88,12 +88,8 @@ static int hsr_netdev_notify(struct notifier_block *nb, unsigned long event, }/* Make sure we recognize frames from ourselves in hsr_handle_frame() */ - port =hsr_port_get_hsr (hsr,HSR_PT_SLAVE_B ); res =hsr_create_self_node (hsr, - master->dev->dev_addr, - port ? - port->dev->dev_addr : - master->dev->dev_addr); + master->dev->dev_addr);if (res)netdev_warn (master->dev,"Could not update HSR node address.\n" );
net/hsr/hsr_main.h +2 -3
@@ -97,7 +97,7 @@ struct hsr_sup_tag { } __packed;struct hsr_sup_payload { -unsigned char macaddress_A[ETH_ALEN ]; +unsigned char macaddress[ETH_ALEN ]; } __packed;static inline void set_hsr_stag_path (struct hsr_sup_tag *hst,u16 path)
@@ -181,8 +181,7 @@ struct hsr_proto_ops { };struct hsr_self_node { -unsigned char macaddress_A[ETH_ALEN ]; -unsigned char macaddress_B[ETH_ALEN ]; +unsigned char macaddress[ETH_ALEN ];struct rcu_head rcu_head; };
net/hsr/hsr_netlink.c +1 -17
@@ -327,12 +327,10 @@ static int hsr_get_node_status(struct sk_buff *skb_in, struct genl_info *info)void *msg_head;struct hsr_priv *hsr;struct hsr_port *port; -unsigned char hsr_node_addr_b[ETH_ALEN ];int hsr_node_if1_age;u16 hsr_node_if1_seq;int hsr_node_if2_age;u16 hsr_node_if2_seq; -int addr_b_ifindex;int res;if (!info)
@@ -376,8 +374,6 @@ static int hsr_get_node_status(struct sk_buff *skb_in, struct genl_info *info) res = hsr_get_node_data(hsr, (unsigned char *) nla_data(info->attrs[HSR_A_NODE_ADDR]), - hsr_node_addr_b, - &addr_b_ifindex, &hsr_node_if1_age, &hsr_node_if1_seq, &hsr_node_if2_age,
@@ -390,18 +386,6 @@ static int hsr_get_node_status(struct sk_buff *skb_in, struct genl_info *info)if (res <0 ) goto nla_put_failure; -if (addr_b_ifindex >-1 ) { - res =nla_put (skb_out,HSR_A_NODE_ADDR_B ,ETH_ALEN , - hsr_node_addr_b); -if (res <0 ) - goto nla_put_failure; - - res =nla_put_u32 (skb_out,HSR_A_ADDR_B_IFINDEX , - addr_b_ifindex); -if (res <0 ) - goto nla_put_failure; - } - res =nla_put_u32 (skb_out,HSR_A_IF1_AGE , hsr_node_if1_age);if (res <0 ) goto nla_put_failure;
@@ -450,7 +434,7 @@ static int hsr_get_node_status(struct sk_buff *skb_in, struct genl_info *info)return res; } -/* Get a list of MacAddressA of all nodes known to this node (including self). +/* Get a list of MAC addresses of all nodes known to this node (including self). */ static int hsr_get_node_list (struct sk_buff *skb_in,struct genl_info *info) {
Signature · 1 line
2.55.0
Quoted 50 lines · Felix Maurer
Many places in the hsr module assumed that a single node could be using multiple MAC addresses to communicate in the network. The standard is explicit that PRP nodes should use the same MAC for frames on both ports and commit
b65999e7238e("net: hsr: sync hw addr of slave2 according to slave1 hw addr on PRP") made this clear. For HSR, the standard is less explicit. But after quite some discussions and reading standards, Fernando, Sebastian, and I concluded that it never mentions different MAC addresses for HSR either and instead often suggests using equal addresses for both ports and this should be what hsr interfaces do.A short history of how this assumption formed in the kernel supports this as well: the original HSRv0 code implemented IEC 62439-3:2010 where node tables had two MAC addresses per node. It was an optional feature to support an unspecified address substitution mechanism for _PRP_, also referred to as
PICS_SUBS. Note that we never even supported PRPv0 from the :2010 standard. In IEC 62439-3:2012, the feature was explicitly removed. But with two addresses per node in the node table and selftests setting different addresses for both interfaces, the assumption emerged that all nodes can have two addresses. In :2010, this was optional and the standard is written so that a node not supportingPICS_SUBScould just ignore it. Since 2012:, nodes must use the same address for both ports in the ring.To prevent misconfiguration and simplify the hsr code, this patchset removes the notion of two different MAC addresses for one node in the network entirely. The first patch sets equal addresses on both ports so that we are not running in invalid configurations. I also updates the selftest to not use/expect different addresses on the two ports. The second patch removes MAC address B from the node table and thereby eliminates a lot of code, including the node merging.
I am posting as an RFC for now mostly for two reasons: First, I want to make sure it's generally accepted to have only a single MAC address per node and give everyone the chance to speak up against this.
Second, I adapted the PRP address handling to HSR as well, i.e., to copy the address from port A to port B and master, mostly because it is low effort. I am not sure though if this is the best approach now that we touch this part of the code anyways. I have two alternative ideas how addresses should be handled:
1) Make the master control the port MAC addresses: when the hsr interface is created, generate a MAC address and assign it to both ports. Changing the address afterwards would also only go through the hsr interface which would update both ports.
2) Don't change the MAC addresses of the port interfaces at all. Instead behave similar to bridge where the ports keep their addresses and traffic from the bridge/master gets its own MAC address assigned.
Quoted 2 lines
What do you think? Do you prefer any of these approaches?
Hi Felix, thank you very much for this work. I agree with the overall analysis you provided here.
I would try to go for the same solution in both PRP and HSR. Why the current approach isn't suitable? If we are going to change the approach (for a good reason) I would go for 1).
FWIW, I didn't test this yet.
Quoted 29 lines
Thanks,
FelixCc Fernando Fernandez Mancera <[email protected]>Cc Sebastian Andrzej Siewior <[email protected]>Felix Maurer (2):
hsr: Set equal MAC addresses on port A and B for HSR
hsr: Remove second MAC address from node tablenet/hsr/hsr_debugfs.c | 9 +- net/hsr/hsr_device.c | 23 +- net/hsr/hsr_forward.c | 8 +- net/hsr/hsr_framereg.c | 250 ++---------------- net/hsr/hsr_framereg.h | 14 +- net/hsr/hsr_main.c | 18 +- net/hsr/hsr_main.h | 5 +- net/hsr/hsr_netlink.c | 18 +- tools/testing/selftests/net/hsr/hsr_ping.sh | 33 +-- tools/testing/selftests/net/hsr/hsr_redbox.sh | 13 +- .../testing/selftests/net/hsr/link_faults.sh | 26 -- 11 files changed, 62 insertions(+), 355 deletions(-)base-commit:
f6f3b36c15ed44de1fbb44e645e4fae8c4a4453e--
2.55.0
Hi Felix,
Quoted 8 lines · Felix Maurer
To prevent misconfiguration and simplify the hsr code, this patchset removes the notion of two different MAC addresses for one node in the network entirely. The first patch sets equal addresses on both ports so that we are not running in invalid configurations. I also updates the selftest to not use/expect different addresses on the two ports. The second patch removes MAC address B from the node table and thereby eliminates a lot of code, including the node merging.
Quoted 7 lines
I am posting as an RFC for now mostly for two reasons: First, I want to make sure it's generally accepted to have only a single MAC address per node and give everyone the chance to speak up against this.
Second, I adapted the PRP address handling to HSR as well, i.e., to copy the address from port A to port B and master, mostly because it is low effort. I am not sure though if this is the best approach now that we
+1 for this behavior. Bonding also use this one as default behavior.
Thanks
Hangbin