netdev - Re: [RFC] net: hsr: Provide RedBox support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 1 Mar 2024 15:38:41 +0800
From: "Ziyang Xuan (William)" <william.xuanziyang@...wei.com>
To: Lukasz Majewski <lukma@...x.de>, Oleksij Rempel <o.rempel@...gutronix.de>
CC: Andrew Lunn <andrew@...n.ch>, Eric Dumazet <edumazet@...gle.com>, Florian
 Fainelli <f.fainelli@...il.com>, Vladimir Oltean <olteanv@...il.com>, "David
 S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
	<netdev@...r.kernel.org>, <Tristram.Ha@...rochip.com>, Sebastian Andrzej
 Siewior <bigeasy@...utronix.de>, Paolo Abeni <pabeni@...hat.com>, Ravi
 Gunasekaran <r-gunasekaran@...com>, Simon Horman <horms@...nel.org>, Wojciech
 Drewek <wojciech.drewek@...el.com>, Nikita Zhandarovich
	<n.zhandarovich@...tech.ru>, Murali Karicheri <m-karicheri2@...com>, Dan
 Carpenter <dan.carpenter@...aro.org>, Kristian Overskeid
	<koverskeid@...il.com>, Matthieu Baerts <matttbe@...nel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [RFC] net: hsr: Provide RedBox support

Give opinions only from the code level.

>  
>  void hsr_debugfs_rename(struct net_device *dev)
>  {
> @@ -95,6 +114,19 @@ void hsr_debugfs_init(struct hsr_priv *priv, struct net_device *hsr_dev)
>  		priv->node_tbl_root = NULL;
>  		return;
>  	}
> +
> +	if (!priv->redbox)
> +		return;
> +
> +	de = debugfs_create_file("proxy_node_table", S_IFREG | 0444,
> +				 priv->node_tbl_root, priv,
> +				 &hsr_proxy_node_table_fops);
> +	if (IS_ERR(de)) {
> +		pr_err("Cannot create hsr proxy node_table file\n");
> +		debugfs_remove(priv->node_tbl_root);
> +		priv->node_tbl_root = NULL;
> +		return;
> +	}
I think we can use "goto label" to reduce duplicate codes for error handling.

>  }
>  
> @@ -296,6 +298,7 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
>  	struct hsr_priv *hsr = master->hsr;
>  	__u8 type = HSR_TLV_LIFE_CHECK;
>  	struct hsr_sup_payload *hsr_sp;
> +	struct hsr_sup_tlv *hsr_stlv;
>  	struct hsr_sup_tag *hsr_stag;
>  	struct sk_buff *skb;
>  
> @@ -335,6 +338,16 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
>  	hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
>  	ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
>  
> +	if (hsr->redbox) {
> +		hsr_stlv = skb_put(skb, sizeof(struct hsr_sup_tlv));
> +		hsr_stlv->HSR_TLV_type = PRP_TLV_REDBOX_MAC;
> +		hsr_stlv->HSR_TLV_length = sizeof(struct hsr_sup_payload);
> +
> +		/* Payload: MacAddressRedBox */
> +		hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
> +		ether_addr_copy(hsr_sp->macaddress_A, hsr->macaddress_redbox);
> +	}
If hsr->redbox is true, hsr_sp->macaddress_A will be covered. Do ether_addr_copy() twice.
Is it better like this:

hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));

if (hsr->redbox) {
	...
	ether_addr_copy(hsr_sp->macaddress_A, hsr->macaddress_redbox);
} else {
	ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
}

> +
>  	if (skb_put_padto(skb, ETH_ZLEN)) {
>  		spin_unlock_bh(&hsr->seqnr_lock);
>  		return;

>  
> @@ -448,13 +455,14 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
>  		}
>  
>  		/* Check if frame is to be dropped. Eg. for PRP no forward
> -		 * between ports.
> +		 * between ports, or sending HSR supervision to RedBox.
>  		 */
>  		if (hsr->proto_ops->drop_frame &&
>  		    hsr->proto_ops->drop_frame(frame, port))
>  			continue;
>  
> -		if (port->type != HSR_PT_MASTER)
> +		if (port->type == HSR_PT_SLAVE_A ||
> +		    port->type == HSR_PT_SLAVE_B)

(port->type != HSR_PT_MASTER) is not equivalent to (port->type == HSR_PT_SLAVE_A || port->type == HSR_PT_SLAVE_B).
port->type may be HSR_PT_INTERLINK or others. Or here is a bugfix? Please check.

>  			skb = hsr->proto_ops->create_tagged_frame(frame, port);
>  		else
>  			skb = hsr->proto_ops->get_untagged_frame(frame, port);
> @@ -469,7 +477,9 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
>  			hsr_deliver_master(skb, port->dev, frame->node_src);
>  		} else {
>  			if (!hsr_xmit(skb, port, frame))
> -				sent = true;
> +				if (port->type == HSR_PT_SLAVE_A ||
> +				    port->type == HSR_PT_SLAVE_B)
> +					sent = true;
>  		}
>  	}
>  }

If my opinions be accepted, Can you add "Reviewed-by: Ziyang Xuan <william.xuanziyang@...wei.com>" at next version of patch?