netdev - Re: [RESEND PATCH net-next 2/4] net: hsr: add offloading 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:   Wed, 3 Feb 2021 14:43:13 -0600
From:   George McCollister <george.mccollister@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jonathan Corbet <corbet@....net>, netdev@...r.kernel.org
Subject: Re: [RESEND PATCH net-next 2/4] net: hsr: add offloading support

On Mon, Feb 1, 2021 at 9:23 AM Vladimir Oltean <olteanv@...il.com> wrote:
[snip]
> > @@ -357,6 +367,7 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> >  {
> >       struct hsr_port *port;
> >       struct sk_buff *skb;
> > +     bool sent = false;
> >
> >       hsr_for_each_port(frame->port_rcv->hsr, port) {
> >               struct hsr_priv *hsr = port->hsr;
> > @@ -372,6 +383,12 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> >               if (port->type != HSR_PT_MASTER && frame->is_local_exclusive)
> >                       continue;
> >
> > +             /* If hardware duplicate generation is enabled, only send out
> > +              * one port.
> > +              */
> > +             if ((port->dev->features & NETIF_F_HW_HSR_DUP) && sent)
> > +                     continue;
> > +
>
> Is this right? It looks to me that you are not bypassing only duplicate
> generation, but also duplicate elimination.

I missed this part in my last reply.

When the duplicate generation is offloaded it simply doesn't attempt
to send the frame out more than one slave device.

It won't skip delivering to the master because NETIF_F_HW_HSR_DUP is
not set on the master device. Again the way this loop works is
confusing (at least to me) because it's handling incoming and outgoing
frames in the same loop.

The duplicate elimination prevents sending a frame with the same
sequence number and source twice to the same device.

It's not bypassing the duplicate elimination. You could certainly have
a switch that sends frames out both redundant ports automatically but
didn't eliminate duplicates being delivered to the CPU facing port.

Let me know if you still think there is a problem and we can discuss it further.

>
> I think this is duplicate elimination:
>
>                 /* Don't send frame over port where it has been sent before.
>                  * Also fro SAN, this shouldn't be done.
>                  */
>                 if (!frame->is_from_san &&
>                     hsr_register_frame_out(port, frame->node_src,
>                                            frame->sequence_nr))
>                         continue;
>
> and this is duplicate generation:
>
>         hsr_for_each_port(frame->port_rcv->hsr, port) {
>                 ...
>                 skb->dev = port->dev;
>                 if (port->type == HSR_PT_MASTER)
>                         hsr_deliver_master(skb, port->dev, frame->node_src);
>                 else
>                         hsr_xmit(skb, port, frame);
>         }
>
> So if this is the description of NETIF_F_HW_HSR_DUP:
>
> | Duplication involves the switch automatically sending a single frame
> | from the CPU port to both redundant ports. This is required because the
> | inserted HSR/PRP header/trailer must contain the same sequence number
> | on the frames sent out both redundant ports.
>
> then NETIF_F_HW_HSR_DUP is a misnomer. You should think of either
> grouping duplicate elimination and generation together, or rethinking
> the whole features system.
>
> >               /* Don't send frame over port where it has been sent before.
> >                * Also fro SAN, this shouldn't be done.
> >                */
[snip]