This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at:
https://sashiko.dev/#/patchset/[email protected]?part=3
---
--- Patch [3]: [PATCH net-next v4 3/4] net: hsr: allow PRP RedBox (interlink)
creation ---
commit bc0d779cf60ce71e2ec7abbdba515576e82afb69
Author: Xin Xie <[email protected]>
net: hsr: allow PRP RedBox (interlink) creation
With the PRP interlink datapath, duplicate discard and supervision support
in place, a PRP device can act as a RedBox. Remove the rtnetlink rejection
of "type hsr ... interlink <dev> proto 1"; the feature is implemented
unconditionally by the preceding patches.
> diff --git a/net/hsr/hsr_netlink.c b/net/hsr/hsr_netlink.c
> index 8099f2069a748..88940e8014b24 100644
> --- a/net/hsr/hsr_netlink.c
> +++ b/net/hsr/hsr_netlink.c
> @@ -121,14 +121,8 @@ static int hsr_newlink(struct net_device *dev,
> }
> }
>
> - if (proto == HSR_PROTOCOL_PRP) {
> + if (proto == HSR_PROTOCOL_PRP)
> proto_version = PRP_V1;
> - if (interlink) {
> - NL_SET_ERR_MSG_MOD(extack,
> - "Interlink only works with HSR");
> - return -EINVAL;
> - }
> - }
[Severity: Medium]
Since we are enabling interlink here, does this expose an issue with the
transmit statistics in hsr_forward_skb() that was added earlier in the
series?
When a frame is received on the interlink port from the SAN, or the RedBox
generates a proxy supervision frame, we call hsr_forward_skb() with the
interlink port.
In net/hsr/hsr_forward.c:hsr_forward_skb():
if (port->type == HSR_PT_MASTER || port->type == HSR_PT_INTERLINK) {
port->dev->stats.tx_packets++;
port->dev->stats.tx_bytes += skb->len;
This appears to unconditionally increment the port's tx_packets and tx_bytes,
even though the frame was received (not transmitted) or is being transmitted
out of the slave ports instead of the interlink port. Will this artificially
inflate the network statistics for the physical interlink device?
[Severity: Medium]
This is a pre-existing issue, but does removing this restriction expose a
memory leak in the hsr_dev_finalize() error path?
If hsr_add_port() succeeds for the first slave, its RX handler becomes active
and can receive frames, dynamically allocating nodes in hsr->node_db or
hsr->proxy_node_db via softirqs. If adding the interlink fails shortly after:
net/hsr/hsr_device.c:hsr_dev_finalize():
err_unregister:
hsr_del_ports(hsr);
err_add_master:
hsr_del_self_node(hsr);
if (unregister)
unregister_netdevice(hsr_dev);
return res;
The err_unregister path destroys the ports and unregisters the master device,
but omits a call to hsr_del_nodes(). Does this cause the rtnl core to free
the netdevice and leak the dynamically allocated hsr_node structures?
[Severity: High]
This isn't a bug introduced by this patch, but does allowing tagged frames
on the interlink port trigger a silent SKB data corruption?
When an HSR or PRP tagged frame is received on the interlink port, it is
forwarded to both slave ports. hsr_forward_do() calls create_tagged_frame()
for each slave.
In net/hsr/hsr_forward.c:hsr_create_tagged_frame():
hsr_set_path_id(frame, hsr_ethhdr, port);
return skb_clone(frame->skb_hsr, GFP_ATOMIC);
And similarly in net/hsr/hsr_forward.c:prp_create_tagged_frame():
prp_set_lan_id(trailer, port);
return skb_clone(frame->skb_prp, GFP_ATOMIC);
Both calls directly modify the path or LAN ID in the data buffer of the
original shared SKB before cloning it. Does the second modification for
the second slave overwrite the first modification in the same underlying
buffer, causing the first slave's cloned SKB to be transmitted with the
wrong tag?
>
> return hsr_dev_finalize(dev, link, interlink, multicast_spec,
> proto_version, extack);
