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  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:   Sat, 12 Dec 2020 16:26:22 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        j.vosburgh@...il.com, vfalico@...il.com, andy@...yhouse.net,
        netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 2/4] net: dsa: Link aggregation support

On Fri, Dec 11, 2020 at 09:50:24PM +0100, Tobias Waldekranz wrote:
> On Tue, Dec 08, 2020 at 13:23, Vladimir Oltean <olteanv@...il.com> wrote:
> > Sorry it took so long. I wanted to understand:
> > (a) where are the challenged for drivers to uniformly support software
> >     bridging when they already have code for bridge offloading. I found
> >     the following issues:
> >     - We have taggers that unconditionally set skb->offload_fwd_mark = 1,
> >       which kind of prevents software bridging. I'm not sure what the
> >       fix for these should be.
>
> I took a closer look at the software fallback mode for LAGs and I've
> found three issues that prevent this from working in a bridged setup,
> two of which are easy to fix. This is the setup (team0 is _not_
> offloaded):
>
> (A)  br0
>      /
>   team0
>    / \
> swp0 swp1
>
>
> 1. DSA tries to offload port attributes for standalone ports. So in this
>    setup, if vlan filtering is enabled on br0, we will enable it in
>    hardware which on mv88e6xxx causes swp0/1 to drop all packets on
>    ingress due to a VTU violation. This is a very easy fix, I will
>    include it in v4.

Makes sense, I did not enable VLAN filtering when I tried it.

> 2. The issue Vladimir mentioned above. This is also a straight forward
>    fix, I have patch for tag_dsa, making sure that offload_fwd_mark is
>    never set for ports in standalone mode.
>
>    I am not sure if I should solve it like that or if we should just
>    clear the mark in dsa_switch_rcv if the dp does not have a
>    bridge_dev. I know both Vladimir and I were leaning towards each
>    tagger solving it internally. But looking at the code, I get the
>    feeling that all taggers will end up copying the same block of code
>    anyway. What do you think?

I am not sure what constitutes a good separation between DSA and taggers
here. We have many taggers that just set skb->offload_fwd_mark = 1. We
could have this as an opportunity to even let DSA take the decision
altogether. What do you say if we stop setting skb->offload_fwd_mark
from taggers, just add this:

+#define DSA_SKB_TRAPPED	BIT(0)
+
 struct dsa_skb_cb {
 	struct sk_buff *clone;
+	unsigned long flags;
 };

and basically just reverse the logic. Make taggers just assign this flag
for packets which are known to have reached software via data or control
traps. Don't make the taggers set skb->offload_fwd_mark = 1 if they
don't need to. Let DSA take that decision upon a more complex thought
process, which looks at DSA_SKB_CB(skb)->flags & DSA_SKB_TRAPPED too,
among other things.

> With these two patches in place, setup (A) works as expected. But if you
> extend it to (team0 still not offloaded):
>
> (B)   br0
>      /   \
>   team0   \
>    / \     \
> swp0 swp1  swp2
>
> You instantly run into:
>
> 3. Only traffic which does _not_ have offload_fwd_mark set is allowed to
>    pass from swp2 to team0. This is because the bridge uses
>    dev_get_port_parent_id to figure out which ports belong to the same
>    switch. This will recurse down through all lowers and find swp0/1
>    which will answer with the same ID as swp2.
>
>    In the case where team0 is offloaded, this is exactly what we want,
>    but in a setup like (B) they do not have the same "logical" parent in
>    the sense that br0 is led to believe. I.e. the hardware will never
>    forward packets between swp0/1 and swp2.
>
>    I do not see an obvious solution to this. Refusing to divulge the
>    parent just because you are a part of a software LAG seems fraught
>    with danger as there are other users of those APIs. Adding yet
>    another ndo would theoretically be possible, but not
>    desirable. Ideas?
>
> As for this series, my intention is to make sure that (A) works as
> intended, leaving (B) for another day. Does that seem reasonable?
>
> NOTE: In the offloaded case, (B) will of course also be supported.

Yeah, ok, one can already tell that the way I've tested this setup was
by commenting out skb->offload_fwd_mark = 1 altogether. It seems ok to
postpone this a bit.

For what it's worth, in the giant "RX filtering for DSA switches" fiasco
https://patchwork.ozlabs.org/project/netdev/patch/20200521211036.668624-11-olteanv@gmail.com/
we seemed to reach the conclusion that it would be ok to add a new NDO
answering the question "can this interface do forwarding in hardware
towards this other interface". We can probably start with the question
being asked for L2 forwarding only.

Maybe you are just the kick I need to come back to that series and
simplify it.

Powered by blists - more mailing lists