| 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
| ||
|
Message-ID: <af7de03d-6d31-4659-b1f0-e571fbc77cb0@redhat.com>
Date: Tue, 25 Feb 2025 11:58:32 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Jaakko Karrenpalo <jkarrenpalo@...il.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Simon Horman <horms@...nel.org>,
Lukasz Majewski <lukma@...x.de>, MD Danish Anwar <danishanwar@...com>
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
Jaakko Karrenpalo <jaakko.karrenpalo@...abb.com>
Subject: Re: [PATCH net-next v2 1/2] net: hsr: Fix PRP duplicate detection
On 2/21/25 11:10 AM, Jaakko Karrenpalo wrote:
> From: Jaakko Karrenpalo <jaakko.karrenpalo@...abb.com>
>
> Add PRP specific function for handling duplicate
> packets. This is needed because of potential
> L2 802.1p prioritization done by network switches.
This is IMHO a too terse description of the whys. I don't see how
prioritization should create duplicates. Please expand the commit
message. A cover letter could help
>
> Signed-off-by: Jaakko Karrenpalo <jaakko.karrenpalo@...abb.com>
> Signed-off-by: Jaakko Karrenpalo <jkarrenpalo@...il.com>
Just one of the 2 ;)
> ---
> net/hsr/hsr_device.c | 2 +
> net/hsr/hsr_forward.c | 4 +-
> net/hsr/hsr_framereg.c | 93 ++++++++++++++++++++++++++++++++++++++++--
> net/hsr/hsr_framereg.h | 8 +++-
> net/hsr/hsr_main.h | 2 +
> 5 files changed, 102 insertions(+), 7 deletions(-)
>
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index b6fb18469439..2c43776b7c4f 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -616,6 +616,7 @@ static struct hsr_proto_ops hsr_ops = {
> .drop_frame = hsr_drop_frame,
> .fill_frame_info = hsr_fill_frame_info,
> .invalid_dan_ingress_frame = hsr_invalid_dan_ingress_frame,
> + .register_frame_out = hsr_register_frame_out,
> };
>
> static struct hsr_proto_ops prp_ops = {
> @@ -626,6 +627,7 @@ static struct hsr_proto_ops prp_ops = {
> .fill_frame_info = prp_fill_frame_info,
> .handle_san_frame = prp_handle_san_frame,
> .update_san_info = prp_update_san_info,
> + .register_frame_out = prp_register_frame_out,
> };
>
> void hsr_dev_setup(struct net_device *dev)
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index a4bacf198555..aebeced10ad8 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -536,8 +536,8 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> * Also for SAN, this shouldn't be done.
> */
> if (!frame->is_from_san &&
> - hsr_register_frame_out(port, frame->node_src,
> - frame->sequence_nr))
> + hsr->proto_ops->register_frame_out &&
> + hsr->proto_ops->register_frame_out(port, frame))
The formatting is quite inconsistent and hard to follow in several
places. I'm surprised checkpatch does not complain. Please align the
condition to the relevant bracket.
[...]
> @@ -482,9 +486,11 @@ void hsr_register_frame_in(struct hsr_node *node, struct hsr_port *port,
> * 0 otherwise, or
> * negative error code on error
> */
> -int hsr_register_frame_out(struct hsr_port *port, struct hsr_node *node,
> - u16 sequence_nr)
> +int hsr_register_frame_out(struct hsr_port *port, struct hsr_frame_info *frame)
> {
> + struct hsr_node *node = frame->node_src;
> + u16 sequence_nr = frame->sequence_nr;
> +
> spin_lock_bh(&node->seq_out_lock);
> if (seq_nr_before_or_eq(sequence_nr, node->seq_out[port->type]) &&
> time_is_after_jiffies(node->time_out[port->type] +
> @@ -499,6 +505,87 @@ int hsr_register_frame_out(struct hsr_port *port, struct hsr_node *node,
> return 0;
> }
>
> +/* Adaptation of the PRP duplicate discard algorithm described in wireshark
> + * wiki (https://wiki.wireshark.org/PRP)
> + *
> + * A drop window is maintained for both LANs with start sequence set to the
> + * first sequence accepted on the LAN that has not been seen on the other LAN,
> + * and expected sequence set to the latest received sequence number plus one.
> + *
> + * When a frame is received on either LAN it is compared against the received
> + * frames on the other LAN. If it is outside the drop window of the other LAN
> + * the frame is accepted and the drop window is updated.
> + * The drop window for the other LAN is reset.
> + *
> + * 'port' is the outgoing interface
> + * 'frame' is the frame to be sent
> + *
> + * Return:
> + * 1 if frame can be shown to have been sent recently on this interface,
> + * 0 otherwise
> + */
> +int prp_register_frame_out(struct hsr_port *port, struct hsr_frame_info *frame)
> +{
> + enum hsr_port_type other_port;
> + enum hsr_port_type rcv_port;
> + struct hsr_node *node;
> + u16 sequence_nr;
> +
> + /* out-going frames are always in order
> + *and can be checked the same way as for HSR
> + */
> + if (frame->port_rcv->type == HSR_PT_MASTER)
> + return hsr_register_frame_out(port, frame);
> +
> + /* for PRP we should only forward frames from the slave ports
> + * to the master port
> + */
> + if (port->type != HSR_PT_MASTER)
> + return 1;
> +
> + node = frame->node_src;
> + sequence_nr = frame->sequence_nr;
> + rcv_port = frame->port_rcv->type;
> + other_port =
> + rcv_port == HSR_PT_SLAVE_A ? HSR_PT_SLAVE_B : HSR_PT_SLAVE_A;
> +
> + spin_lock_bh(&node->seq_out_lock);
> + if (time_is_before_jiffies(node->time_out[port->type] +
> + msecs_to_jiffies(HSR_ENTRY_FORGET_TIME)) ||
> + (node->seq_start[rcv_port] == node->seq_expected[rcv_port] &&
> + node->seq_start[other_port] == node->seq_expected[other_port])) {
> + /* the node hasn't been sending for a while
> + * or both drop windows are empty, forward the frame
> + */
> + node->seq_start[rcv_port] = sequence_nr;
> + } else if (seq_nr_before(sequence_nr, node->seq_expected[other_port]) &&
> + seq_nr_before_or_eq(node->seq_start[other_port], sequence_nr)) {
> + /* drop the frame, update the drop window for the other port
> + * and reset our drop window
> + */
> + node->seq_start[other_port] = sequence_nr + 1;
> + node->seq_expected[rcv_port] = sequence_nr + 1;
> + node->seq_start[rcv_port] = node->seq_expected[rcv_port];
> + spin_unlock_bh(&node->seq_out_lock);
> + return 1;
> + }
> +
> + /* update the drop window for the port where this frame was received
> + * and clear the drop window for the other port
> + */
> + node->seq_start[other_port] = node->seq_expected[other_port];
> + node->seq_expected[rcv_port] = sequence_nr + 1;
> + if ((u16)(node->seq_expected[rcv_port] - node->seq_start[rcv_port])
> + > PRP_DROP_WINDOW_LEN)
> + node->seq_start[rcv_port] =
> + node->seq_expected[rcv_port] - PRP_DROP_WINDOW_LEN;
I could be too sensible, but the above really hurts my eyes;) something
alike:
u16 seq_exp, seq_diff;
// [...]
seq_exp = node->seq_expected[rcv_port]
seq_diff = seq_exp - node->seq_start[rcv_port];
if (seq_diff > PRP_DROP_WINDOW_LEN)
node->seq_start[rcv_port] = seq_exp - PRP_DROP_WINDOW_LEN;
would be better.
Cheers,
Paolo
Powered by blists - more mailing lists