| 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
| ||
|
Date: Fri, 1 Mar 2024 10:55:50 +0100
From: Lukasz Majewski <lukma@...x.de>
To: "Ziyang Xuan (William)" <william.xuanziyang@...wei.com>
Cc: Oleksij Rempel <o.rempel@...gutronix.de>, 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
Hi William,
> 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.
This code is already NAK'ed from network maintainers, as debugfs is not
acceptable to give any "API like" information.
Instead the "netlink" API shall be used to provide this information.
>
> > }
> >
> > @@ -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);
> }
It may be a bit misleading, as the hsr_sp is the payload for TLV fields
in HSR supervisory frames.
It shall be done as in the code as:
1. First you need to send this HSR "node" MAC address (the
ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr))
2. If the box is a RedBox, then this supervisory frame shall have
_appended_ another TLV with RedBox mac address (it can be the same
as the HSR node in the special case).
The hsr_sp->macaddress_A holds the address to the V (value) field of
the TLV.
>
> > +
> > 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.
Without Redbox support you don't use HSR_PT_INTERLINK. This port shall
behave in similar way to HSR_PT_MASTER - e.g. it will send/receive HSR
untagged frames. That is why the condition has been altered to only
prepare HSR tagged frames for HSR ring port A/B.
>
> > 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?
I will add your Reviewed-by: tag, no problem.
Thanks for the input :-)
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists